Refactor ResourceObjectNormalizationCacher to use VariationCache

Something like this, so much cleaner.

The last submitted patch, 2: drupal-3379984-2.patch, failed testing. View results →

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.

The last submitted patch, 4: drupal-3379984-4.patch, failed testing. View results →

Locally this test works as intended, which confuses me. I've triggered 2 more runs, see if it was a fluke.

Yeah as I mentioned on Slack, the jsonapi tests sometimes fail in weird ways, most recently for me in 📌 Implement the new access policy API Needs work . 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.

The last submitted patch, 8: drupal-3379984-8.patch, failed testing. View results →

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.

After a few hours of investigation i made this child issue: 🐛 Cache POST/PATCH in ResourceObjectNormalizationCacher breaks MenuLinkContentTest Active . This patch above works as intented, unfortunately only menulinkcontenttest is broken if we add caching for post and patch. Rest of the entities work fine.

So for now no extra caching.

    // This class used to rely on the render cache, which only caches things for
    // GET and HEAD requests, so let's keep that functionality for now while we
    // evaluate whether this behavior is desired.
    // @todo Follow up on https://www.drupal.org/project/drupal/issues/3379984.
    if (!$this->requestStack->getCurrentRequest()->isMethodCacheable()) {
      return FALSE;
    }

Perhaps the todo in that piece of code should also mention 🐛 Cache POST/PATCH in ResourceObjectNormalizationCacher breaks MenuLinkContentTest Active ? Not married to that, but will probably help later. Other than that i'd RTBC.

Update the @todo in both places.

as per slack and #13

I'm triaging RTBC issues → . I read the IS, the comments and the comments in the MR. I didn't find any unanswered questions or other work to do.

It is nice to see a bug uncovered and a follow up made for that.

Also, three is a meta for fixing all @todos for a closed issues. The IS over there needs to be updated with this so everyone know this one has an issue in progress. Actually, I can do that now. I have updated the Meta and made this a child of that one.

Leaving at RTBC.

Thanks for the reviews!

+++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
@@ -78,8 +95,16 @@ public function setRenderCache(RenderCacheInterface $render_cache) {
-    return $cached ? $cached['#data'] : FALSE;
+    // This class used to rely on the render cache, which only caches things for
+    // GET and HEAD requests, so let's keep that functionality for now while we
+    // evaluate whether this behavior is desired.
+    // @todo Follow up on https://www.drupal.org/project/drupal/issues/3381898.
+    if (!$this->requestStack->getCurrentRequest()->isMethodCacheable()) {
+      return FALSE;
+    }
+

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. Also in comments we don't usually refer to the past status of the code base, I understand why it's been done here, but if you don't have the history you'd really wonder why we're explaining the code currently does the same thing as it used to do but in the future might not.

Could be something like '@todo investigate whether to cache POST and PATCH requests'?

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 🐛 Cache POST/PATCH in ResourceObjectNormalizationCacher breaks MenuLinkContentTest Active 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.

JSON:API returns a resource after patch. And it wouldn't neccesarily invalidate cache, what if nothing changed in the patch request? Then jsonapi could return a cached response for that resource. In naive implementations of patching a lot of resources this could be quite helpfull in speeding up those requests.

Post is a little harder to imagine no change, so thats pretty valid. Although I wonder if the currect setup also disable cache on perhaps permission denied requests that get responded. That is something that could be validated.

So thinking through this a todo in the line of '@todo investigate whether to cache POST and PATCH requests' is a good option to give a better context.

posted same second as krisstiaan

Think that change is fine @kristiaanvandeneynde although `@todo investigate whether to cache POST and PATCH requests` is a little more explicit than your todo.

I'd also say keep the reference to the issue. But perhaps in that issue we'd need to add the question if it even makes sense.

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.

Don't mean to be rude, but could you add an interdiff? That helps review a lot:

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... →

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

Hopefully this is clear enough now. :)

+++ b/core/modules/jsonapi/src/EventSubscriber/ResourceObjectNormalizationCacher.php
@@ -54,13 +61,23 @@ class ResourceObjectNormalizationCacher implements EventSubscriberInterface {
+   *   The request stack.
    */
-  public function setRenderCache(RenderCacheInterface $render_cache) {
-    $this->renderCache = $render_cache;
+  public function setRequestStack(RequestStack $request_stack) {

My last question was should/could this do 'best effort' bc where we keep the method (as a no-op) and trigger a deprecation.

Presumably if someone had wired this class up or subclassed it or something, then all the other changes would break that anyway. And also it's an event subscriber which is equivalent to a hook implementation, no-one should rely on it.

so... answering my own question: no, let's not try to do that.

Committed fde6ee3 and pushed to 11.x. Thanks!

Nice!

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