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

custom commands are no longer getting picked up #3610

Closed
joachim-n opened this issue Jul 9, 2018 · 24 comments
Closed

custom commands are no longer getting picked up #3610

joachim-n opened this issue Jul 9, 2018 · 24 comments

Comments

@joachim-n
Copy link
Contributor

I've just updated to Drush 9.3.0.

My custom command is no longer getting picked up.

It's available on Packagist as drupal-code-builder/drupal-code-builder-drush.

Composer installs the package at: web/drush/contrib/drupal-code-builder-drush

In that, there is a class:

namespace Drush\Commands\CodeBuilder;

use Consolidation\AnnotatedCommand\AnnotationData;
use Consolidation\AnnotatedCommand\CommandData;
use Consolidation\AnnotatedCommand\CommandError;
use Drush\Commands\DrushCommands;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

/**
 * Provides commands for generating code with the Drupal Code Builder library.
 */
class CodeBuilderCommands extends DrushCommands {

Drush list doens't pick this up.

@weitzman
Copy link
Member

weitzman commented Jul 9, 2018

See bottom of the release notes - https://github.com/drush-ops/drush/releases/tag/9.3.0. We made a change in order to have code match the docs.

@greg-1-anderson
Copy link
Member

Sorry for the breaking change, but the published implementation was a bug. 😞

@joachim-n
Copy link
Contributor Author

Authors of Drush9 global commands (i.e. commands that don't live within a module) will want to make sure they use the namespace Drush/Commands and not the briefer Drush.

So does that mean it must be Drush\Commands\MyClass? It can't have an extra namespace like mine does?

I have pointed out before how this is a flaw, as it means that all custom commands have to share the same namespace for their classes, and thus have to resort to pseudonamespacing their short classnames with a package prefix to avoid potential clashes!

@greg-1-anderson
Copy link
Member

I remember that old discussion, but I don't think it ever reached any conclusion.

If your source tree looks like this:

src/
└── Commands
        └── Foo
            ├── MyClass.php
            └── PolicyCommands.php

Then in this instance your namespace would be Drush\Commands\Foo. This is not ideal.

As I mentioned previously, Robo has a Composer-based solution for this that allows for flexible namespaces, but Drush has not adopted this mechanism. We probably don't want lots of different ways to create global extensions, as these are less common.

I agree that the current solution is not great, but I'm not sure what the best way to allow dynamic namespaces should be. Suggestions and PRs welcome.

@joachim-n
Copy link
Contributor Author

joachim-n commented Jul 10, 2018

Then in this instance your namespace would be Drush\Commands\Foo. This is not ideal.

Is that permissible for the command to be picked up? Because I think that would be OK. That way, my command class can be Drush\Commands\MyPackage\MyClass. The subnamespace MyPackage keeps my own classes separate from any other package.

(If that is the case, then the docs & release notes could do to be updated, as this makes it looks like it MUST be Drush\Commands\MyClass: "will want to make sure they use the namespace Drush/Commands and not the briefer Drush.")

However, that is the situation I have, and it's not getting picked up:

namespace Drush\Commands\CodeBuilder;
[SNIP]
class CodeBuilderCommands extends DrushCommands {

Does the source tree's hierarchy of folders matter for the class detection system?

Because while I have that namespace structure, I do NOT have that file structure. The top level of my package has these:

CodeBuilderCommands.php
README.md
composer.json

I've used the PSR4 section in composer.json to allow the class to be in the root of the package, even though it's in a namespace:

    "psr-4": {
      "Drush\\Commands\\CodeBuilder\\": ""
    }

This is because the whole package is pretty much just that class. So having it nested in some folders is a bit pointless.

Could that be the problem with the command not getting picked up?

@greg-1-anderson
Copy link
Member

Don't have time to test / research, so there may be slight errors in my answer below. However, I think you'll be fine with:

"psr-4": {
      "Drush\\": "src"
    }

Path:

src/Commands/CodeBuilder/CodeBuilderCommands.php

Or some variation thereof. Take the example from the docs and add CodeBuilder to the directory path and namespace, but leave the psr-4 the same.

@joachim-n
Copy link
Contributor Author

Thanks for the quick feedback!

That doesn't look too far from what I've got, but I've run into problems tweaking it because Composer's autoload doesn't seem to pick up my on-the-fly changes to the code in my package... so I'm going to need to set aside some time to faff with creating a separate branch to then get Composer to install it...

(BTW, I'm guessing you maybe have this sort of workflow when working on the various libraries that Drush uses ... any tips you could share on https://stackoverflow.com/questions/51266513/how-do-i-work-on-a-library-thats-included-with-composer ?)

@weitzman
Copy link
Member

See composer’s path repository. https://getcomposer.org/doc/05-repositories.md#path

@greg-1-anderson
Copy link
Member

I commented on your StackOverflow question. The problem is that Drush does not support global plugins that have dependencies. Drupal modules can have dependencies, and Drush extensions attached to a module can use the dependencies of the module it is attached to. That's as far as we took it. Global Drush extensions are primarily intended for implementing policy files, which do not need external dependencies. Implementing a code generator as a Drush 9 plugin is a more demanding use case than our plugin mechanism currently supports, and a bit outside the bounds of the use cases we anticipated on supporting with our plugin mechanisms.

Potential paths forward:

  1. Implement your own standalone phar and exec Drush to get the info you need about Drupal.
  2. Contribute to the code generator that is already part of Drush core
  3. Contribute a PR to improve the plugin mechanism for Drush.

Regarding #1, see Robo as a framework and g1a/starter. I am in the process of factoring out backend invoke from Drush, which would make it easier to process the results from your calls out to Drush.

Regarding #2, I know that lots of folks probably would prefer that Drush had chosen one of the other available code generators that are out there. I can only offer regrets to those who don't agree with the decision that was made.

Regarding #3, this should be discussed before starting any serious work in this direction. We do not want to support too many different plugin mechanisms, and those that we do support should support our common use cases.

@joachim-n
Copy link
Contributor Author

See composer’s path repository. https://getcomposer.org/doc/05-repositories.md#path

That sounds like what I'm trying to do! Not quite the same as what I asked on SO, but the net result is the same: I can hack away on my own git repo of my package, and the files I am working on can be seen and used by Composer as it manages the main app.

Will give it a go. Thanks!

The problem is that Drush does not support global plugins that have dependencies.

So are you saying that Drush global plugins can't, or should not be, installed with Composer? I'm reeling a bit from this! I've been managing to run the Drupal Code Builder Drush plugin with Drush 9, installed with Composer, until a recent update meant my Drush plugin was no longer being detected.

If the problem is just dependencies, then I don't mind adding a note to the README for Drupal Code Builder Drush to say that users should also do "composer require drupal-code-builder/drupal-code-builder". That's pretty simple.

Or is there a problem with autoloading?

I'll have another poke at Composer and see why my class wasn't even spotted by the autoloader when I next have enough time to properly get into it.

As for your suggested solutions:

  1. Implement your own standalone phar and exec Drush to get the info you need about Drupal.

The point of my Drush plugin is that it's a Drush command. I don't really want to require users to have to have a separate executable to run for the code builder.

  1. Contribute to the code generator that is already part of Drush core

I think that's a non-starter. Both of the other Drupal code builders are massively limited, in that they don't do any code analysis. They only work with Core, and the components they support are all hardcoded (e.g., Drush's code generator has verbatim copies of all the hook data which is in Drupal core... https://github.com/Chi-teck/drupal-code-generator/tree/master/templates/d8/hook ... which is really bad for maintainability!).

  1. Contribute a PR to improve the plugin mechanism for Drush.

If I had much more free time then I'd love to. But I have two small kids and my coding time is in small amounts of time. I never get more than an hour or two at a time. That makes it really hard to dive into a whole new codebase. Over the last year or so I've several times had to get into Drush / Robo / Annotated Command to figure out why things aren't working with my plugin, and as powerful as all of your code is, I'm sorry to say I find it really hard to understand with that many levels of abstraction. Probably if I wasn't suffering from chronic sleep deprivation I'd find it easier.

I know that lots of folks probably would prefer that Drush had chosen one of the other available code generators that are out there. I can only offer regrets to those who don't agree with the decision that was made.

Appreciated.

Moshe did get in touch with me a few years ago about integrating DCB, but at the time he ended up concluding the code was too much of a mess. Arguably, it was back then, but even so, I think it was more maintainable than the alternatives. I tried to convince him and offered guarantees of clean-up work and refactoring (and lots of clean-up has been done since then), but he stopped replying to my emails.

Still, DCB has a stable and documented API, semver releases, and there's even code for a Drush command that would work if it were part of Drush rather than a plugin, so if Drush decides to switch to a better code generator in the future, you know where I am :)

@greg-1-anderson
Copy link
Member

If the problem is just dependencies, then I don't mind adding a note to the README for Drupal Code Builder Drush to say that users should also do "composer require drupal-code-builder/drupal-code-builder". That's pretty simple.

If you're always installing as a site-local project in the Drupal site, then you're dependencies will all be taken care of. I'm not sure that Drush will pick up your commands in this instance. This is the angle we should pursue. If it's already working (once the namespace issues are taken care of), then we're done. Once your composer.json is being considered part of Drupal's included code, then all of the available options (e.g. the composer path repository feature Moshe mentioned) should work fine.

If Drush still isn't picking up your commands, then maybe we just need to hop on a hangout sometime and figure out where the hangup is.

@joachim-n
Copy link
Contributor Author

I've got the autoloading working. A short custom script in the repo root shows it loads:

<?php

require __DIR__ . '/vendor/autoload.php';

$c = new \Drush\Commands\CodeBuilderCommands;
$c = new \Drush\Commands\CodeBuilder\CodeBuilderCommands;

As you can see from that, I've tried the command in two different places, and neither works.

The file structure is:

src/Commands/CodeBuilder/CodeBuilderCommands.php
src/Commands/CodeBuilderCommands.php

with this in the autoload:

    "psr-4": {
      "Drush\\Commands\\": "src/Commands"
    }

I've buckled up and gone digging in Annotated Command...

When I run 'drush list', Consolidation\AnnotatedCommand\CommandFileDiscovery::discover() is only called once, with this param:

array:1 [
  0 => "/Users/joachim/Sites/composer-d8/vendor/drush/drush/src"
]

BTW, the drupal-composer/drupal-project composer template is putting my Drush command in /drush/contrib/drupal-code-builder-drush which is outside of the webroot. There is also a /web/drush/contrib folder which has a README, policy.drush.inc, and contrib folder. Is that normal?

@joachim-n
Copy link
Contributor Author

Drush\Config\ConfigLocator::getSiteCommandFilePaths() is getting called with:

array:2 [
  0 => "/Users/joachim/Sites/composer-d8/web/drush"
  1 => "/Users/joachim/Sites/composer-d8/drush"
]

So it looks like my question in my previous comment about /drush vs /web/drush is answered -- it shouldn't matter.

But it's returning an empty array. So it looks like that's the crux of the problem.

@joachim-n
Copy link
Contributor Author

joachim-n commented Jul 11, 2018

Ok found it!

The Finder is looking in:

  0 => "/Users/joachim/Sites/composer-d8/web/drush"
  1 => "/Users/joachim/Sites/composer-d8/drush"

with this filter:

            ->path('#^src/Commands$|^Commands$#')

So that is going to find /Users/joachim/Sites/composer-d8/drush/Commands and /Users/joachim/Sites/composer-d8/drush/src/Commands but not /Users/joachim/Sites/composer-d8/drush/contrib/my-package/src/Commands, which is where Composer installs a Drush custom command!

Take out both the ^ in the regex and it picks up my command again!

PS. It also needs followLinks(), so the Composer development method suggested by @weitzman above works!

@joachim-n
Copy link
Contributor Author

#3615

@joachim-n
Copy link
Contributor Author

joachim-n commented Jul 11, 2018

Urgh, just to confuse things, I'm now on my other machine, and the Composer template I have here has this:

            "drush/Commands/{$name}": ["type:drupal-drush"]

That doesn't pick up my command either - it gets put in /drush/Commands/drupal-code-builder-drush. Not tested my PR with it though.

Which is the correct path for packages of type type:drupal-drush?

@greg-1-anderson
Copy link
Member

composer-installers has this:

https://github.com/composer/installers/blob/master/src/Composer/Installers/DrupalInstaller.php#L12

Where do the other variations come from?

@greg-1-anderson
Copy link
Member

drupal-composer/drupal-project 8.x has:

https://github.com/drupal-composer/drupal-project/blob/8.x/composer.json#L67

Where does the contrib variant come from?

@greg-1-anderson
Copy link
Member

I guess with drush/Commands we'd be expecting to find things like composer-d8/drush/Commands/my-package/src/Commands

@joachim-n
Copy link
Contributor Author

Where does the contrib variant come from?

My composer.json file says:

"name": "drupal-composer/drupal-project",

So it must be an older version of https://github.com/drupal-composer/drupal-project/blob/8.x/composer.json

Which now has:

"drush/Commands/{$name}": ["type:drupal-drush"]

But there are 3 different composer.jsons for setting up a Drupal site, documented at https://www.drupal.org/docs/develop/using-composer/using-composer-to-manage-drupal-site-dependencies.

And for drush commands they have:

  • "drush/Commands/{$name}":
  • not sure
  • drupal-composer/drupal-project: "drush/contrib/{$name}"

Furthermore, on https://www.drupal.org/docs/develop/using-composer/using-composer-to-manage-drupal-site-dependencies#installer-dirs it says:

        "drush/{$name}": ["type:drupal-drush"],

So that is THREE different installer paths for Drush commands! Which ones are correct?

@greg-1-anderson
Copy link
Member

Thanks for tracking all of that down. Let's support both drush/contrib and drush/Commands, and obsolete drush/{$name}. This will allow us to avoid deep-searching arbitrary directories for commands, and still pick up most of the variants that are in use. We could submit a PR to composer-installers, which is where folks who do not specify the path (e.g. "B") will get their orders from.

That's my opinion, anyway. @weitzman ?

@weitzman
Copy link
Member

sounds good to me.

@joachim-n
Copy link
Contributor Author

Updated the PR based on @greg-1-anderson's most recent comment.

@weitzman
Copy link
Member

weitzman commented Aug 2, 2018

OK, lets discuss further in #3615

@weitzman weitzman closed this as completed Aug 2, 2018
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

3 participants