-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow registering of command info alterer services #3447
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
Allow registering of command info alterer services #3447
Conversation
|
Woot woot! Lets see what @greg-1-anderson has to say. |
|
The code looks good and I am generally +1 on this. Could you be a little more specific about what you are doing with this? Are you just renaming / re-aliasing existing commands, as the tests are doing? |
| $serviceCommandInfoAltererlist = $container->get(DrushServiceModifier::DRUSH_COMMAND_INFO_ALTERER_SERVICES); | ||
| $commandFactory = Drush::commandFactory(); | ||
| foreach ($serviceCommandInfoAltererlist->getCommandList() as $altererHandler) { | ||
| $commandFactory->addCommandInfoAlterer($altererHandler); |
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 should probably write a debug log message if a module adds one of these.
|
My big concern here is that folks could get really confused if one module is altering commands in core or another module that is not related (in the same suite as) to the module doing the alteration. Could you write some documentation on how this feature could be used, and also strongly encourage folks to debug log when altering commands, so that this behavior is discoverable to anyone who does get confused. It won't be possible for us to enforce logging, of course, but we can at least encourage it. It would also be good if you could inject a logger into the woot command and debug log there, just so that the test code serves as a good example of how to alter commands. It isn't necessary to have an assert that ensures the log is printed, as long as it's there for illustrative purposes. |
|
@greg-1-anderson, we are preparing to add migrate commands to Drush core in #3402. But we want to coordinate with |
|
Perhaps some of these operations should happen automatically in annotated command. I haven't checked yet to see if this is possible / easy, but if other annotations in a |
|
@greg-1-anderson , I saw that array annotations, such as @option, @Usage are merged, not replaced. The others are kept from the original command. |
|
|
||
| class WootCommandInfoAlterer implements CommandInfoAltererInterface, LoggerAwareInterface | ||
| { | ||
| use LoggerAwareTrait; |
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.
You can drop the LoggerAwareInterface / LoggerAwareTrait here. Drush will only inject its logger into command classes that implement these interfaces -- not any object at all. Since you are instantiating this class via the Drupal service file (drush.services.yml), then Drupal will inject your logger factory, so the code you have below is sufficient.
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.
OK, removed the trait and interface.
greg-1-anderson
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.
👍
|
LGTM thanks for contributing this - should be very useful. |
We are able to alter/replace commands in modules but we cannot intercept and alter the command info (annotations). This PR adds such capability.
How it works?
\Consolidation\AnnotatedCommand\CommandInfoAltererInterface.drush.services.ymland should be tagged with thedrush.command_info_alterertag.alterCommandInfo()of the alterer class.See the testing
wootmodule for details.