Skip to content

[Filesystem] Add a failing test covering a bug in Filesystem::mirror() #26399

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

Closed
wants to merge 3 commits into from

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Mar 4, 2018

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets none
License MIT
Doc PR

When calling Filesystem::mirror() with a $iterator parameter (to customize the list of files/folders to mirror), and when using the delete option on top of that, things break.

This new test covers the problem (but do not fix it). I have been scratching my head at it for an hour but I'm really having trouble with the implementation of Filesystem::mirror().

I believe the bug was introduced following #23473, more specifically:

- $origin = str_replace($targetDir, $originDir, $file->getPathname());
+ $origin = $originDir.substr($file->getPathname(), $targetDirLen);

Here is my debug session right before the exception:

capture d ecran 2018-03-04 a 20 25 29

Here are the values of the variables:

$originDir = '/tmp/source';
// I used a weird name on purpose so that it's longer than `source` to trigger the bug
$targetDir = '/tmp/targettarget';

// Here is the computed value of the file we are trying to delete:
$origin = '/tmp/sourcece1';
// it should be `/tmp/source/source1` instead if I understand correctly

I'm out of ideas for tonight sorry! I don't know if PR with failing tests are useful at all? I won't mind if you close it.

mnapoli added 2 commits March 4, 2018 20:09
When calling `Filesystem::mirror()` with a `$iterator` parameter (to customize the list of files/folders to mirror), and when using the `delete` option on top of that, things break.

This new test covers the problem (but do not fix it).
$this->assertTrue(is_dir($targetPath));
$this->assertDirectoryExists($targetPath.'source1');
$this->assertDirectoryNotExists($targetPath.'source2');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test works fine, I only wrote it because the $iterator parameter is not covered at all by tests.

@@ -11,6 +11,8 @@

namespace Symfony\Component\Filesystem\Tests;

use Symfony\Component\Finder\Finder;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up using the finder in the Filesystem component tests, is that OK? Should I add it to require-dev of the component I guess?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoiding the finder dependency by using a native iterator would be best

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Mar 12, 2018
@nicolas-grekas
Copy link
Member

@mnapoli up to work on a fix in this PR also?

@mnapoli
Copy link
Contributor Author

mnapoli commented Mar 12, 2018

Unfortunately I'm completely swamped lately, I wish I could help more but I won't be able to (even though I'd really love a fix too ^^).

I'm fine if you'd rather close this for now.

@nicolas-grekas
Copy link
Member

Status: needs work

@nicolas-grekas
Copy link
Member

Closing as we don't keep failing tests unless an implementation is planned.
Please open an issue if you want to keep track of the topic, or better: submit a fix when you have some time.

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

Successfully merging this pull request may close these issues.

4 participants