Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedJul 12, 2017
I would split the test case into two (one with the |
nicolas-grekas commentedJul 28, 2017
| $this->assertFileExists($targetPath.'source'); | ||
| $this->assertFileExists($targetPath.'target'); | ||
| chdir($oldPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this can be removed now
| $this->assertFileExists($targetPath.'source'); | ||
| $this->assertFileNotExists($targetPath.'target'); | ||
| chdir($oldPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same here
| { | ||
| $sourcePath =$this->workspace.DIRECTORY_SEPARATOR.'source'.DIRECTORY_SEPARATOR; | ||
| $oldPath =getcwd(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not needed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We do not need thechdir() 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.
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/restorechdir()s are following the same pattern as the existing teststestFilesExistsFails() andtestMirrorCopiesRelativeLinkedContents() and it seems good practice to leave things as you find them.
There was a problem hiding this comment.
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 intearDown().
There was a problem hiding this comment.
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 tomirror().
So what we could is...
Another PR perhaps?
| { | ||
| $sourcePath =$this->workspace.DIRECTORY_SEPARATOR.'source'.DIRECTORY_SEPARATOR; | ||
| $oldPath =getcwd(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not needed
xabbuh commentedSep 13, 2017
ping @symfony/deciders |
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@gitlost thanks for fixing this bug ... and congrats on your first Symfony contribution!
fabpot commentedSep 13, 2017
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.