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

Command files aren't getting searched deeply enough in DRUPAL/drush #2918

Closed
joachim-n opened this issue Aug 30, 2017 · 15 comments
Closed

Command files aren't getting searched deeply enough in DRUPAL/drush #2918

joachim-n opened this issue Aug 30, 2017 · 15 comments
Assignees

Comments

@joachim-n
Copy link
Contributor

Follow-on from #2911:

If admins setup their Composer project via drupal-project, and you classify Codebuilder as a drupal-drush type, then Codebuilder will be installed into the right place

That's putting the command file into the right place, but annotated-command isn't picking it up.

My project has been placed in: /Users/joachim/Sites/8-drupal/drush/contrib so I have this file with my commands in:

/Users/joachim/Sites/8-drupal/drush/contrib/drupal-code-builder-drush/CodeBuilderCommands.php

However, debug output from \Consolidation\AnnotatedCommand::discoverCommandFilesInLocation() shows that the discovery process doesn't actually go into the 'contrib' folder:

"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush, == 0, \Drupal"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/Command, <= 2, \Drupal\Command"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/CliTools, <= 2, \Drupal\CliTools"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/CommandFiles, <= 2, \Drupal\CommandFiles"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/src, == 0, \Drupal"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/src/Command, <= 2, \Drupal\Command"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/src/CliTools, <= 2, \Drupal\CliTools"
"== discoverCommandFilesInLocation: /Users/joachim/Sites/8-drupal/drush/src/CommandFiles, <= 2, \Drupal\CommandFiles"
@weitzman
Copy link
Member

Thats a Drush bug. We need to configure that location. Please submit a PR. Thanks @joachim-n

@joachim-n
Copy link
Contributor Author

I can have a go at fixing it, but I'm not sure how you want it fixing:

                $searchpath[] = $drupal_root . '/../drush';
                $searchpath[] = $drupal_root . '/drush';
                $searchpath[] = $drupal_root . '/sites/all/drush';
                $commandFiles = $discovery->discover($searchpath, '\Drupal');

Should that code be changed to instead look in drush/contrib/*, iterating over all the subfolders in each drush/contrib?

Also, how should namespaces be handled? Prior to moving my command project into drush/contrib, there was a problem in that annotated-command used the project folder as part of the expected namespace. That's going to fail in a lot of cases, as a lot of git projects aren't valid namespace components. It also seems a bit brittle to me.

@greg-1-anderson
Copy link
Member

We could consider switching to consolidation/robo#604.

Not sure if there is any need to consider backwards compatibility for Drush 8. There are a lot of small details that seem to have never gotten worked out; perhaps no one is using this mechanism at the moment.

@weitzman
Copy link
Member

That "plugin mechanism" looks good to me. In the case of Codebuilder, I think that all @joachim-n needs to do is add an entry to his autoload-ps4 section of composer.json (see docs at top of consolidation/robo#604.) and then composer dumpautoload. @joachim-n could you confirm that this makes your plugin functional?

If above works, we just need to figure out documentation and possibly testing.

@greg-1-anderson
Copy link
Member

That Robo PR has not been merged into Robo, and therefore is not available in Drush yet. But we could add it pretty quickly if it was desired.

@joachim-n
Copy link
Contributor Author

I've added PSR-4 to the composer.json at https://github.com/drupal-code-builder/drupal-code-builder-drush/tree/9.x, but it sounds like I'm not in a position to try this if there are changes needed to both Robo and Drush.

@joachim-n
Copy link
Contributor Author

Great to see this is in progress! :)

The code I was using when I hit this problem is at https://github.com/drupal-code-builder/drupal-code-builder-drush, in case it's useful as a sample of a non-module based command.

@greg-1-anderson
Copy link
Member

I moved this to "in progress" because I definitely want to get it in before rc1. I don't think that we'll get the new plugin mechanism in, but we'll make sure that global commandfiles in "contrib" directories are discovered.

I haven't started the code, though, so there's no PR yet.

@joachim-n
Copy link
Contributor Author

I appreciate the intent! :)

In case it's any help with your work, you should be able to use my command as a reference -- 'composer require drupal-code-builder/drupal-code-builder-drush'.

I've added hacks to that to allow it to be used as a module-based command (so people can use that Drush command without hacking their own copy of Drush with workarounds) but AFAIK those won't get in the way when in a non-module context -- the drush.services won't get picked up, and the hacky src/CodeBuilderCommands.php gets loaded by Drupal's module-based autoloader, and not that package's own Composer autoloader spec.

I'd offer to make you a temporary branch with those hacks ripped out, but then I have no idea how you'd use composer to install a different branch...

@greg-1-anderson
Copy link
Member

Composer can install any branch of a project by requesting version dev-BRANCHNAME.

If a commandfile needs a drush.services.yml file, then it should be part of a module, and should not be available as a global or __DRUPAL_ROOT__/drush-installed module.

@joachim-n
Copy link
Contributor Author

joachim-n commented Oct 6, 2017

If a commandfile needs a drush.services.yml file, then it should be part of a module, and should not be available as a global or DRUPAL_ROOT/drush-installed module.

I think you've misunderstood me. My command is meant to be global. It has no need of a drush.services.yml.

But at the moment due to this bug, it doesn't work at all as a non-module command. The only way I have got it to work locally so I can work on it is by hacking my code file locations as hardcoded strings into Drush's command discovery system in my vendor copy of Drush.

I would like people to use my command while I develop it, before this bug in Drush is fixed.

Therefore, as a hideous hack, I have added files to my command's repo that make it also function as a Drupal 8 module, with a module info file, and so declare the Drush command as a module-based command.

Once this bug is fixed, I will rip all that out.

Would you like me to make you a branch with those hacks removed, so it's a clean non-module command?

@greg-1-anderson
Copy link
Member

I see. Yes, that would be helpful.

@joachim-n
Copy link
Contributor Author

greg-1-anderson added a commit that referenced this issue Oct 10, 2017
…d __PROJECT_ROOT__/drush) where commandfiles may be found.
@greg-1-anderson
Copy link
Member

@joachim-n Please try #3052

@weitzman
Copy link
Member

Closing in favor of #3052

greg-1-anderson added a commit that referenced this issue Oct 16, 2017
…d __PROJECT_ROOT__/drush) where commandfiles may be found.
greg-1-anderson added a commit that referenced this issue Oct 16, 2017
* Fixes #3054: Only replace command aliases (e.g. 'en' + 'si') when they appear as the first non-option / non-alias parameter on the argv list.

* Fixes #2918: Add site-specific search paths (__DRUPAL_ROOT__/drush and __PROJECT_ROOT__/drush) where commandfiles may be found.

* Fix problem with bootstrapping w/ no site.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants