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

Drush 8 discover my new drush 9 command file as well. #3041

Closed
segi opened this issue Oct 9, 2017 · 16 comments
Closed

Drush 8 discover my new drush 9 command file as well. #3041

segi opened this issue Oct 9, 2017 · 16 comments

Comments

@segi
Copy link

segi commented Oct 9, 2017

I ported my drush command set to be drush 9 compatible, my module just only contains a drush command set so it isn't a real module. So every time if somebody wants to use it, he/she needs to run with drush -r {docroot} -i {docroot/module/mymodule}.
My new command file is working with drush 9, but I wanted to test my module with drush 8 as well.
The drush failed with PHP error: PHP Fatal error: Class 'Drush\Commands\DrushCommands' not found
because the annotationcommand_adapter_get_discovery() method finds my commands class but it's using class which isn't available in drush 8.

I tested with drush 8.1.14

@greg-1-anderson
Copy link
Member

greg-1-anderson commented Oct 9, 2017

TL;DNR UPDATE: Don't use services.yml, use drush.services.yml.

These issues need to be addressed.

  • Drush commands that do not need to be modules is being addressed already in Command files aren't getting searched deeply enough in DRUPAL/drush #2918. See that issue for technique (although it does not work in Drush yet).
  • We discussed having an extras section in drush.services.yml where Drush extensions could declare their compatibility. Empty would mean Drush 8 only. Extensions could declare the minimum version of Drush they support, with one entry for each major version supported.
  • We should backport DrushCommands to Drush 8 so that commandfiles may implement this if desired. (n.b. Drush 9 does not require you to implement this base class if you don't want to use the services it provides.)
  • We should almost certainly also provide ConfigAwareInterface
  • We should consider providing SiteAliasAwareInterface, although I think this is not necessary.

Drush has provided backwards-compatible execution of the old-style Drush commands from Drush 2 through Drush 8. We should plan for a similar long run for the new mechanism to run from Drush 8 through many future versions major version upgrades of Drush.

@weitzman
Copy link
Member

weitzman commented Oct 9, 2017

I cant reproduce this. Please post your full module so we can try to reproduce.

For example, I enabled devel module and its submodule devel_generate. Then I ran some Drush commands using the Drush8 phar. I am seeing that the new Drush9 commandfiles get loaded.

You mention annotationcommand_adapter_get_discovery(). That is only in play before full bootstrap. Commandfiles that belong to modules get loaded via a drush.services.yml file, and not via that filesystem discovery.

I think you you should try to package your commandfile not as a module but rather as a composer package. If that sounds hard, just commit it to /drush in your composer project or even sites/all/drush in your drupal directory.

@segi
Copy link
Author

segi commented Oct 9, 2017

Currently I'm trying to port the acsf module but now I don't have anything shareable, but I could reproduce the problem with SandwichCommands.php as well.
I just copied SandwichCommands.php under my src/Commands folder and I run:
php drush.phar -r /my_drupal_install/docroot/ --include=/my_drupal_install/docroot/modules/my_module/
PHP Fatal error: Class 'Drush\Commands\DrushCommands' not found in /my_drupal/docroot/modules/my_mdoule/Commands/SandwichCommands.php on line 19 PHP Stack trace: PHP 1. {main}() /home/my_home/dev/drush.phar:0 PHP 2. require() /home/my_home/dev/drush.phar:10 PHP 3. drush_startup() phar:///home/my_home/dev/drush.phar/drush:114 PHP 4. drush_run_main() phar:///home/my_home/dev/drush.phar/includes/startup.inc:365 PHP 5. drush_main() phar:///home/my_home/dev/drush.phar/includes/startup.inc:458 PHP 6. drush_preflight_command_dispatch() phar:///home/my_home/dev/drush.phar/includes/preflight.inc:64 PHP 7. drush_parse_command() phar:///home/my_home/dev/drush.phar/includes/preflight.inc:571 PHP 8. drush_get_commands() phar:///home/my_home/dev/drush.phar/includes/command.inc:1418 PHP 9. annotationcommand_adapter_commands() phar:///home/my_home/dev/drush.phar/includes/command.inc:1120 PHP 10. annotationcommand_adapter_get_commands() phar:///home/my_home/dev/drush.phar/includes/annotationcommand_adapter.inc:102 PHP 11. annotationcommand_adapter_create_commandfile_instance() phar:// /home/my_home/dev/drush.phar/includes/annotationcommand_adapter.inc:269 PHP 12. include_once() phar:///home/my_home/dev/drush.phar/includes/annotationcommand_adapter.inc:288 Drush command terminated abnormally due to an unrecoverable error. [error] Error: Class 'Drush\Commands\DrushCommands' not found in my_drupal/docroot/modules/my_module/Commands/SandwichCommands.php, line 19
The phar was a drush 8.1.14. The drupal docroot didn't contain an existing install just a 8.4 codebase.
I really hope based on this description you can reproduce the problem.

@weitzman
Copy link
Member

weitzman commented Oct 9, 2017

Please don't ever point --include at a module. Thats not needed or supported. Drush loads from enabled modules automatically.

@weitzman
Copy link
Member

weitzman commented Oct 9, 2017

For porting, see https://weitzman.github.io/blog/port-to-drush9.

@greg-1-anderson
Copy link
Member

The main use case we are concerned about right now is modules with drush.services.yml. If you have a ported module that is causing problems with Drush 8, please provide a repository URL for reproduction.

@greg-1-anderson
Copy link
Member

I figured this one out.

In Drush 9, you are supposed to put your command services in the file drush.services.yml. However, I have noticed that sometimes folks use services.yml instead of drush.services.yml. This is not recommended, but it works; Drush would have to go out of its way to prevent that from working.

Drush 8 never implemented drush.services.yml, but it will read command files out of services.yml.

I think we should either remove services.yml commandfile handling from Drush 8 -- something that I don't think anyone used -- or also implement drush.services.yml and ensure there is some way for commandfiles to express Drush version compatibility.

@weitzman
Copy link
Member

I think 'neither' is a viable option too. if very few people used the services.yml approach in Drupal 8, then there isn't much of a problem. Those people are blissfully using Drush8 and we would bother them with such a change.

I'd be up for more docs about how this is a bad idea or a deprecated idea.

@weitzman
Copy link
Member

Tentatively closing this.

@greg-1-anderson
Copy link
Member

We should put in additional safeguards at a minimum. There is potential for conflict between sitewide commands (DRUPAL_ROOT/drush). Folks can manage their own global locations tho.

@alisonjo315
Copy link

Is this true or false?
Drush 8 will continue to work (or "happen to work") with Drupal 8 for the foreseeable future.

I've definitely thought I knew the answer in both directions several times while reading various issue threads. Thanks!

@weitzman
Copy link
Member

The answer is more subtle than true/false question. See #2787 (comment)

@alisonjo315
Copy link

alisonjo315 commented Oct 18, 2017

(I can't tell whether it's better to reply here or there (#2787); I picked over there, b/c over there seemed like a broader thread about overall Drupal 8.4 + drush compatability -- but seriously was mostly a guess; if this is a better place for my reply, please please say so and I'll re-post here and delete/strikethrough my other one.)

@weitzman
Copy link
Member

I think we'll go with 'do nothing' for now.

@greg-1-anderson
Copy link
Member

We absolutely need a way for folks to be able to write a Drush-9-only commandfile and put it in DRUPAL_ROOT/drush & have it not crash Drush 8 (and visa-versa). The rest is optional.

@weitzman
Copy link
Member

Fixed by #3189

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

No branches or pull requests

4 participants