plugin.cache_clearer:
class: Drupal\Core\Plugin\CachedDiscoveryClearer
lazy: true
However the service is always instantiated and used directly so the lazy proxy is pointless:
\Drupal::service('plugin.cache_clearer')->clearCachedDefinitions();
Even worse, the service is not lazily instantiated anyway:
> $proxy = \Drupal::service('plugin.cache_clearer');
= Drupal\Core\ProxyClass\Plugin\CachedDiscoveryClearer {#8061}
> (new ReflectionProperty($proxy, 'service'))->getValue($proxy);
= Drupal\Core\Plugin\CachedDiscoveryClearer {#8062}
The $service property should not be set until a method is called on the actual service, but the addCachedDiscovery method calls configured in PluginManagerPass trigger the instantiation of the real service as soon as the proxy service is created.
Remove the lazy proxy. Use a tagged iterator instead of calls to addCachedDiscovery.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !7134
Comments
Comment #2
longwaveComment #4
longwaveComment #5
longwavePostponed until we land #3432595: Use a tagged service iterator for uninstall validators instead of individual lazy proxies or #3414208: Add support for tagged_iterator to YamlFileLoader so we can use tagged iterators without having to add support code.
Comment #6
longwaveComment #7
longwaveComment #8
smustgrave commentedAny recommended way for testing this one?
Comment #9
longwaveIt's a kinda low level refactoring, any automated test coverage that uses the
plugin.cache_clearerservice directly or indirectly should be enough to prove I haven't broken anything.Comment #10
smustgrave commentedRan the test-only feature https://git.drupalcode.org/issue/drupal-3432827/-/jobs/1378533 which I'll admit I'm leaning on.
Looking at the refactor though don't see anything that stands out that could be a problem. And tests are still passing.
Comment #11
catchThis looks good. There are constructor and interface changes here, but I really wonder why we have an interface for this at all.
Tried to think what a deprecation looks like, I think we need a 10.3.x MR that adds @deprecated to the interface method we're removing. I'm not sure we should do anything else though because this is so low level and unlikely to be interacted with. Added a CR.
Comment #12
longwaveThe problem is that by leaving the method on the interface, even if deprecated, is that anyone else implementing has to also leave the implementation in place. Instead I have still removed the method from the interface, but kept it on the concrete class, and added BC so it still works but issues a deprecation if you call the old method.
This is also too late for 10.3 now, so I have deprecated in 11.1 for removal in 12.0.
Comment #13
smustgrave commentedFailure seems unrelated to this change, HEAD may be broken as I'm seeing the same failure across multiple MRs
Deprecation update seems good though.
Comment #14
alexpott@longwave why did you remove autoconfiguration?
Comment #15
catchComment #16
longwave@alexpott that only works if you specify
autoconfigure: truein each services.yml that declares a plugin manager, so it's not backward compatible (similar to what we found in #3446026: Adding media library openers use autoconfigure and tags in 10.3.x has BC consequences)Comment #17
longwaveQuestion answered, back to RTBC.
Comment #19
catchOK yeah I tried to use autoconfigure in another MR recently and ran into the same issue that every services.yml needs to enable it. We should open an issue to do that in core and try to figure out a way to encourage contrib to do it (or could we switch things so it's enabled by default in a minor?).
Also talked to @longwave about whether we should try to backport this to 10.4.x, but since it shouldn't affect contrib at all, it doesn't really help with 10.4.x/11.1.x compatibility. So... let's leave it in 11.1.x.
Committed/pushed to 11.x, thanks!
Comment #20
catchOpened #3449569: Use autoconfigure more in core.
Comment #21
quietone commentedAdding tag for follow up in #19
The change record is mostly a diff. The change record should explain the change, what sites are affected and the action that needs to be taken.
Comment #22
smustgrave commentedOpened #3449635: Enable autoconfigure for services.yml
Not sure I can update the CR
Comment #23
znerol commentedFixed the change record. I hardly can imagine that anybody is overriding this service or calling into
addCachedDiscovery()from some weird place. What do I know, though...Comment #24
quietone commentedThe followup created in in #22 was closed as a duplicate. The follow up already existed, it is #3449569: Use autoconfigure more in core
@znerol, thank you. That is much better.
The change record is published.
Comment #25
quietone commented