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] Fixed makePathRelative#22321
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
If `0 === $depth` evaluates to true `str_repeat('../', $depth)` would result in the empty string anyway, so there is no need to handle this case.agoat commentedApr 6, 2017
+1 |
LinkingYou commentedApr 6, 2017
+1 |
| * @param string $endPathAbsolute path of target | ||
| * @param string $startPathAbsolute path where traversal begins | ||
| * @param string $endPathPath of target | ||
| * @param string $startPathPath where traversal begins |
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.
This is a BC break. The method expects the paths to be absolute as documented.
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.
Well, then this should definitely be mentioned in the documentation. There's no hint that only absolute paths are targeted besides this inline doc comment.
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.
Why is this a BC break? Absolute paths work the same way as they did before.
Relative paths worked in the past and there are already unit tests (FilesystemTest.php:1097) that test the behavior of relative paths, so I think they should be allowed in the documentation of the method.
If relative paths should not be supported, I think we would have to throw an exception if a relative path gets passed, but that would be a BC break I think.
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.
It should support relative paths for some reasons:
- It worked before 3.2.7!
- Absolute paths will still work (No BC break).
- As@loilo mentioned, the documentation doesn't tell that only absolute paths can be used.
- It makes life easier (Otherwise everyone have to do extra checks on absolute paths, before using this method).
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.
I didn't see that test case initially. I'm this case, we IMO should indeed support this and update the docblock as suggested. But I think we should indeed think about deprecating this behaviour.
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.
Adding the “Absolute” back should then be part of#24202 I think. Otherwise I would have to remove the unit tests as@chalasr wrote in#22321 (comment)
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.
I see it different here: "Absolute" should still be part of the docblock to make our intent clear that we want this method to receive absolute paths. People who not look at the tests do not need to know that this currently works with relative paths too.
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.
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.
Just discussed with@xabbuh and we come to the conclusion that the docblock clearly states the passed path must be absolute, that has been decided when introducing the method and should not be changed now.
Given we have a test in place for relative paths, let's improve the current implementation regarding relative paths as done here, so we can officially deprecate passing relative paths to the method in#24202 (or officially support any path, changing the docblock, but I don't think it's the right direction as currently discussed on the PR) and move forward.
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.
| array('C:/aa/bb/../../cc','C:/aa/../dd/..','cc/'), | ||
| array('C:/../aa/bb/cc','C:/aa/dd/..','bb/cc/'), | ||
| array('C:/../../aa/../bb/cc','C:/aa/dd/..','../bb/cc/'), | ||
| array('aa/bb','aa/cc','../bb/'), |
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.
This is not something that is supported. The docblock states that the paths have to be absolute.
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.
There was already an existing unit test (FilesystemTest.php:1097) that tested relative 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.
Let's not add new ones. Tests should respect the contract which is to pass an absolute path.
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.
@chalasr the function always supported relative paths (before 3.2.7). There are even pre-existing test cases for that.
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.
Tests should respect the contract which is to pass an absolute path.
@chalasr Should I remove the pre-existing relative path tests then too? Or should I just remove the relative path tests that I added?
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.
Can you please provide a new link to this test? Previous one is outdated and I can't find it
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.
Yes, it’s line 1102 in the current master branch:
| array('var/lib/symfony/','var/lib/symfony/src/Symfony/Component','../../../'), |
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.
Indeed, it should be removed (we did revert a similar test for the very same reason#22133 (comment)). It's safe to do it here I think.
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.
Done ina2a1d0d
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.
@chalasr Actually, the example that you gave was a test I mistakenly added in that very same PR. So it was never part of a merge. :)
| } | ||
| $stripDriveLetter =function ($path) { | ||
| if (strlen($path) >2 &&substr($path,1,2) ===':/' &&ctype_alpha($path[0])) { |
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.
instead ofsubstr() I suggest to use':' === $path[1] && '/' === $path[2]
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.
I usedsubstr() because it’s done that way inisAbsolutePath()Filesystem.php:587. Should I change it though?
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.
Should I change thesubstr() to':' === $path[1] && '/' === $path[2] to get this pull request approved?
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@xabbuh's suggestion
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.
Done inbce33c4
ausi commentedJun 9, 2017
@xabbuh any updates on this? Can I do something to get this bugfix merged? |
robfrawley commentedJun 10, 2017
Overall, I love the bug fixes introduced in this PR with regard to relative paths. That said, it seems weird to allow empty input and expect any meaningful output.
Does this really make sense? Should passing empty strings be deprecated? I wouldn't say that most people would expect these outputs and it would likely just hide a bug within their API usage, IMHO. |
ausi commentedJun 10, 2017
I think it should be allowed to pass an empty string. If But I think there are cases that cannot produce a meaningful result, like start |
ausi commentedAug 7, 2017
Is there anything I can do to get these fixes merged? |
| * @param string $endPathAbsolute path of target | ||
| * @param string $startPathAbsolute path where traversal begins | ||
| * @param string $endPathPath of target | ||
| * @param string $startPathPath where traversal begins |
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.
This should be reverted, if we have to support it fully that should be discussed separately and done on 3.4, as said above.
| array('C:/aa/bb/../../cc','C:/aa/../dd/..','cc/'), | ||
| array('C:/../aa/bb/cc','C:/aa/dd/..','bb/cc/'), | ||
| array('C:/../../aa/../bb/cc','C:/aa/dd/..','../bb/cc/'), | ||
| array('aa/bb','aa/cc','../bb/'), |
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.
Let's not add new ones. Tests should respect the contract which is to pass an absolute path.
This reverts commitcec473e.
xabbuh commentedAug 10, 2017
Does the PR now still solve anything at all? I miss a new test case that would fail otherwise. |
ausi commentedAug 10, 2017
Yes, it still solves all the issues described in the initial issue comment. Especially the BC break that was introduced in 3.2.7. I removed the test cases because@chalasr requested that I should add them in a feature pull request for version 3.4. |
xabbuh commentedAug 10, 2017
Well, but it doesn't make sense to introduce a bugfix that in fact implements a feature that should be part of 3.4. So we need to decide whether not fully supporting relative path arguments was a BC break (because we had tests for relative paths despite the API docs telling the opposite) or if that is a new feature. And then there should be one PR containing proper tests for what the code change does achieve. ping @symfony/deciders |
ausi commentedAug 10, 2017
| { | ||
| $paths =array( | ||
| array('/var/lib/symfony/src/Symfony/','/var/lib/symfony/src/Symfony/Component','../'), | ||
| array('/var/lib/symfony/src/Symfony/','/var/lib/symfony/src/Symfony/Component/','../'), |
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.
Can we have simple casearray('aa', 'bb', '../aa/'), as this is example of whatinstallRequirementsFile fromSensio\Bundle\DistributionBundle\Composer\ScriptHandler is breaking in composer script in latest Symfony?
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.
Yes, this does work with my fixes. But if relative paths should be supported at all is currently in discussion as you can see in the comments of this pull request.
xelaris commentedAug 14, 2017
As mentioned by@kubawerlos the SensioDistributionBundle relies on support for relative paths, when calling |
a2a1d0d to28b29eeCompareausi commentedSep 10, 2017
kubawerlos commentedSep 10, 2017
Haven't been tracing this, but in Symfony 3.3.8 it's back to correct paths. |
ausi commentedSep 10, 2017
@kubawerlos I don’t see any change to the What is the result of |
kubawerlos commentedSep 10, 2017
@ausi just found it, it was fixed directly inSensioDistributionBundle, so the problem still exists here. |
ausi commentedSep 16, 2017
@chalasr your review status still shows up as “requested changes”, are there any changes I’ve forgotton? |
fabpot commentedSep 17, 2017
Thank you@ausi. |
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
Uh oh!
There was an error while loading.Please reload this page.
Updating to Symfony 3.2.7@agoat noticed a bug with
Filesystem::makePathRelative()incontao/core-bundle#751:makePathRelative('aa/cc', 'bb/cc')returned correctly../../aa/cc./I think this issue was introduced with#22133.
While working on the fix I noticed some other issues too:
./path segments,15982d4../got ignored at the beginning of relative paths,9586e88This pull request fixes all these issues and adds tests for them.