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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Filesystem] Fixed makePathRelative #22321

Closed
wants to merge 11 commits into
base: 2.7
from

Conversation

@ausi
Contributor

ausi commented Apr 6, 2017

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) that test the behavior of relative paths, cec473e

This pull request fixes all these issues and adds tests for them.

ausi added some commits Apr 6, 2017

Remove unnecessary if construct
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.

@ausi ausi changed the title from Fix/make path relative to [Filesystem] Fixed makePathRelative Apr 6, 2017

@agoat

This comment has been minimized.

Show comment
Hide comment
@agoat

agoat commented Apr 6, 2017

+1

@LinkingYou

This comment has been minimized.

Show comment
Hide comment
@LinkingYou

LinkingYou commented Apr 6, 2017

+1

Show outdated Hide outdated src/Symfony/Component/Filesystem/Filesystem.php
@@ -878,6 +879,17 @@ public function providePathsForMakePathRelative()
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/'),

This comment has been minimized.

@xabbuh

xabbuh Apr 7, 2017

Member

This is not something that is supported. The docblock states that the paths have to be absolute.

@xabbuh

xabbuh Apr 7, 2017

Member

This is not something that is supported. The docblock states that the paths have to be absolute.

This comment has been minimized.

@ausi

ausi Apr 7, 2017

Contributor

There was already an existing unit test (FilesystemTest.php:1097) that tested relative paths.

@ausi

ausi Apr 7, 2017

Contributor

There was already an existing unit test (FilesystemTest.php:1097) that tested relative paths.

This comment has been minimized.

@chalasr

chalasr Aug 8, 2017

Member

Let's not add new ones. Tests should respect the contract which is to pass an absolute path.

@chalasr

chalasr Aug 8, 2017

Member

Let's not add new ones. Tests should respect the contract which is to pass an absolute path.

This comment has been minimized.

@fritzmg

fritzmg Aug 8, 2017

Contributor

@chalasr the function always supported relative paths (before 3.2.7). There are even pre-existing test cases for that.

@fritzmg

fritzmg Aug 8, 2017

Contributor

@chalasr the function always supported relative paths (before 3.2.7). There are even pre-existing test cases for that.

This comment has been minimized.

@ausi

ausi Aug 9, 2017

Contributor

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?

@ausi

ausi Aug 9, 2017

Contributor

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?

This comment has been minimized.

@chalasr

chalasr Aug 9, 2017

Member

Can you please provide a new link to this test? Previous one is outdated and I can't find it

@chalasr

chalasr Aug 9, 2017

Member

Can you please provide a new link to this test? Previous one is outdated and I can't find it

This comment has been minimized.

@ausi

ausi Aug 9, 2017

Contributor

Yes, it’s line 1102 in the current master branch:

array('var/lib/symfony/', 'var/lib/symfony/src/Symfony/Component', '../../../'),

@ausi

ausi Aug 9, 2017

Contributor

Yes, it’s line 1102 in the current master branch:

array('var/lib/symfony/', 'var/lib/symfony/src/Symfony/Component', '../../../'),

This comment has been minimized.

@chalasr

chalasr Aug 9, 2017

Member

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.

@chalasr

chalasr Aug 9, 2017

Member

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.

This comment has been minimized.

@ausi

ausi Aug 10, 2017

Contributor

Done in a2a1d0d

@ausi

ausi Aug 10, 2017

Contributor

Done in a2a1d0d

This comment has been minimized.

@xabbuh

xabbuh Aug 10, 2017

Member

@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. :)

@xabbuh

xabbuh Aug 10, 2017

Member

@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. :)

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Jun 9, 2017

Contributor

@xabbuh any updates on this? Can I do something to get this bugfix merged?

Contributor

ausi commented Jun 9, 2017

@xabbuh any updates on this? Can I do something to get this bugfix merged?

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Jun 10, 2017

Contributor

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.

Start End Expected
"" "" "./"
"aa/" "" "../"
"" "aa/" "aa/"

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.

Contributor

robfrawley commented Jun 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.

Start End Expected
"" "" "./"
"aa/" "" "../"
"" "aa/" "aa/"

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

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Jun 10, 2017

Contributor

I think it should be allowed to pass an empty string. If "aa/" is a valid path then "" should be too, otherwise every developer that uses this method has to handle empty strings themselves. Your three examples are prefectly fine for me, did you expect any different output?

But I think there are cases that cannot produce a meaningful result, like start "../../" and end "../". For these cases it might make sense to throw an exception.

Contributor

ausi commented Jun 10, 2017

I think it should be allowed to pass an empty string. If "aa/" is a valid path then "" should be too, otherwise every developer that uses this method has to handle empty strings themselves. Your three examples are prefectly fine for me, did you expect any different output?

But I think there are cases that cannot produce a meaningful result, like start "../../" and end "../". For these cases it might make sense to throw an exception.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Aug 7, 2017

Contributor

Is there anything I can do to get these fixes merged?

Contributor

ausi commented Aug 7, 2017

Is there anything I can do to get these fixes merged?

Show outdated Hide outdated src/Symfony/Component/Filesystem/Filesystem.php
@@ -878,6 +879,17 @@ public function providePathsForMakePathRelative()
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/'),

