Better feedback with bad arguments or options

These are good ideas. I have also considered that drush might do additional argument checking using techniques similar to those shown in #658420: helper functions for drush_backend_invoke().

1. right now, we don't know if a given option needs a value
2. right now, we have no dictionary of valid options, or for which commands those options apply. i suppose the universe of global options is known but we actually want to open that up to expansion by command files. and today, commands can take action in the pre/post drush_invoke phases based on an option/
3. seems plausible today

in general, we are not particularly close to achieving this. any thoughts on how to proceed?

I think we have a good handle today on the universe of valid options. We rely on this for bash completion.

Perhaps would be good to expand option definition to know when one requires a value or not. Also would be good to expand arg definition so we can mark some as required (and perform validation).

Anyway, would be great if someone would take this on.

Closed duplicate issue #996092: Better handling of unknown option.

This patch will check to see if any of the provided cli options are missing from the list of options provided in the command record. Suboptions, global options, and option short-forms are all tested. If a command will pass along arbitrary options to some other command (e.g. drush core-rsync), then it can set the 'allow-additional-options' flag to turn of option error checking.

There is another feature in here I suspect we might not need. If 'allow-additional-options' is a list of command names, then the options from those commands are also accepted as valid. For example, 'pm-update' could list 'pm-updatecode' and 'updatedb'. However, in practice, commands that do this already duplicate the allowed options in their command record, so perhaps this can be taken out.

This is only lightly tested; please review for design and concept. If this implementation is okay in concept, then a review still will need to be made to insure that every option that is tested via drush_get_option really is described in the command help record somewhere, because now it will be impossible to use these options if they are not.

Oh, also required options are not supported yet, but that won't be hard to add.

Code looks really good.

1. Lets do a simple count() on the unknown options so that we use a singular in an error like 'Unknown options: zzz'
2. Do backend calls send any undocumented options? I don't think so, but worth asking.
3. Drush usually uses the term 'verify' instead of 'check' so perhaps rename _drush_check_cli_options().

We should document the importance of declaring options at sandwich file and in docs-commands topic. Also document the 'allow-additional-options' key. I like how that key can take an array of command names.

Lets commit this and then add any undocumented options that folks are using. We might want to provide a global kill switch for this test if we think that we'll never get all the options documented. I think this can go in now, or after required arguments are added.

Committed work-in-progress: http://drupalcode.org/project/drush.git/commit/2e7c943

1. & 3.: Done.

2. It is not feasible to test for options on backend invoke calls. It is too common for backend invoke to get merged options and pass them on to the next drush command, so the options list might be "dirty" and contain items that will just be ignored. The current code is carefully positioned to catch typos on the cli without interfering with scripted functions.

user-password was the only command I could find that had a required option. I have not implemented 'required' yet; existing code needs some work to allow command option values to be arrays, as the global options already can be. Will fix that later when I have more time.

I changed the code to only print a warning on unknown options, then committed it. We can switch it back to an error once the warnings are cleared up. I fixed some of them, but there are a few more I didn't have time to do yet:

drush ssh: 'alias-path'
pm-*: 'cache', 'skip'
user-cancel: 'delete-content'

Still need to document feature per #6.

Added documentation to docs-commands and sandwich example. Enhanced the functionality of the help command to include the options from subcommands declared in the 'allow-additional-options' item in the help output.

Commit: http://drupalcode.org/project/drush.git/commit/c217226

Note: The commit comment was supposed to be:

#663290 by greg.1.anderson: document allow-additional-options, and also merge in options from subcommands in the output of `drush help`.

Unfortunately, I carelessly allowed the `drush help` to execute, so my commit comment is now somewhat longer than intended. :p I'm a bit shy about running git ammend on master. Help?

This issue still needs work insofar as there are still options that need to be included in command definitions.

policy.drush.inc recommends using if (!drush_get_option('token') == 'secret') to control a policy. Any command hook that wants to add options must now also be provided a way to specify what those options are. I guess that drush_get_global_options should call some new hook alter to allow commandfiles with command hooks to add new global options.

Added a slew of options to various command structures.

Some options, such as --complete-debug, should be verified but not displayed in help. I added a 'hidden' attribute to indicate this. I made a bunch of options hidden; maybe many of these should be made visible.

I added an --invoke flag to indicate that drush is being called from a script, and the options should not be tested as a result. I was wrong above -- backend invoke does test for valid options, and this was causing problems. I changed the validation routine to skip option checks when --backend or --invoke are specified. I also used --invoke a number of times in the unit tests to avoid warnings in cases where one big options array is passed to multiple drush commands (where some options are not relevant to every command they are used with).

Support for 'required' options also added. user-password is the only command I've found so far with a required option.

Support added to drush_hook_help_alter to allow global options to be hooked in. Added documentation in policy.drush.inc and drush.api.php.

There is still one warning about an unknown option in the unit tests. The option in question is documented, but the warning still comes up due to a bootstrapping flaw. I will therefore address this in a separate issue.

Missing options are still reported as warnings, not errors, but I think we should run like this for a little while before switching it over. For now, there is a --strict option that you may specify that will force drush to return an error on unknown options. The unit tests use this feature when testing unknown options.

Committed to master; marking as 'fixed'. Re-open with any issues.

Nice work Greg.

In this issue I made a mostly unrelated feature request which is for *arguments* to be able to be marked as required and for drush core to verify that. Then we can remove a bunch of similar handling in the commands

Here's a required args patch. This is what it says:

$ drush ev
Missing required argument.  See `drush help php-eval` for information on usage.   [error]

I did not expand the arg definition, as was done with options; instead, I added another 'required-arguments' field that is either TRUE (require all arguments) or an integer (the number of arguments that are required). Seemed simpler and sufficient.

Looks good. OK to commit after the following are done ...

1. Would be especially nice if the error msg stated the argument names that are missing, but we can do that in a follow-up.
2. Lets document this in sandwich command and the topic about command authoring.
3. Add new key to drush_command_defaults()
4. Which commandfiles have not yet been reviewed for required args? Also, did you see any opportunity to remove validation in command callbacks?

I applied greg.1.anderson's required-args.patch to the tip of master (currently at f133d9ebee8e6e). Works for me.

Here's a patch to get rid of a little bit of extraneous validation. This is handled by required-arguments now.

Committed #12 and #14, along with 1., 2. and 3. from #13. 4. from #13 is done in #14. Note, however, that I did not add any docs to the sandwich command, because the new options are not used there. They are documented in the appropriate topic command, though.

Marking as "fixed", although at some point in the future we should change a missing option from a warning to an error.

We need to get the engines into the available option list. I'm seeing Unknown option: --package-handler

The engine options were there -- they were just in the form 'package-handler=git_drupalorg'. Doh. Stripped off the later part when flattening the options in the verification function.

Add "Needs change notification".

remove tag