Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
cache system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
19 Jan 2024 at 18:27 UTC
Updated:
26 Feb 2024 at 11:14 UTC
Jump to comment: Most recent
Problem/Motivation
In #3397522: Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core we are trying to reduce the use of ContainerAwareTrait as Symfony has deprecated it.
CacheTagsInvalidator is container aware because it needs to retrieve all cache bins by ID.
Instead of injecting the entire container we can inject the bins via a service collector method.
Steps to reproduce
Proposed resolution
Add service collector tags to CacheTagsInvalidator.
We already use service collectors for the invalidator services.
Merge request link
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3415940
Show commands
Start 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:
- 3415940-convert-cachetagsinvalidator-to
changes, plain diff MR !6247
Comments
Comment #3
longwaveComment #4
longwaveComment #5
longwaveComment #6
spokjePHPCS upset about now-useless use statement.
Comment #7
longwaveComment #8
spokjeFWIW: One nitpick, one question and a general "I don't fully grok the test before nor after, but I'm pretty sure it's testing the same thing in both cases.".
Comment #9
longwaveThanks for reviewing. I addressed the feedback.
Regarding the test: Previously the
getInvalidatorCacheBins()looped through both non-memory and memory cache parameters, and then could handle services that do and don't implement CacheTagsInvalidatorInterface, so four cases in total. Now we don't need the parameters or cache bin types, the test only needs to check services that do and don't implement that interface, so there are only two cases - the memory-specific ones are redundant.Comment #10
spokjeThanks @longwave, I kinda sorta figured that about the test, all other feedback is addressed: => RTBC.
Comment #11
longwaveComment #12
kristiaanvandeneyndeWe cannot mix cache bins and memory cache bins as the latter do not 100% behave as a cache bin. For more info, see #3402850: Fix MemoryCache discovery and DX
Comment #13
longwaveThanks for reviewing. I get why they need to be separated in the cache factory, but not in the invalidator; as long as they implement the interface normal bins and memory bins are invalidated in the same way.
Comment #14
kristiaanvandeneyndeAs I mentioned in the review, will ask for a second opinion on whether we need to be this strict with keeping the two separated. Still NW because of the property typehint suggesting something that may not be true.
Comment #15
kristiaanvandeneyndeSorry, in my latest review comment I misread a few lines. The property can be typehinted as such as you check for the interface when the service is collecting the tagged services.
Comment #16
kristiaanvandeneyndeTo be fair, now that I've taken a closer look and that you're checking the tagged services for the invalidator interface, I'm less fussed about it. The code makes it clear that you're not interested in the cache backends as caches, but rather as services that might implement the cache tags invalidator interface.
Just poked @catch on Slack and he also seems to be fine with it under the circumstances presented here. Sorry about the previous noise but seeing the two types of cache backends mixed together in one property automatically raised a red flag with me. After further review, it's actually quite OK the way you implemented it.
RTBC
Comment #17
longwaveNo worries, I appreciate thoughtful reviews that take time to question what I did and why, especially when it might have side effects elsewhere.
Applied the comment improvement, leaving at RTBC.
Comment #19
catchAgreed that for tag invalidation we don't need to make the persistent/memory distinction. Committed/pushed to 11.x, thanks!