ItemCacheTagsTest and ShortcutCacheTagsTest needlessly test the render cache.

Rerolled patch from #14

ItemCacheTagsTest was removed with aggregator so title doesn't need it

Left a comment on MR.

Trying to get feedback.

Think my one comment still stands but will leave in review for others. Certainly not a hill I’ll die on lol

Haha, I’m with you on being efficient and avoiding unnecessary duplication of resources.

We could refactor EntityCacheTagsTestBase so that the cache bins are stored in a class member variable within the setUp() method, which is a common practice. However, I always try to be cautious about not straying too far from the original scope.

You have a lot of experience, so I’m always a bit worried about overlooking something.

Let’s see if others can share their thoughts, and we’ll decide from there. :)

Thanks so much for all the effort you put into reviewing so many (often complex) issue queues!

Been some time without additional feedback. Since the one open thread was my own going to mark and see what a committer things. Since this is tests optimization I'm not as picky with the thread so it could be ignored.

This method doesn't do any http requests, so I think it could move to a kernel test?

I hadn't realized that! Good idea! 🙂

I've implemented it and also extracted some common functionality into a trait.

Not sure if I missed it the first time but looking at the summary doesn't mention adding a new trait. Think if that's going to be an approach IS should be updated and a CR written.

Thanks for your review @smustgrave!

Using a trait wasn’t the initial decision, but after @catch’s suggestion to move Functional/ShortcutCacheTagsTest::testEntityCreation() to a kernel test, it was no longer possible to reuse the functions through inheritance. As a result, they were moved into a trait.

I've updated the proposed solution in the IS to reflect this.

I don't think a CR would be necessary since EntityCacheTagsTestBase, which originally contained these methods, now uses the trait, meaning any subclass will automatically inherit them. Additionally, since these functions were not static, a static call like EntityCacheTagsTestBase::getRenderCacheBackend() would never have existed anywhere.

Will leave for others then. But if we don’t want to declare a new trait then not sure I see the point of doing it..

Traits can be a bit tricky, as they were introduced to solve the multiple inheritance problem.

I found this article quite interesting when I read it some time ago: PHP Traits: If They Weren’t Evil, They’d Be Funny.

If we consider traits as an "official" tool for developers to use across multiple tests, then you're probably right about the need for a CR. However, in this case, the trait is simply a way to reuse code without overcomplicating things.

Would it have been better to implement this with composition using a service or something similar? Or is a trait a bigger architectural change that warrants a CR to properly inform developers about this new tool?

These are still open questions for me, given my yet limited experience with the core codebase. 🙂

If we added a new trait to the runtime codebase it might warrant a CR, but a trait just to share logic across a couple of tests does not need a change record, for the same reason test code in general is considered @internal per https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

Thanks for your input, @catch. Sometimes the boundaries are unclear, and I find it valuable to hear multiple perspectives and learn from them for future situations.

The MR seems close to what I originally had in mind. Only not fond of the naming. The trait might imply that it also works for regular caches, but that's not the case as it's heavily oriented towards variation caches.

Thanks for your suggestion, @kristiaanvandeneynde!

After reviewing it more thoroughly, I'm not entirely sure about the need for "Variation" in the method name. Perhaps "Variation" is an implementation detail, there isn’t another method that directly returns a CacheBackendInterface, and more importantly, the method is type-hinted.

However, I've made the change for others to review. 🙂

Yeah it was merely a suggestion. I'm fine to review either way you want to change it, but I do think we should try to make it so our trait and method names cannot be considered ambiguous.

Yes, yes, I totally agree! I'm very concerned about naming, I always take my time to make sure I choose the best possible name. It's just that this time, I'm not entirely sure because of the questions I mentioned :)

Hmm, now EntityCacheTagsTestBase loses the getRenderCacheBackend() method and gets getRenderVariationCacheBackend() in return. This could upset people extending said base class.

Then again, the previous name was incorrect now that it was returning a VariationCacheInterface, which is not a backend, but a wrapper around a backend. I need to fix VariationCacheFactoryInterface in that regard.

So, with my sincerest apologies, could we have one more round of changes please?

• getDefaultVariationCacheBackend() -> getDefaultVariationCache()
• getRenderVariationCacheBackend() -> getRenderVariationCache()

Then the names match what they are actually retrieving.

From the BC policy:

Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered an internal API and may change if necessary.

So on the base class we should have the old method (getRenderCacheBackend) call the new method (getRenderVariationCache) along with a deprecation notice.

FYI: 🐛 VariationCacheFactoryInterface documentation isn't fully correct Active

No worries! The search for a good and elegant solution often requires a few iterations. 🙂

I think I searched for other uses of EntityCacheTagsTestBase::getRenderCacheBackend() and couldn't find any, but I overlooked the possibility that contrib modules might be extending this class as well, so I think it's a good idea to keep the method and deprecate it.

I've fixed the naming as you suggested, restored the getRenderCacheBackend() method, deprecated it, and created a CR.

Thanks for the detailed information you provided!

Let me know if I've forgotten something.

Amazing work, got nothing left to say.

Edit: I wonder if test classes are considered @internal, meaning a CR and deprecation wouldn’t be strictly necessary. However, doing so is certainly the more considerate approach.

fwiw this is exactly how we approach it most of the time, if we really need to skip deprecating and introduce a hard break then we can and will, but if it's straightforward to provide bc then that's preferred to avoid disrupting contrib modules too much/often in minor releases.

Committed/pushed to 11.x, thanks!

catch → closed merge request !10908

Thanks for clarifying this, @catch 😊