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

Backport static create factory commands for Drush 11.x #5565

Merged
merged 7 commits into from
May 18, 2023

Conversation

greg-1-anderson
Copy link
Member

@greg-1-anderson greg-1-anderson commented May 14, 2023

This backports static create factory command handlers to Drush 11.x, supporting commands, hooks and info alterers for PSR-4 discovered commands and module commands. Does not include the port of Drush command implementations to use static create factory methods, or any of the modernized classes or refactoring from the 12.x version.

Having this basic capability in Drush 11.x will make module maintainers more likely to adopt static create factory commands earlier, since this will allow them to work across two major versions of Drush.

@greg-1-anderson greg-1-anderson marked this pull request as ready for review May 17, 2023 00:01
@weitzman
Copy link
Member

Code looks good.

  1. What happens to a commandfile that has a static create() method AND a drush.services.yml?
  2. What do we do about the drush commandfile generator. Seems like that should start generating with create()?
  3. Any changes to the DI and command authoring docs?
  4. Will there be any warnings about the drush,services.yml constraint in a module's composer.json?

@greg-1-anderson
Copy link
Member Author

  1. I think we need to address this in both 12.x and 11.x branches.
  2. Yeah I suppose we could generate static create method classes in 11.x; definitely should do so in 12.x.
  3. Yeah, could backport some of the docs here.
  4. drush.services.yml processing is unchanged (adapter not backported), so this should still behave the same way.

I'll do 3 here now; maybe 1 and 2 should be follow-on work, done in 12.x branch first.

@weitzman
Copy link
Member

Well, 4) makes the composer.json stanza unneeded for modules that choose to use create(). Want to make sure we dont error or warn in this case.

@weitzman
Copy link
Member

We should probably just remove the debug message about the composer.json stanza. That feature is going away anyway.

@greg-1-anderson
Copy link
Member Author

Ah, you're right, there's no reason to encourage folks to pin down the version selection in composer.json even in the 11.x branch. We can just take that out.

@greg-1-anderson
Copy link
Member Author

greg-1-anderson commented May 17, 2023

1. I just tested writing a command with a redundant static create method and a drush.services.yml file, and it works fine, presuming that both are well-formed. Note that commands with a static create method must be in the namespace ...\Drush\Commands, whereas the recommended location for drush.services.yml was previously ...\Commands, so overlap is unlikely, unless folks happened to move their commands away from that location and happened to choose the next one. Since it doesn't cause a problem even when done, and it would take extra code to catch the "error", I think we can just leave this alone.

4. Removed the warning in this PR. I think it's already gone in 12.x, but I'll confirm. (UPDATE: Warning still exists in 12.x branch, removed in #5570)

Still in progress on 2 and 3; will work on them in the 12.x branch first, then move here.

@greg-1-anderson
Copy link
Member Author

12.x branch already used static create when creating Drush command files; ported that code here.

@greg-1-anderson
Copy link
Member Author

Docs updates also backported; that should take care of 1-4; let me know if you notice any other gaps.

Copy link
Member

@weitzman weitzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@greg-1-anderson greg-1-anderson merged commit f76a691 into 11.x May 18, 2023
2 checks passed
@greg-1-anderson greg-1-anderson deleted the static-create-for-11.x branch May 18, 2023 05:12
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

Successfully merging this pull request may close these issues.

None yet

2 participants