[WIP][Filesystem] Several issues with Filesystem::makePathRelative #25169
Conversation
Simperfit
force-pushed
the
feature/fix-make-relative-path
branch
from
58ad30b
to
04c1efa
Compare
|
For 2.7? |
| @@ -1094,44 +1094,46 @@ public function providePathsForMakePathRelative() | |||
| $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/', '../'), | |||
| array('/var/lib/symfony/src/Symfony', '/var/lib/symfony/src/Symfony/Component', '../'), | |||
| 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.
not sure why you change some tests...? Can you give more context? 🤔
There was a problem hiding this comment.
seems like he just removed a duplicated assertion :) missed the leading slash
There was a problem hiding this comment.
I've removed some dupplicate and the third point of the issue says that we should be able to dectect the trailing slash, that's why.
There was a problem hiding this comment.
I agree. But we should not remove the tests without the trailing slash either, just add new ones as both ways should work :-)
Simperfit
force-pushed
the
feature/fix-make-relative-path
branch
from
04c1efa
to
a69279c
Compare
|
rebased on 2.7 |
| $endPathRemainder = implode('/', array_slice($endPathArr, $index)); | ||
|
|
||
| // Construct $endPath from traversing to the common path, then to the remaining $endPath | ||
| $relativePath = $traverser.('' !== $endPathRemainder ? $endPathRemainder.'/' : ''); | ||
| if ('/' !== substr($endPath, -1)) { |
Simperfit
force-pushed
the
feature/fix-make-relative-path
branch
2 times, most recently
from
9225f6d
to
53700b6
Compare
Simperfit
force-pushed
the
feature/fix-make-relative-path
branch
from
53700b6
to
7acfd17
Compare
Simperfit
changed the title
|
|
||
| if ($endPath[1] !== $startPath[1]) { | ||
| return ''; |
There was a problem hiding this comment.
Why do you return empty string here? It doesn't make sense. Maybe throw exception?
| @@ -849,56 +1100,65 @@ public function providePathsForMakePathRelative() | |||
| array('/usr/lib/symfony/', '/var/lib/symfony/src/Symfony/Component', '../../../../../../usr/lib/symfony/'), | |||
| array('usr/lib/symfony/', 'var/lib/symfony/src/Symfony/Component', '../../../../../../usr/lib/symfony/'), | |||
| array('/var/lib/symfony/src/Symfony/', '/var/lib/symfony/', 'src/Symfony/'), | |||
| array('/aa/bb', '/aa/bb', './'), | |||
There was a problem hiding this comment.
Why are you changing these? Some changes you have done were already tested. E.g. you change here /aa/bb to /aa/bb/, but that one already exists. So much changes in tests is reason why everybody is hesitant to review this. Please go through your changes in tests and only change those that are wrong, otherwise just add new ones.
| @@ -370,19 +370,25 @@ public function makePathRelative($endPath, $startPath) | |||
| if ('\\' === DIRECTORY_SEPARATOR) { | |||
There was a problem hiding this comment.
Comment above needs to be updated. This block no longer just normalizes separators
| } | ||
| $stripDriveLetter = function ($path) { | ||
| if (strlen($path) > 2 && ':' === $path[1] && '/' === $path[2] && ctype_alpha($path[0])) { | ||
| return array(substr($path, 2), $path[0].$path[1]); |
There was a problem hiding this comment.
I wouldn't do this here, you changed what this function does - now name of function is wrong. I suggest to do something like
$isAbsolutePath = function($path) {
return strlen($path) > 2 && ':' === $path[1] && '/' === $path[2] && ctype_alpha($path[0]);
}
if ($isAbsolutePath($endPath) && $isAbsolutePath($startPath) && substr($endPath, 0, 2) !== substr($startPath, 0, 2)) {
throw new \InvalidArgumentException("You can't make relative links across drives");
}
$stripDriveLetter = function ($path) (use $isAbsolutePath) {
if ($isAbsolutePath) {
return substr($path, 2);
}
return $path;
};
$endPath = $stripDriveLetter($endPath);
$startPath = $stripDriveLetter($startPath);|
|
||
| if ($endPath[1] !== $startPath[1]) { |
There was a problem hiding this comment.
This will do unexpected things if paths don't contain :. Array is returned only when it does. So when it doesn't, you are getting here second string character of relative path.
|
Closing as there is no more activity here. Feel free to reopen though (related issue is kept open). |
I needed to open my windows in order to test this ;).