This comment has been minimized.

@chalasr

chalasr Aug 8, 2017

Member

Let's not add new ones. Tests should respect the contract which is to pass an absolute path.

@chalasr

chalasr Aug 8, 2017

Member

Let's not add new ones. Tests should respect the contract which is to pass an absolute path.

Revert "Fix parameter description"
This reverts commit cec473e.
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Aug 10, 2017

Member

Does the PR now still solve anything at all? I miss a new test case that would fail otherwise.

Member

xabbuh commented Aug 10, 2017

Does the PR now still solve anything at all? I miss a new test case that would fail otherwise.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Aug 10, 2017

Contributor

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.

Contributor

ausi commented Aug 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

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Aug 10, 2017

Member

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

Member

xabbuh commented Aug 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

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Aug 10, 2017

Contributor

I agree, that’s why my pull request included the doccomment change and the tests in the first place.

IMO the “feature” that relative paths are supported was always there (including a unit test) and got broken in 3.2.7.

Should I remove 193cdf8 and a2a1d0d from this pull request?

Contributor

ausi commented Aug 10, 2017

I agree, that’s why my pull request included the doccomment change and the tests in the first place.

IMO the “feature” that relative paths are supported was always there (including a unit test) and got broken in 3.2.7.

Should I remove 193cdf8 and a2a1d0d from this pull request?

@xelaris

This comment has been minimized.

Show comment
Hide comment
@xelaris

xelaris Aug 14, 2017

Contributor

As mentioned by @kubawerlos the SensioDistributionBundle relies on support for relative paths, when calling Filesystem::makePathRelative with symfony-var-dir and symfony-bin-dir from the extra settings in the composer.json (usually "var" and "bin").
When composer 1.5.0 was released a week ago, the version of symfony/filesystem included changed from v2.8.18 to v2.8.26. Since then the SensioDistributionBundle generates wrong require statements for SymfonyRequirements.php. Corresponding issues (sensiolabs/SensioDistributionBundle#324 and composer/composer#6599) were created in these repositories.

Contributor

xelaris commented Aug 14, 2017

As mentioned by @kubawerlos the SensioDistributionBundle relies on support for relative paths, when calling Filesystem::makePathRelative with symfony-var-dir and symfony-bin-dir from the extra settings in the composer.json (usually "var" and "bin").
When composer 1.5.0 was released a week ago, the version of symfony/filesystem included changed from v2.8.18 to v2.8.26. Since then the SensioDistributionBundle generates wrong require statements for SymfonyRequirements.php. Corresponding issues (sensiolabs/SensioDistributionBundle#324 and composer/composer#6599) were created in these repositories.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Sep 10, 2017

Contributor

So there are already two known projects that got broken because of this issue. Wouldn’t it make sense to merge this?

Should I remove 193cdf8 and a2a1d0d from this pull request?

I removed these commits now from this pull request.

Contributor

ausi commented Sep 10, 2017

So there are already two known projects that got broken because of this issue. Wouldn’t it make sense to merge this?

Should I remove 193cdf8 and a2a1d0d from this pull request?

I removed these commits now from this pull request.

@kubawerlos

This comment has been minimized.

Show comment
Hide comment
@kubawerlos

kubawerlos Sep 10, 2017

Contributor

Haven't been tracing this, but in Symfony 3.3.8 it's back to correct paths.

Contributor

kubawerlos commented Sep 10, 2017

Haven't been tracing this, but in Symfony 3.3.8 it's back to correct paths.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Sep 10, 2017

Contributor

@kubawerlos I don’t see any change to the makePathRelative() method since version 3.2.7, could you point to the commit where it got fixed?

What is the result of makePathRelative('aa/cc', 'bb/cc') in your installation?

Contributor

ausi commented Sep 10, 2017

@kubawerlos I don’t see any change to the makePathRelative() method since version 3.2.7, could you point to the commit where it got fixed?

What is the result of makePathRelative('aa/cc', 'bb/cc') in your installation?

@kubawerlos

This comment has been minimized.

Show comment
Hide comment
@kubawerlos

kubawerlos Sep 10, 2017

Contributor

@ausi just found it, it was fixed directly in SensioDistributionBundle, so the problem still exists here.

Contributor

kubawerlos commented Sep 10, 2017

@ausi just found it, it was fixed directly in SensioDistributionBundle, so the problem still exists here.

ausi referenced this pull request in sensiolabs/SensioDistributionBundle Sep 13, 2017

ausi added some commits Sep 14, 2017

@xabbuh

xabbuh approved these changes Sep 15, 2017

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Sep 16, 2017

Contributor

@chalasr your review status still shows up as “requested changes”, are there any changes I’ve forgotton?

Contributor

ausi commented Sep 16, 2017

@chalasr your review status still shows up as “requested changes”, are there any changes I’ve forgotton?

@fabpot

fabpot approved these changes Sep 17, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 17, 2017

Member

Thank you @ausi.

Member

fabpot commented Sep 17, 2017

Thank you @ausi.

fabpot added a commit that referenced this pull request Sep 17, 2017

bug #22321 [Filesystem] Fixed makePathRelative (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/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

@fabpot fabpot closed this Sep 17, 2017

This was referenced Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment