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] normalize paths before making them relative #22133

Merged
merged 1 commit into from Mar 24, 2017

Conversation

Projects
None yet
8 participants
@xabbuh
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
Show outdated Hide outdated src/Symfony/Component/Filesystem/Tests/FilesystemTest.php
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/'),

This comment has been minimized.

@jakzal

jakzal Mar 23, 2017

Member

These two are not absolute paths while makePathRelative() says it accepts two absolute paths.

@jakzal

jakzal Mar 23, 2017

Member

These two are not absolute paths while makePathRelative() says it accepts two absolute paths.

This comment has been minimized.

@Tobion

Tobion Mar 23, 2017

Member

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

@Tobion

Tobion Mar 23, 2017

Member

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

This comment has been minimized.

@ro0NL

ro0NL Mar 24, 2017

Contributor

👍 for returning as is.

@ro0NL

ro0NL Mar 24, 2017

Contributor

👍 for returning as is.

This comment has been minimized.

@xabbuh

xabbuh Mar 24, 2017

Member

oops, that was not intended, fixed

@xabbuh

xabbuh Mar 24, 2017

Member

oops, that was not intended, fixed

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Mar 24, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 24, 2017

Member

Thank you @xabbuh.

Member

fabpot commented Mar 24, 2017

Thank you @xabbuh.

@fabpot fabpot merged commit d50ffa1 into symfony:2.7 Mar 24, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 24, 2017

bug #22133 [Filesystem] normalize paths before making them relative (…
…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

@xabbuh xabbuh deleted the xabbuh:issue-22083 branch Mar 25, 2017

This was referenced Apr 4, 2017

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Apr 7, 2017

Contributor

Unfortunately, the unit tests for this change have not been comprehensive, so we didn't realize that the change breaks backwards compatibility (see #22321).

Contributor

leofeyer commented Apr 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).

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment