Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
\Drupal\Core\Extension\ExtensionDiscovery is not a service. It is always instantiated in hard-coded ways, and stores all discovered extensions in a static class property. Once extensions in a particular extension collection directory (such as ./modules or ./modules) are discovered, the statically cached information can and will not be updated until the next request.
As a consequence of this behavior, enabling a module and subsequently using it in the same request is impossible, because enabling the module requires a bootstrap, which requires extensions to be discovered. Any extension that is enabled after bootstrap, will not be available until the next request.
This has caused minor problems in the past, even in Drupal 7, but currently this is a major roadblock for Console.
Proposed resolution
Convert the extension discovery into a service and add a method to reset scanned dependencies, forcing the class to scan again upon the next request for the list of available extensions.
This will also allow us to inject the extension discovery into the module and theme installers, and trigger a rescan when a new extension is installed. This can be done in a follow-up.
Remaining tasks
Convert ExtensionDiscovery to a service and add a method to reset the static cache.
User interface changes
None.
API changes
Code instantiating ExtensionDiscovery must be converted to use the service instead, either through injection or service location.
Comments
Comment #1
XanoComment #2
dawehner#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is still the thing IMHO which should be actually done.
Comment #3
jibranI agree with @dawehner.
Comment #4
jibran@Xano would you like to make this argument in #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList so that we can change that to bug report and convince core committers?
Comment #5
jonhattanWe have the very same problem in Drush - https://github.com/drush-ops/drush/issues/5
Comment #6
gapple#2156401: Write install_profile value to configuration and only to settings.php if it is writeable adds a static reset method to ExtensionDiscovery for testing purposes. (Which unfortunately conflicts with DrupalConsole's non-static declaration in a subclass).
Comment #7
dawehnerOne thing you have to keep in mind is that at the moment the ExtensionDiscovery is by design not really swappable, because its needed to build the actual container, so wapping it out on the container level would work different than expected.
Comment #8
Mile23I like this idea, but with some caveats mentioned here: #2186491-17: [meta] D8 Extension System: Discovery/Listing/Info
I'm coming at this because I want to be able to test
ExtensionDiscovery: #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testingBasically we need to be able to use
ExtensionDiscoverywithout a container or any dependencies, in order to do super-quick test discovery.Comment #9
dawehnerWell, we even need it in order to be able to not have circular dependency problems.
So to be clear, with #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList you'll no longer use the ExtensionDiscovery in any of your code.
Comment #11
donquixote CreditAttribution: donquixote as a volunteer commentedIt is a noble effort trying to fix existing components.
But maybe it is easier to write new components that work as promised, and use them as services or whichever way we like, while leaving the old ones in place. Usages of the old components would slowly pick up on the new ones.
The ExtensionDiscovery has some problems which can be fixed. One of them is adding the additional key
[$this->root]to the static cache, as in #2605654-48: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing.But beyond this, we probably need to leave some of the crappy behavior in place for existing components that are already depending on it. One of the flaws is the existence of a
setProfileDirectories()method, which makes this object mutable and thus not suitable as a service.If a depending component already needs to be modified (to use a service instead of calling a constructor), it can as well use a different tool that does the job more reliably.
I experimented, and came up with an architecture where a lot of things can be swapped out for testing, and individual components can be tested individually.
#2724231: Replacement for ExtensionDiscovery, following SOLID principles
I think this is the direction to go. Not necessarily 1:1 my demo code, but the direction.
Comment #12
XanoIt's clear to me that we cannot make this a service. Could we make it a low-level setting instead? That way we allow experimental discovery methods while keeping this out of the service container.
Comment #22
catchMarking duplicate of #2724231: Replacement for ExtensionDiscovery, following SOLID principles.
Comment #23
Mile23