8000 [Filesystem] mirror - fix copying content with same name as source/target. by gitlost · Pull Request #23473 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Filesystem] mirror - fix copying content with same name as source/target. #23473

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.

Sign up for GitHub

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 4 commits into from
Closed

[Filesystem] mirror - fix copying content with same name as source/target. #23473

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
65aadc4
[Filesystem] mirror - fix copying content with same name as source/ta…
gitlost Jul 11, 2017
444cf22
Replace tabs with spaces.
gitlost Jul 11, 2017
a12161f
Split test into 2 cases.
gitlost Aug 13, 2017
ff39f61
Move getcwd/chdir to before/after mirror call.
gitlost Aug 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Filesystem] mirror - fix copying content with same name as source/ta… 
…rget.
  • Loading branch information
@gitlost
gitlost committed Jul 11, 2017
commit 65aadc4200cbd9396879e70b1401b20fcec43ea5
6 changes: 4 additions & 2 deletions src/Symfony/Component/Filesystem/Filesystem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,7 @@ public function mirror($originDir, $targetDir, \Traversable $iterator = null, $o
{
$targetDir = rtrim($targetDir, '/\\');
$originDir = rtrim($originDir, '/\\');
$originDirLen = strlen($originDir);

// Iterate in destination folder to remove obsolete entries
if ($this->exists($targetDir) && isset($options['delete']) && $options['delete']) {
Expand All @@ -452,8 +453,9 @@ public function mirror($originDir, $targetDir, \Traversable $iterator = null, $o
$flags = \FilesystemIterator::SKIP_DOTS;
$deleteIterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($targetDir, $flags), \RecursiveIteratorIterator::CHILD_FIRST);
}
$targetDirLen = strlen($targetDir);
foreach ($deleteIterator as $file) {
$origin = str_replace($targetDir, $originDir, $file->getPathname());
$origin = $originDir.substr($file->getPathname(), $targetDirLen);
if (!$this->exists($origin)) {
$this->remove($file);
}
Expand All @@ -475,7 +477,7 @@ public function mirror($originDir, $targetDir, \Traversable $iterator = null, $o
}

foreach ($iterator as $file) {
$target = str_replace($originDir, $targetDir, $file->getPathname());
$target = $targetDir.substr($file->getPathname(), $originDirLen);

if ($copyOnWindows) {
if (is_file($file)) {
Expand Down
30 changes: 30 additions & 0 deletions src/Symfony/Component/Filesystem/Tests/FilesystemTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,36 @@ public function testMirrorCopiesRelativeLinkedContents()
$this->assertEquals('\\' === DIRECTORY_SEPARATOR ? realpath($sourcePath.'\nested') : 'nested', readlink($targetPath.DIRECTORY_SEPARATOR.'link1'));
}

public function testMirrorContentsWithSameNameAsSourceOrTarget()
{
$sourcePath = $this->workspace.DIRECTORY_SEPARATOR.'source'.DIRECTORY_SEPARATOR;

$oldPath = getcwd();
Copy link
Member

Choose a reason for hiding this comment

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

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you be less cryptic please? What isn't needed and why? Ditto for other comments.

Copy link
Member

Choose a reason for hiding this comment

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

We do not need the chdir() call after the assertions at the end of both methods. Therefore, we can also removed the $oldPath variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give a reason please? The save/restore chdir()s are following the same pattern as the existing tests testFilesExistsFails() and testMirrorCopiesRelativeLinkedContents() and it seems good practice to leave things as you find them.

Copy link
Member

Choose a reason for hiding this comment

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

Doing clean up work after performing assertions is not reliable. As soon as one of the assertions fails clean up work will not happen anymore. So what we could is remember the current directory inside the base class' setUp() method and restore it in tearDown().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing clean up work after performing assertions is not reliable.

True, I've moved them to directly before and after the call to mirror().

So what we could is...

Another PR perhaps?

chdir($this->workspace);

mkdir($sourcePath);
touch($sourcePath.'source');
touch($sourcePath.'target');

$targetPath = $this->workspace.DIRECTORY_SEPARATOR.'target'.DIRECTORY_SEPARATOR;

$this->filesystem->mirror('source', $targetPath);

$this->assertTrue(is_dir($targetPath));
$this->assertFileExists($targetPath.'source');
$this->assertFileExists($targetPath.'target');

unlink($sourcePath.'target');

$this->filesystem->mirror('source', 'target', null, array('delete' => true));

$this->assertTrue(is_dir($targetPath));
$this->assertFileExists($targetPath.'source');
$this->assertFileNotExists($targetPath.'target');

chdir($oldPath);
Copy link
Member

Choose a reason for hiding this comment

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

same here

}

/**
* @dataProvider providePathsForIsAbsolutePath
*/
Expand Down
0