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.
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.
Maybe LegacyServiceFinder instead?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
"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.
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.
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.
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.
| * 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.
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.