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.
donquixoteCreditAttribution: donquixote as a volunteer commented
Actually the internal (static) cache does not depend on profile directories, so there is no reason why it should be reset.
The real flaws currently of the static cache are:
The missing cache key for [$this->root]. I think adding this will fix a lot.
Static caching of mutable objects, so modifications leak back. Probably a lot depends on this already, so maybe we cannot do anything about it. Resetting the cache where we previously did not, might even cause BC breaks! Because it would mean that components that previously got the same object now get distinct ones.
That the cache is baked into the ExtensionDiscovery object, and cannot be accessed standalone. Constructing an ExtensionDiscovery object gives you access to the static cache. This is structurally wrong.
Likewise, we cannot create an ExtensionDiscovery object that has its own cache instance. This would be useful e.g. for benchmarks, and obviously for testing.
EDIT: The combination of a static cache and the file cache. See #6.
Resetting on setProfileDirectories() does not really change this.
Instead I would propose a ::staticReset() method.
donquixoteCreditAttribution: donquixote as a volunteer commented
I missed one detail in #2.
The combination of a static cache and the file cache is really a bad idea.
The premise of the static cache was that this does not really depend on anything, and that the files do not change during a request.
By adding a persistent cache into the mix, this assumption no longer holds.
Resetting on setProfileDirectories() still won't help here.
I think there is no BC-friendly way to fix this, other than to remove the file cache.
Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.
Comments
Comment #2
donquixote CreditAttribution: donquixote as a volunteer commentedActually the internal (static) cache does not depend on profile directories, so there is no reason why it should be reset.
The real flaws currently of the static cache are:
[$this->root]
. I think adding this will fix a lot.Resetting on setProfileDirectories() does not really change this.
Instead I would propose a ::staticReset() method.
Comment #3
donquixote CreditAttribution: donquixote as a volunteer commentedFor unit tests and such.
And otherwise, just fix what we can. But on the long run, fade out this component.
Comment #4
Mile23Pretty sure this is duplicate of #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing
Comment #5
Mile23Does this issue have a plan parent?
Comment #6
donquixote CreditAttribution: donquixote as a volunteer commentedI missed one detail in #2.
The combination of a static cache and the file cache is really a bad idea.
The premise of the static cache was that this does not really depend on anything, and that the files do not change during a request.
By adding a persistent cache into the mix, this assumption no longer holds.
Resetting on setProfileDirectories() still won't help here.
I think there is no BC-friendly way to fix this, other than to remove the file cache.
Comment #7
donquixote CreditAttribution: donquixote as a volunteer commentedI would suggest to do this first:
#2729711: Improve test coverage for ExtensionDiscovery