fixed the filesystem loader with relative paths #2195
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stof
reviewed
Oct 20, 2016
| { | ||
| $this->rootPath = realpath(null !== $rootPath ? $rootPath : getcwd()).DIRECTORY_SEPARATOR; |
There was a problem hiding this comment.
this is the place being broken if $rootPath is inside a phar, as $this->rootPath will be false
There was a problem hiding this comment.
I would say that we don't want to support this use case (if everything is in a phar, then using an absolute path is not really an issue anymore). So, if realpath returns false here, I would prefer to throw an exception.
There was a problem hiding this comment.
well, if someone uses relative path for the loader paths and passes __DIR__ as root path (instead of making all paths absolute in the loader paths), your suggestion would break the project once it gets bundled as phar.
This seems worse than forbidding passing relative paths to the loader altogether.
fabpot
force-pushed
the
loader-relative-paths
branch
6 times, most recently
from
e5041b6 to
2fd5b0f
Compare
|
Ready. |
fabpot
force-pushed
the
loader-relative-paths
branch
from
2fd5b0f to
5f25a11
Compare
Tobion
reviewed
Oct 21, 2016
| /** | ||
| * Constructor. | ||
| * | ||
| * @param string|array $paths A path or an array of paths where to look for templates | ||
| * @param string|array $paths A path or an array of paths where to look for templates | ||
| * @param string $rootPath The root path common to all relative paths |
fabpot
force-pushed
the
loader-relative-paths
branch
from
5f25a11 to
a343c92
Compare
fabpot
added a commit
that referenced
this pull request
Oct 21, 2016
This PR was merged into the 1.x branch. Discussion ---------- fixed the filesystem loader with relative paths closes #2145 TODO - [x] document the new `$rootPath` option - [x] add test with a `$rootPath` different from `getcwd()` - [x] test the cache key does not vary when `$rootPath` changes Commits ------- a343c92 fixed the filesystem loader with relative paths
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #2145
TODO
$rootPathoption$rootPathdifferent fromgetcwd()$rootPathchanges