-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Create a service adapter for drush.services.yml #5553
Conversation
…rvices.yml discovery in the Drush Drupal Kernel trait.
src/Runtime/DrushServiceFinder.php
Outdated
*/ | ||
class DrushServiceFinder | ||
{ | ||
|
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.
Committed by mistake, but might end up using eventually. Intended as the home for the drush.services.yml discovery methods, but I put that work off and just used the existing code in the Drush Drupal Kernel trait for the initial PoC.
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.
Maybe LegacyServiceFinder
instead?
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.
Good suggestion; this code vanishes if support for drush.services.yml is ever removed, and we are going to deprecate that even if it sticks around for b/c.
src/Runtime/ServiceInstantiator.php
Outdated
|
||
public function __construct(protected ContainerInterface $container, protected ContainerInterface $drushContainer) | ||
{ | ||
$this->drushServicesContainer = new DrushContainer(); |
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.
Services created in a drush.services.yml file are stored in this container so that they can be referenced as needed e.g. as the parameter to some other service or command also in a drush.services.yml file. Usually, only services in the same file can reference each other, as there is no other way to ensure the initialization order of Drush services.
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.
Maybe with a little extra work, we could use the features of league container to lazy-initialize the commandfiles. I'm not sure if it's necessary to allow cross-Drush-service-file references, though. Since Drush services can reference Drupal services, it would be preferable to just make a Drupal service for any shared service needs a Drush command might have.
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.
Is this a third container? I have no idea if there is a better way but its at least a thing that will raise an eyebrow.
- Original Drush container
- New container containing Drupal service data
- Drupal container.
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.
We could re-use the drupal container or the drush container or just put it in an array. The scope is only during instantiation; it is not needed thereafter.
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.
Looking great. Thanks for pursuing my musing from long ago.
src/Runtime/ServiceInstantiator.php
Outdated
/** | ||
* Given a drush.services.yml file (parsed into an array), | ||
* instantiate all of the services referenced therein, and | ||
* cache them in our dynamic service container. |
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.
"cache them" - the drush containers are rebuilt from scratch on every request, right? maybe we can say 'collect them' instead. if we are dumping containers to disk then we a similar cache rebuild issue that plagues the current module defined commands.
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.
Ah, yeah, there is no persistent cache here.
src/Runtime/ServiceInstantiator.php
Outdated
|
||
public function __construct(protected ContainerInterface $container, protected ContainerInterface $drushContainer) | ||
{ | ||
$this->drushServicesContainer = new DrushContainer(); |
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.
Is this a third container? I have no idea if there is a better way but its at least a thing that will raise an eyebrow.
- Original Drush container
- New container containing Drupal service data
- Drupal container.
…ompiler pass to find commands and etc.
…er. Use ServiceManager to pass generators to the generate command.
… tests only bootstrap once
Down to one failing test. In this PR, the archive restore command fails:
Sure enough, it is not defined. If I switch back to the 12.x branch, though, this same test works, but |
…, already commented-out code.
…viceManager class so that a referece to the autoloader is not needed (directly) by the Generate command.
src/Runtime/LegacyServiceFinder.php
Outdated
|
||
public function getDrushServiceFiles() | ||
{ | ||
if (empty($drushServiceYamls)) { |
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.
Probably it should be $this->drushServiceYamls
src/Runtime/ServiceManager.php
Outdated
* (DrushBoot8) using the LegacyServiceFinder and LegacyServiceInstantiator | ||
* classes. | ||
* | ||
* TODO: That's the intention, anyway. For now, we just stuff gererators here. |
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.
* TODO: That's the intention, anyway. For now, we just stuff gererators here. | |
* TODO: That's the intention, anyway. For now, we just stuff generators here. |
{ | ||
} | ||
|
||
public function loadServiceFiles(array $serviceFiles) |
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.
Lets add doxygen, param hints, return type hints, etc.
I've still got a little doxygen and a couple TODOs to do, but this is pretty close to ready to merge now. |
Done with the refactoring. I'm planning on leaving PSR-4 command info alterers as future work, so this PR is now down to just "naming things" and docblock work. I'll merge as soon as I'm done with that, unless I get more feedback. |
…alized due to a missing required parameter.
Current status: admire, but do not use.
A while back, @weitzman had an idea that we could create a drush.services.yml service adapter that could read these files and instantiate them ourselves, without using a Symfony compiler pass at all. This PR is the beginnings of just such a service adapter.
Currently, the existing Drush Drupal Kernel trait is being used to find drush.services.yml. Because of the way the Drupal Kernel trait is tied to the Drupal service container, and because we are no longer storing Drush services in the service container, the result at this time is that the very next one Drush call after a
drush cr
(e.g.drush list
) will see all of the module commands, and each Drush call after that will not see any module commands at all. Factoring the drush.services.yml discovery out of this trait should clear up this problem.More work may be needed for command info alterers, generators and etc., but it seems that the adapter is working fairly well so far, so the technique seems promising. The result is that with this adapter, we will not need to remove support for drush.services.yml, and folks won't need to rewrite their module Drush commands to take advantage of the server-containerless DI of Drush 12. The new static create factory syntax will still be recommended and preferred, of course, since it is clearer and easier to read, but using it doesn't have to be mandatory with this adapter.