Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for binaries (address #1) #12

Merged
merged 3 commits into from Jul 1, 2015

Conversation

wjzijderveld
Copy link
Contributor

This is a hacked solution, but works :)

Just wanted to open the PR to discuss options.

Our use case is the following:
We use phpunit as a dev-dependency and use the executable provided to run it.
At first I just ran the phpunit binary that gets installed by the root composer. That didn't work, as it then loads 2 autoloaders: the one from the root and the one from the component because we tell in phpunit.dist.xml to load the bootstrap, which loads the autoloader from the component.

Undecided

  • The target dir for the binaries (probably implement bin-dir like composer has?), as it now hardcodes to <componentDir>/bin

@beberlei
Copy link
Owner

Great! Can we start with vendor/bin though? That is save directory to put stuff for now, and then yes we need "bin-dir" support.

@wjzijderveld
Copy link
Contributor Author

Makes sense :)

But while making the adjustments I found an problem. With my current implementation it just happened to work by accident :-)

In the case of phpunit, it tries to load the autoload by looking in 3 locations:
array(__DIR__ . '/../../autoload.php', __DIR__ . '/../vendor/autoload.php', __DIR__ . '/vendor/autoload.php')
This is a problem when we copy the executable to the vendor/bin directory, as it will never find the autoload.php When I symlink the file, I have the same problem I began with: it then tries to load the autoloader from the root package (as that is where to original executable is).

I'm not even sure this is easy to fix, as there is no way to tell how the executable tries to load files. It might try to use the autoloader, but it might also just try to include files relative from it's own location.

One thing that works with the autoloader, is to change the require in autoload_files.php to require_once. But I doubt composer is willing to change that...

@wjzijderveld wjzijderveld changed the title WIP: Adding support for binaries (address #1) Adding support for binaries (address #1) Jun 15, 2015
@wjzijderveld
Copy link
Contributor Author

Just updated the PR.

  • Adjusted the dumper to use require_once for files (no longer using the composerRequire function).
  • I added a test for the autoloading part.
  • Also needed to adjust a test to get them running. ([foo,bar] vs [bar,foo])

@wjzijderveld
Copy link
Contributor Author

Found another edge-case yesterday. I'm not sure this is something Fiddler can and should account for, but for my case it's quit annoying :-)

I was trying to get phpspec working with our components and fiddler, but they retrieve the autoloader pretty weird:

  • They first load the autoloader based on getcwd()
  • After that (regardless if it succeeded or failed) it will load the autoloader based on FILE.

This would mean (when executed from the component), that it will first get the autoloader from the component. And after that it will load the autoloader from root.
The autoloader from the root will then try to autoload files again, causing 'cannot redeclare' errors.

When executed from the root it would mean the autoloader wouldn't know about the files from the component, so all tests fail 😄

Only solution I see now, is removing autoloading files from Fiddler all together, but allow for defining specific in the Fiddler.json, for cases where you do need it? Or if that turns out to be to invasive, maybe add an option to fiddler to skip autoloading files?

beberlei added a commit that referenced this pull request Jul 1, 2015
Adding support for binaries (address #1)
@beberlei beberlei merged commit 3d83d1b into beberlei:master Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants