Update Druplicon hook and ExampleCommandFile to new hook APIs #2370
Conversation
| { | ||
| if ($args['options']['french']) { | ||
| if ($commandData->input()->getOption('french')) { |
There was a problem hiding this comment.
The naming here feels a little off. My impression from the name CommandData is that it refers to a command definition. If I wanted to learn info about the current request I would go to Request->getOptions() or similar. I could see Request->getCommandInfo() as a way to fetch the current command definition.
Having said that, its a minor point, and you gave already released annotated-command so no need to act on this.
There was a problem hiding this comment.
CommandData is only used in the hook system; there are no other clients of that, so I could bend the semver and change its name in 2.1.0 and it wouldn't hurt anything.
CommandData includes information about the command for a hook. The information it includes is the Input object (which is where getOption() comes from -- Symfony's InputInterface::getOption()), the Output object, and the annotation data, which includes the command name, but does not include the entire command record.
There was a problem hiding this comment.
OK thanks. There may be no need to change the name then ... Do you think that --druplicon should write to Output object instead of drush_print(). --druplicon is a silly edge case so maybe we just dont worry about it.
| return; | ||
| } | ||
| drush_log(dt('Displaying Druplicon for "!command" command.', array('!command' => $commandName))); | ||
| if ($commandData->input()->getOption('druplicon')) { |
There was a problem hiding this comment.
I guess this is the new drush_get_option(). Excited to see how command configuration gets wired into this system.
There was a problem hiding this comment.
Robo already has a Config object that can be passed in to a command handler via dependency injection. I intend to make a system similar to Drush's current procedural system that will just set the option values directly on the $input object, so a command object would only need to have the Config object injected if it needed access to more than just its command args and options.
|
Oh, changes approved -- guess I'll merge. Still getting used to the new GitHub system too. |
Making the
masterbranch like #2366