Refactor ResourceObjectNormalizationCacher to use VariationCache

Something like this, so much cleaner.

This fixes NodeTest, it was testing the caching layer directly. No clue why MenuLinkContentTest would fail all of a sudden because there is zero change being made other than swapping out the cache with the one render cache now uses internally anyway.

Yeah as I mentioned on Slack, the jsonapi tests sometimes fail in weird ways, most recently for me in #3376846: Implement the new access policy API. I've submitted the work to convert to the new variation cache and it would be awesome if someone with more jsonapi knowledge (I have almost zero) could fix this sole test failure.

Do not commit this, but this makes it go green locally. So it seems the test is running into caching issues because triggering a role save makes it work. I've run into quite a few occasions where the jsonapi tests go out of sync because they run multiple requests from one process and then you have to call $this-refreshVariables() to get the test back in sync.

But, at first sight, this does not seem like such a case. It really seems like there might be a bug in the test or jsonapi here because the VariationCache now powers the render cache. So swapping out the workaround using the render cache with a direct VariationCache call shouldn't change anything.

Either way, let's see what the testbot says, but patch from #4 is still the current one we need to focus on.

One thing I noticed is that RenderCache still contains this piece of code:

  public function set(array &$elements, array $pre_bubbling_elements) {
    // Form submissions rely on the form being built during the POST request,
    // and render caching of forms prevents this from happening.
    // @todo remove the isMethodCacheable() check when
    //   https://www.drupal.org/node/2367555 lands.
    if (!$this->requestStack->getCurrentRequest()->isMethodCacheable() || !$this->isElementCacheable($elements)) {
      return FALSE;
    }

Which, as explained in the comment, makes sense for render arrays, but not for other services abusing the render cache by dumping just anything into a render array. This is the only thing that does not run when you swap from render cache to VariationCache, so that might be the culprit.

isMethodCacheable() returns TRUE for GET and HEAD so, before, nothing in that test would get cached aside from the penultimate GET call. Now, all POST requests above that also get cached (if jsonapi allows it, I don't know jsonapi that well). Either way, it might be that the test was written around a broken cache implementation because it sure seems as ResourceObjectNormalizationCacher would cache nothing but GET/HEAD requests in the past.

This also seems to be proven by the fact that NodeTest::assertCacheableNormalizations() checks using GET. I'm fairly sure if we change said test to use POST, PATCH or whatever that isn't GET/HEAD, that it would fail with the old code and succeed with the new.

Huh, locally on PHP 8.1 it goes green with the latest patch and red without. Seems like @bbrala is facing similar things.

This keeps the behavior of render cache where only GET and HEAD are considered cacheable. This was not necessary for other parts of core where we swapped out the render cache use with VariationCache (such as DynamicPageCacheSubscriber), but it might make sense here because jsonapi actively deals with HTTP methods in requests.

Either way, the fact that, without this, the test would still go green for POST but not PATCH when we cache all HTTP methods worries me. It seems like using the render cache for this long obscured a potential bug in jsonapi, so I left a @todo in this patch to follow up and investigate on why the tests failed for PATCH when we cache all HTTP methods.

Having said all of that, I expect this patch to go green because now it's fully idempotent compared to the old behavior.

Update the @todo in both places.

Thanks for the reviews!

It's not clear to me why we'd ever want to cache PUT and PATCH responses which are likely to invalidate the cache anyway.

We did uncover that POST/PATCH works for all but one test. That's why the @todo points to #3381898: Cache POST/PATCH in ResourceObjectNormalizationCacher breaks MenuLinkContentTest where this is specifically being addressed. Having said that and not being too well versed in the ways of jsonapi, if POST/PATCH return the object that was created or updated, then it makes no sense to cache those responses as they will always be invalidated on the next POST/PATCH call.

How about:

+++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
@@ -78,8 +95,16 @@ public function setRenderCache(RenderCacheInterface $render_cache) {
-    return $cached ? $cached['#data'] : FALSE;
+    // @todo Figure out if we want to support all methods and drop this check.
+    // @todo Follow up on https://www.drupal.org/project/drupal/issues/3381898.
+    if (!$this->requestStack->getCurrentRequest()->isMethodCacheable()) {
+      return FALSE;
+    }

Splitting it up into two @todos means we could either fix the failing test and still keep the first @todo under discussion or fix them both in one go. Keeping the reference to the other DO issue also gives some background on why this is being discussed in the first place.

Re #21 Okay, updated as such. Setting back to RTBC as this is merely a comment change.

Actually, this is what you get when trying to be quick and use patch workflow. I forgot to update it in the other place. Leaving at needs review because I am too trigger happy it seems. Sorry about that, trying to squeeze this in between project work.

Sure thing, should have used MR from the start to be honest :D