[Filesystem] mirror - fix copying content with same name as source/target. #23473
Conversation
|
I would split the test case into two (one with the |
| $this->assertFileExists($targetPath.'source'); | ||
| $this->assertFileExists($targetPath.'target'); | ||
|
|
||
| chdir($oldPath); |
| $this->assertFileExists($targetPath.'source'); | ||
| $this->assertFileNotExists($targetPath.'target'); | ||
|
|
||
| chdir($oldPath); |
| { | ||
| $sourcePath = $this->workspace.DIRECTORY_SEPARATOR.'source'.DIRECTORY_SEPARATOR; | ||
|
|
||
| $oldPath = getcwd(); |
There was a problem hiding this comment.
Could you be less cryptic please? What isn't needed and why? Ditto for other comments.
There was a problem hiding this comment.
We do not need the chdir() call after the assertions at the end of both methods. Therefore, we can also removed the $oldPath variable.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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?
| { | ||
| $sourcePath = $this->workspace.DIRECTORY_SEPARATOR.'source'.DIRECTORY_SEPARATOR; | ||
|
|
||
| $oldPath = getcwd(); |
|
ping @symfony/deciders |
There was a problem hiding this comment.
@gitlost thanks for fixing this bug ... and congrats on your first Symfony contribution!
|
Thank you @gitlost. |
…s source/target. (gitlost) This PR was squashed before being merged into the 2.7 branch (closes #23473). Discussion ---------- [Filesystem] mirror - fix copying content with same name as source/target. | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23472 | License | MIT | Doc PR | Uses `substr()` and lengths in `Filesystem::mirror()` rather than `str_replace()` to avoid multiple replacements. Commits ------- b524c84 [Filesystem] mirror - fix copying content with same name as source/target.
Uses
substr()and lengths inFilesystem::mirror()rather thanstr_replace()to avoid multiple replacements.