Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Filesystem] normalize paths before making them relative#22133
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedMar 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 |
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These two are not absolute paths while makePathRelative() says it accepts two absolute paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 inhttps://github.com/guzzle/psr7/blob/master/src/UriResolver.php#L149
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 for returning as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oops, that was not intended, fixed
fabpot commentedMar 24, 2017
Thank you@xabbuh. |
…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
leofeyer commentedApr 7, 2017
Unfortunately, the unit tests for this change have not been comprehensive, so we didn't realize that the change breaks backwards compatibility (see#22321). |
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/aUpdating to Symfony 3.2.7@agoat noticed a bug with `Filesystem::makePathRelative()` incontao/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,cec473eThis pull request fixes all these issues and adds tests for them.Commits-------2bc1150 [Filesystem] Fixed makePathRelative