acquia / blt

Acquia's toolset for automating Drupal 8 and 9 development, testing, and deployment.
https://docs.acquia.com/blt/
GNU General Public License v2.0
442 stars 394 forks source link

consolidation/annotated-command update changes annotation styles #3501

Closed pixlkat closed 5 years ago

pixlkat commented 5 years ago

This is not a BLT issue per se, but in case anyone else has some custom drush commands that begin behaving unexpectedly with regards to default option values, this is probably the issue.

I recently updated a project using composer update to pull in all the BLT/Drupal core/lightning updates. We have drush pinned to 9.4.0 in our root composer.json.

After that I noticed that some custom drush commands were failing with a message like invalid option iterations: 1).. The options were specified in the drush command method $options array with the expected default values where values are required/expected.

Our annotation for the command for the iterations option above was

   * @option iterations
   *   How many times to send data (Default: 1).

This PR in consolidation/annotated-command allows annotated args and options to specify their default values in their descriptions. The regex grabs everything which follows the keyword Default: to the end of the line.

I removed the punctuation surrounding the default value in our command annotation and the commands got the expected default value.

   * @option iterations
   *   How many times to send data Default: 1

While I understand the need for the regex to be as greedy as it is, I am not convinced that the annotation should take precedence over the default values specified in the code as part of the method declaration.

mikemadison13 commented 5 years ago

@pixlkat Can you cover any of the details you used to solve this issue? Then I think we can close it out as a documentation ticket.

Thanks for posting!

pixlkat commented 5 years ago

@mikemadison13 - I discovered the error when our automated tests failed in pipelines. The only place in code that I could see the option value matching the error output was in the annotation documentation. Running the drush command locally on the command line also failed; if I provided a value for the option in the error on the command line, that value was used, and the next option which specified a default value in an option annotation that contained punctuation failed.

Note that if you aren't performing some kind of allowed values checks in your command code you won't see the error explicitly but may experience other mysterious failures.

Since the only thing that had changed in that code was fixing a phpcs validation error because of how the $option array was being declared (and removing that change had no effect) I assumed something else had changed.

I checked composer.lock and noticed that the consolidation/annotated-command package had been updated from 2.11.2 to 2.12.0. Running composer why consolidation/annotated-command reports it is required both by Robo and drush.

Thanks to @wu-edward for supplying me with this diff - it was obvious from this that the code now sets the default option value obtained by running preg_match('#(.*)(Default: *)(.*)#', trim($description), $matches) on the description.

TL;DR: If you are documenting the possible command options via annotations in a custom drush command, make sure you are treating the default value as an actual value that will be used by the command.

i.e.

* @option foo
*   Description of foo Default: bar
mikemadison13 commented 5 years ago

Thanks for the info!