[Finder] Remove duplicate slashes in filenames #26763
Conversation
|
original change was targetting 2.7, this branch is targetting master=4.1, please adjust target branch |
| @@ -540,9 +540,9 @@ public function in($dirs) | |||
|
|
|||
| foreach ((array) $dirs as $dir) { | |||
| if (is_dir($dir)) { | |||
| $resolvedDirs[] = $dir; | |||
| $resolvedDirs[] = rtrim($dir, '/\\'); | |||
There was a problem hiding this comment.
should be rtrim($dir, '/'.\DIRECTORY_SEPARATOR); (same below)
There was a problem hiding this comment.
\DIRECTORY_SEPARATOR can be '/' or '\'
'/'.\DIRECTORY_SEPARATOR would be literal // on *nix systems.
I can change that of course, just asking myself what the benefit would be
There was a problem hiding this comment.
the benefit would be that on *nix, \ should not be trimmed
|
please add missing test that were not present in original PR, that caused integrated project to fail. (eg one with |
Does this mean, the PR should be based on 2.7 branch? |
correct |
c65b064 to
a7007e9
Compare
|
Working on tests for symlinked dirs now |
|
Finder tests should pass now. Let me know if I can do anything else |
| @@ -26,55 +26,80 @@ public function testDirectories() | |||
| { | |||
| $finder = $this->buildFinder(); | |||
| $this->assertSame($finder, $finder->directories()); | |||
| $this->assertIterator($this->toAbsolute(array('foo', 'toto')), $finder->in(self::$tmpDir)->getIterator()); | |||
| $this->assertIterator(self::toAbsolute(array('foo', 'toto')), $finder->in(self::$tmpDir)->getIterator()); | |||
There was a problem hiding this comment.
please revert all these changes, they are unrelated to the PR and make reviewing harder than strictly required
There was a problem hiding this comment.
yeah, sorry. Should be fixed, but of course is unrelated to this change
|
This comment might give more explanation why I changed the code like that. |
|
Thank you @helhum. |
This PR was squashed before being merged into the 2.7 branch (closes #26763). Discussion ---------- [Finder] Remove duplicate slashes in filenames | Q | A | ------------- | --- | Branch? | 2.7 up to 4.0 | Bug fix? | yes | New feature? | no | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26757 | License | MIT This PR takes another approach to fix in excess slashes in Finder than #26337 which does a simple rtrim instead of the breaking realpath. Commits ------- cdde6d9 [Finder] Remove duplicate slashes in filenames
|
Are there changes needed in the testsuite? |
|
@DanielRuf What do you mean? I changed and added tests within this PR. Anything else you think needs to be done? |
|
see #26785 |
I still see some failing tests at Travis |
Great, exactly what I've meant =) |
|
Thanks! This (and the bump to |
…(DerDu) This PR was squashed before being merged into the 3.4 branch (closes #28604). Discussion ---------- [Finder] fixed root directory access for ftp/sftp wrapper | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27423 | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> This fixes a flaw introduced in 3.4.7 by #26763 In order to access the root folder with ftp wrapper, there MUST BE a slash present. - Currently: from 3.4.7 on it just ```rtrim``` all seperators (```/```, ```\```) from directories - Now: IF the directory is a (s)ftp:// wrapper (```#^s?ftp://#```) this fix just adds a slash (```/```) again Commits ------- 9630a38 [Finder] fixed root directory access for ftp/sftp wrapper
This PR takes another approach to fix in excess slashes in Finder than #26337
which does a simple rtrim instead of the breaking realpath.