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
#918538: Decouple cache tags from cache bins makes Cache::invalidateTags() a simple wrapper for \Drupal::service('cache_tags.invalidator')->invalidateTags($tags);
As a follow-up, we could look into injecting this into services instead of calling out to the static method. That should allow us to simplify unit tests that currently need to set up a container.
Proposed resolution
Inject into new services and existing standalone services that are not commonly subclassed. (E.g., changing default plugin manager would be a bad idea at this point).
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#12 | 2398085-cache-invalidtags-12.patch | 31.25 KB | vlad.n |
Comments
Comment #1
BerdirThat got in...
Comment #2
BerdirThis might work as a novice task, if you know a about about services and injection.
Start with just one example, I would suggest Token. inject the service, then call that instead of the static method. You probably need to update unit tests for that class.
Comment #3
BerdirAnother thing to consider is performance. The invalidator uses a tagged services approach, so instantiating it will also create all invalidators that are tagged (not the cache bins, though).
Comment #4
sidharthapHere is the initial patch. @Berdir please suggest...
Comment #6
sidharthapworking on it.
Comment #7
sidharthapPatch created from new D8 instance.
Comment #9
sidharthapComment #11
sidharthapComment #12
vlad.n CreditAttribution: vlad.n commentedRe-roll.
Comment #15
Wim LeersThis is not a simple replacement. This is an addition. Therefore out of scope IMO.
This, which is what the patch is doing everywhere, is not injecting. Please inject the service. Otherwise there's no point.
Comment #16
dawehnerThe thing with the injection here is that the cache tags invalidator should not be needed on a normal page request
but just in a really rare circumstances so I'm curious whether its right to initialize them basically on every request, I guess there is no way around that?
Isn't this also an addition?
Comment #17
Wim LeersMake it a proxy service? Seems very logical.
Comment #18
pwolanin CreditAttribution: pwolanin as a volunteer commentedThis seems to be just switching to
\Drupal::service('cache_tags.invalidator')
instead of injecting the dependency?Comment #19
andypostYep all places should get service via proper DI
Comment #20
andypostComment #21
radubutco CreditAttribution: radubutco as a volunteer commentedComment #22
bogdan.racz CreditAttribution: bogdan.racz commentedComment #23
radubutco CreditAttribution: radubutco as a volunteer commentedComment #24
radubutco CreditAttribution: radubutco as a volunteer commentedThis is still work in progress, uploading the patch for a possible review.
Comment #25
Wim LeersLet's see what testbot says :)
Comment #27
Wim LeersComment #29
Mile23There's basically no way this is a novice task.
Especially when you consider that
ConfigEntityBase
is one of the classes we'll be changing here. It inherits fromEntity
which you must then figure out how to inject services into.Comment #30
Mile23Comment #31
andypostLast patch is totally wrong we can inject services into entity classes cos
Node::create()
Basically only static calls should be replaced like #12 started
this is no-go for entities
Comment #32
bogdan.racz CreditAttribution: bogdan.racz commentedComment #33
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI think the trick here is to use setter injection instead of changing the arguments.
Comment #34
BerdirNo, there is no trick. entities do not support dependency injection.