[BUGFIX] Fix autoload to work properly with composer dependencies #401
Conversation
bin/validate-json
Outdated
| require_once $fileName; | ||
| } | ||
| // support running this tool from git checkout | ||
| if (is_dir(__DIR__ . '/../vendor')) { |
There was a problem hiding this comment.
Would dirname work better here to help avoid the path directory separator headache?
There was a problem hiding this comment.
Can you clarify? PATH_SEPARATOR has nothing to do with filesystem paths - it's the ':' character used as a list separator when defining multiple search paths (ie. what you'll find in the $PATH environment variable). Not sure how dirname has anything to do with that...
There was a problem hiding this comment.
I edited the comment to refer to directory separator; I didn't realize my phrasing collided with that other constant at the time I wrote it.
Screwing with relative paths in strings to wire things up always seems dirty to me, especially with directory separators differing between platforms.
An example of what I am suggesting:
$ cd /path/to/json-schema/bin
$ php -a
Interactive shell
php > echo __DIR__;
/path/to/json-schema/bin
php > echo dirname(__DIR__);
/path/to/json-schema
php > echo dirname(__DIR__) . DIRECTORY_SEPARATOR . 'vendor';
/path/to/json-schema/vendorWhat do you think?
There was a problem hiding this comment.
Works for me, I'll do that now.
|
Wow, nice! LGTM |
| } | ||
| require($composerAutoload); |
There was a problem hiding this comment.
just curiosity, why not just require __DIR__.'/../vendor/autoload.php'; ?
There was a problem hiding this comment.
So that it can be resolved from anywhere in the include path. If you're asking "why use dirname", that's because @bighappyface doesn't like the relative path & hardcoded Unix directory separator.
There was a problem hiding this comment.
ok, it seems a bit odd to me to alter the include_path to access a file you know the absolute path; but well, in a binary it does not matter much
There was a problem hiding this comment.
It seems odd to me too. The impression I got from reading the original code is that the author anticipated the utility script being split from the rest of the package and installed elsewhere, and the include path business is because of that. I don't know the reasons behind that approach being present in the code, so I kept it when I made the change.
erayd
changed the title
What
Replace the autoloader in
validate-jsonwith the composer one.Why
Because the current autoloader cannot handle dependencies, which means that
validate-jsoncannot findMabeEnum\Enum.