- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Drush 10 / Symfony 6 compatibility #5108
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
Conversation
@@ -42,7 +42,7 @@ public function example() | |||
* | |||
* @hook init sut:simple | |||
*/ | |||
public function customLogger(DrushArgvInput $argv, AnnotationData $annotationData): void | |||
public function customLogger(ArgvInput $argv, AnnotationData $annotationData): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we have a breaking change here (which hooks are affected?). Just noting it for release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a breaking change. If it is, we can put back DrushArgvInput and make it an empty extension of ArgvInput.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess at a minimum this is a breaking change for anyone who copied DrushArgvInput
from this code into their hook. I think that technically, though, using DrushArgvInput
or even ArgvInput
as I have here is a bug. It should be InputInterface
, so that there isn't a type error if a non-ArgvInput $input is injected. I don't think it's possible for $input to be incompatible with ArgvInput, but it will be incompatible with DrushArgvInput if --strict=0
is used.
Maybe worth putting into the release notes, but I don't think it's absolutely necessary to maintain b/c in Drush 11. If you want to, though, we can do what I said above and put an empty DrushArgvInput class for Drush 11, and remove in 12. That's pretty low effort / low impact, and the most conservative thing to do.
I think I ran into this problem. Let me help the search engines direct people to this page:
As a work-around, I hacked Drupal's |
5c320b6
to
38c5883
Compare
All green, just a little more cleanup to do. |
Found another thing. |
9bf4eb5
to
71f1acd
Compare
Squashed and rebased with HEAD of 11.x. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks.
Symfony 6 changed the method signature in a number of classes that we extend. It is therefore not possible for us to use the same source file with both Symfony 6 and Symfony 4/5. However, we do not want to have a different Drush executable for Drupal 9 and Drupal 10.
To get around these seemingly incompatible goals, we introduce a dynamic class loader, that will bring in a different set of sources for different major versions of Symfony. We use the Symfony 4 sources with Symfony 4 and 5, because they are compatible. Symfony 5 is only used in early versions of Drupal 10, though, so perhaps we do not need to support it long term.