Convert CacheTagsInvalidator to use a service collector

PHPCS upset about now-useless use statement.

FWIW: 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.".

Addressed 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.

Thanks @longwave, I kinda sorta figured that about the test, all other feedback is addressed: => RTBC.

We cannot mix cache bins and memory cache bins as the latter do not 100% behave as a cache bin. For more info, see 📌 Fix MemoryCache discovery and DX Needs review

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.

As 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.

Sorry, 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.

To 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

No 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.

Agreed that for tag invalidation we don't need to make the persistent/memory distinction. Committed/pushed to 11.x, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.