[Filesystem] normalize paths before making them relative #22133
Merged
Conversation
This file contains hidden or 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
Member
xabbuh
commented
Mar 23, 2017
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #22083 |
| License | MIT |
| Doc PR |
jakzal
reviewed
Mar 23, 2017
| array('/aa/../bb/cc', '/aa/dd/..', '../bb/cc/'), | ||
| array('/aa/bb/../../cc', '/aa/../dd/..', 'cc/'), | ||
| array('../aa/bb/cc', '/aa/dd/..', 'bb/cc/'), | ||
| array('../../aa/../bb/cc', '/aa/dd/..', '../bb/cc/'), |
There was a problem hiding this comment.
These two are not absolute paths while makePathRelative() says it accepts two absolute paths.
There was a problem hiding this comment.
Agree. Also this doesn't make sense. If the target path is already relative, what is it relative against? So just removing the leading dot segments changes the path as it's not clear what to refers to. So either you throw an exception that this is not accepted, or you can just return the relative target path as-is, since it is already relative. The latter is what I did in https://github.com/guzzle/psr7/blob/master/src/UriResolver.php#L149
There was a problem hiding this comment.
oops, that was not intended, fixed
|
Thank you @xabbuh. |
fabpot
added a commit
that referenced
this pull request
Mar 24, 2017
…xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [Filesystem] normalize paths before making them relative | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22083 | License | MIT | Doc PR | Commits ------- d50ffa1 normalize paths before making them relative
This was referenced Apr 4, 2017
Merged
Merged
Merged
|
Unfortunately, the unit tests for this change have not been comprehensive, so we didn't realize that the change breaks backwards compatibility (see #22321). |
fabpot
added a commit
that referenced
this pull request
Sep 17, 2017
This PR was squashed before being merged into the 2.7 branch (closes #22321). Discussion ---------- [Filesystem] Fixed makePathRelative | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Updating to Symfony 3.2.7 @agoat noticed a bug with `Filesystem::makePathRelative()` in contao/core-bundle#751: - In Symfony 3.2.6 `makePathRelative('aa/cc', 'bb/cc')` returned correctly `../../aa/cc` - In Symfony 3.2.7 the same method call returns `./` I think this issue was introduced with #22133. While working on the fix I noticed some other issues too: - An unnecessary if construct that did nothing, fc745f4 - Missing normalization of `./` path segments, 15982d4 - `../` got ignored at the beginning of relative paths, 9586e88 - The documentation of the method only allowed absolute paths, but there are already unit tests ([FilesystemTest.php:1097](https://github.com/symfony/symfony/blob/ab93feae3f9a16c4f18c5736435d18fa36338d2c/src/Symfony/Component/Filesystem/Tests/FilesystemTest.php#L1097)) that test the behavior of relative paths, cec473e This pull request fixes all these issues and adds tests for them. Commits ------- 2bc1150 [Filesystem] Fixed makePathRelative
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.