Skip to content
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

Fixes #915: Sync drushrc.php with template drushrc.php via update hook. #1265

Merged
merged 2 commits into from
Mar 28, 2017

Conversation

malikkotob
Copy link
Contributor

No description provided.

@grasmash grasmash added in progress Enhancement A feature or feature request labels Mar 28, 2017

if ($this->getFileSystem()->exists($sourcePath)) {
$this->getFileSystem()->copy($sourcePath, $targetPath, $overwrite);
if ($overwrite) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the file was copied but not overwritten, won't $fileCopied be FALSE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grasmash that's true. Good catch. Looking back at the fileSystem copy method, I realized we actually don't have a way of knowing if the file actually copied if $overwrite is FALSE without us manually comparing the last modified date on the target file and the source file. I can either modify the logic to check that information (which feels a bit excessive) or I can simply not return anything - which is what the fileSystem copy method is currently doing (doesn't feel great either). Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not return anything. An exception should be throw if it fails

@malikkotob malikkotob changed the title Connects to #915: Sync drushrc.php with template drushrc.php via update hook. Fixes #915: Sync drushrc.php with template drushrc.php via update hook. Mar 28, 2017
@grasmash grasmash merged commit 9719bf3 into acquia:8.x Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants