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

Make pm:install and pm:uninstall respect --simulate option #5152

Merged
merged 3 commits into from Jun 23, 2022

Conversation

Kingdutch
Copy link
Contributor

In Drush 8 it was possible to pass --no to pm:uninstall to let it output only what modules would be uninstalled without actually uninstalling any modules. This was removed in Drush 9 after discussion in #205 (PR #1057 was declined as a result).

However, we had some automation that used this to see what would be uninstalled so that this could be reported for user verification, even in the case only a single module would be uninstalled. This is now broken and that command uninstalls the module when it's the only one being uninstalled.

To preserve the new desired behaviour but still easily allow automation to determine what changes would be made by running pm:install or pm:uninstall this PR adds a --dry-run option that will not actually perform any of the changes.

An alternative implementation would be to provide entirely separate commands, but that feels like overkill.

@weitzman
Copy link
Member

weitzman commented May 23, 2022

Seems a good proposal to me.

  1. Lets do our printing via $this->logger->debug(). We dont usually use writeln()
  2. Drush already has a --simulate global option, so lets check for that via if (\Drush\Drush::simulate())
  3. Lets add a usage example for --simulate

@Kingdutch
Copy link
Contributor Author

@weitzman

Lets do our printing via $this->logger->debug(). We dont usually use writeln()

I mostly copied the existing code here. Do you want to change the output of the command itself to use debug() instead of writeln too? I'd like to keep the output between --simulate and the actual execution the same, otherwise automation may still be difficult/unpredictable.

@Kingdutch
Copy link
Contributor Author

I've made changes 2 and 3 but have omitted 1. Changing to logger()->debug() would make the output hidden unless the --debug flag is passed as well, which would cause all sorts of output that's not useful to users who want to preview the results of their actions, it would also make automation a lot more difficult as unneeded output would have to be filtered out. I also don't see any other log levels that would make sense over normal output to stdout.

@Kingdutch Kingdutch changed the title Add --dry-run option to pm:install and pm:uninstall Make pm:install and pm:uninstall respect --simulate option May 24, 2022
@weitzman weitzman merged commit 37de676 into drush-ops:11.x Jun 23, 2022
@weitzman
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants