Found by removing the cache max-age=0 hack, applying solution described by #3033457-2: Use a lazy builder for placeholdering would cause toolbar button caches to become poisoned. For example:

  • Navigate to one node
  • Navigate to a second node.
  • The second node would contain the link details of the first.
  • When opening the sidebar the first node sidebar would be displayed instead of the second.

Patch reworks cacheability of \hook_toolbar(), removing max-age=0 and adding route context since the button varies by route.

CommentFileSizeAuthor
sidebar-cache-poisoning.patch1.77 KBdpi

Comments

dpi created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Doing this with a cacheability object makes it a bit complicated due to the many early returns. Wondering if the code would be easier if we'd refactor it to only have a single return and invert/nest the different checks, but that might have its own drawbacks. This is going to conflict with #2988331: Content translation moderation already now quite a bit and that refactoring would obviously make it worse.

Not 100% sure if it's better to placeholder this or just cache it. I guess if it is indeed just the route cache context and $entity as the cacheable dependency then it should be OK since this currently doesn't do any access checks or other more dynamic things (like checking if there are any local tasks). So, seems like a good idea to do this now and keep the placeholdering issue open as an option if we need to add more contexts.

dpi’s picture

dpi
Location Perth, Australia
commented

Thanks @Berdir, I agree the many early returns are ugly, the whole function needs a bit of cleanup for readability. But also dont want to be disruptive if another issue is touching these LOCs.

So far the patch is working well on a pre-prod environment.

dpi’s picture

dpi
Location Perth, Australia
commented

Creds

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Sidebar toolbar button cache forever/incomplete contexts » Make moderation sidebar toolbar element cacheable with correct cacheability

Updating the issue title, because what you are describing is a problem with your custom hook and doesn't happen with just moderation sidebar IMHO. The problem is that it is completely uncacheable right now :)

I just tried and the patch in fact seems to not conflict with my issue, which is surprising. I was going to upload a patch that keeps the array cacheability definition on top and just adds a single line to do this:

CacheableMetadata::fromRenderArray($items['moderation_sidebar'])->addCacheableDependency($entity)->applyTo(...);

But since it doesn't conflict, waiting with that for now, lets see what the maintainer thinks.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Did a retest to make sure this still works with the new tests, we're doing quite a few variations there with different nodes, so I think if we have bugs it should fail now. If not then this can be committed IMHO.

Note that the effect of this might not be immediately visible without #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable in manual testing. But either without the shortcut module or with that patch, it should be possible to see that the dynamic page cache header goes to HIT/MISS with this instead of UNCACHEABLE.

Might also be possible to specifically test that, didn't check that yet.

  • dpi authored a6ea622 on 8.x-1.x 
    Issue #3034261 by dpi, Berdir: Make moderation sidebar toolbar element...
samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Looks good to me - I think we can trust the current test coverage and add more if this causes any regressions. Thanks all!

Status: Fixed » Closed (fixed)

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