Support CacheOptionalInterface in BlockViewBuilder, use it in language switcher block to prevent max-age 0 bubbling

The block cache tags should also depend on language of the interface which was used to render the block.

Is this not same / similar to #2318437: Replace the hardcoded langcode key on blocks with the 'language' cache context?

To elaborate on that, thanks to #2107427: Regression: Language names should display in their native names in the language switcher block the switcher will be different per each language at least, so if the links are different, no help in doing it by JS, since the link text will also be different. Not much cached then, if you want to both inject the href and the text by JS. So is there a point in caching it NOT by language then? If we cache by language then it is the same as #2318437: Replace the hardcoded langcode key on blocks with the 'language' cache context AFAIS.

@Gabor: The language texts will be the same everywhere, I don't see how that would be a problem? The problem with the URL is that it would be different on every page, that's what makes caching hard.

Ha, right. I guess caching per language per page is not that much values / too many cache entries.

Yay, #2107427: Regression: Language names should display in their native names in the language switcher block now landed, this may be continued.

Posting at least to the point, what I did, till I read the comment, to not work on it ;)

It adds the Javascript part and the redirect URL, so the basic idea is working. Caching-Part I didn't touched.

Updated the summary as per comment #11

Put that at the top of the summary.

I'm going to work on this @ drupaldevdays Montpellier

Uploading work in progress:

* For anonymous users, we have a post_render_cache which performs a str_replace. Not sure if this is the best way.
* For authenticated users, we link to home page in the given language. If JS is enabled, we enhance this with the current path.

Contains a new IsAnonymousUserCacheContext.
We still lack to clear the caches if new language are added, and we need to handle the current language so it's considered the active one.

Sorry, no interdiff. Next steps would be clarifying those points, cleaning up and injecting services where needed.

Thanks for reviewing!

1. Using that cache context now.
2. Now we generate the context placeholder dinamically. However, as it is parsed as an url we need to do some decoding magic. We need to clean that up in next iterations of the patch.
3. Used that, injected Render too.
4. Fixed that.
5. Actually I noticed that we can have different blocks per language negotiation method. Added the derivative id which contains the negotiation type.
6. Not only translations should invalidate this, but adding languages themselves.

Note:
I'm using \Drupal::routeMatch() instead of \Drupal::destination()->get() because destination comes with a prefix. We need the internal path. E.g (node/1, never /node/1 or /es/node/1). Maybe we need to figure out a better way.

We are still missing modifying existing tests, and we need to add the active class to the current language for the current negotiation this block instance cares about. As we use a redirect handler, the active-link won't work here unless we do something about it.

1. We talked about this and that won't work. We need to add the active class for the active language, as active-links don't detect this as it is not the current route. We may want to fake data-drupal-link-system-path so active-links detects it, or do it ourselves in JS for registered + #post_render_cache callback.
2. Good catch.
3. Added as a service, which enables services injection.

Also renamed the controller to LanguageSwitcherController, which makes more sense.

Rerolled.

Another reroll.

The @data-drupal-link-system-path now points to language-redirect. We were using that from the JS active-links, so we need to do:

a) Fake data-drupal-link-system-path so the active-links library works for the language switcher links.
b) Use our own js for settings active links.

Which is the preferred way?

The issue summary and #21 both mention a JS solution for authenticated users is the best approach. Why exactly is that and why wouldn't using #post_render_cache work here?

@Xano: while I don't know the answer to that, it looks logical that service a static generated HTML fragment and a static JS file (as part of an aggregated JS file that is already locally cached) would be faster to serve from the server if there is no post-render PHP required?

Also #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache is removing post render cache.

@Gábor Hojtsy: Thanks for taking the time to answer my question. I was always taught about graceful degradation or, more recently, progressive enhancement, so relying on JavaScript for this sounds like bad practice when I follow these approaches to review the patch. I'm really only trying to understand the reasoning, rather than criticize the chosen approach.

@Xano: yeah the question is what to gracefully degrade to. The language switcher links without JS proposed by @Wim would degrade to switching to the front page of that language I guess. I am not saying I am fully content with that as a degradation target, but if the performance difference is significant, then it makes more sense then needing to hardcode frontpage language links instead on a site to avoid using this dynamic block AFAIS.

Unassigning stale issue. Hopefully someone else will pursue this.

So after a while of trying to find out why my pages were not being cached, I finally stumbled on the language switcher block and this issue in the comments.
The patch and direction from 3 years ago seem a bit complicated from my point of view, but I might be missing something. I feel we could just add the correct cache context to the render array of the cache block and remove the cache max-age to get the desired result.
I can see that you don't really want the language block to be cached for every language, page and url query values. But there is so much variance in the used links, which could also have been modified by hook_language_switch_links_alter. And I personally think the links should be output in the page's html code, not added via Javascript. But if you don't agree, consider my patch a workaround until there is a clean solution in core.

The thing here is that I really don't mind having a lot of cache entries (up to one per page per language), so long as the page itself has a usable max-age for varnish to cache it. But the way it is in core right now, max-age will always be 0. So far the attached patch seems to be working fine on my site, though I'm not sure I'm using all the correct cache contexts.

Page cache and the external cache status don't care about max-age, so this should not be the reason that your pages are not cached. And for authenticated users, it should use placeholders.

BTW @Mirroar returning a render array from getCacheMaxAge is invalid.

@alexej_d You must have misread the patch, it removes the getCacheMaxAge function.

@Berdir From my understanding the max-age bubbles up to the page, but retesting again without my patch, the Cache-Control headers seem to be fine all of a sudden. So I guess the current core solution simply doesn't render cache the language switcher block but doesn't affect the response's cache headers, which is what I was worried about.

Sorry for the confusion.

Yes, see #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

But I'll comment over there about this now, because once that issue is fixed/changed, what you expected/assumed is exactly what would happen.

Fixed the patch, but it doesn't seem to help, still getting Cache-Control: must-revalidate, no-cache, private in the response headers.

@mErilainen: This problem does neither cause nor fix that problem, that must be something else, could be captcha or something else that calls the page cache kill switch.

I updated the patch and cleaned it up a bit. Note that those patches didn't work at all because it should be 'contexts' not 'context', so there were no contexts on that block.

I think instead of custom solutions here, we should look into my ideas in #2888838-11: Optimize render caching. If we'd prevent render caching of (small) parts that are cached per url/query args and instead use auto-placeholdering for that, then it would just work as far as I see.. I quickly tested with putting url.query_args into the auto-placeholder conditions and it worked as expected, but it is still getting render cached as far as I see.

Lets see what testbot thinks about it.

Oops.

Needs IS update and probably a regression test.

A test is tricky, there is no actual bug, it simply doesn't get cached. The only thing we could maybe test is that cache entries are created for it.

The other thing we could do in preparation of #2352009: Bubbling of elements' max-age to the page's headers and the page cache is adding explicit test coverage that a page with a language switcher is cached, but that will not fail in HEAD, it would only fail with that other issue without this patch.

From a developers perspective I'd go for #56: It's the most straightforward implementation respecting the Cache API's cache contexts as it contains all possible variations. The fact that there are (way) too many variations and it may not be worth caching could be handled elsewhere or we could extend the API as suggested with a flag to indicate it is not worth caching.

Besides LanguageBlock I found these candidates where the flag would apply aswell:
- Drupal\book\Plugin\Block\BookNavigationBlock (see https://www.drupal.org/node/2483181)
- Drupal\form_test\Plugin\Block\RedirectFormBlock (see https://www.drupal.org/node/2351015)
- UncacheableDependencyTrait (I suppose this is a special case)

I agree with @mpp, this seems like a great solution. It is really hard to write extra tests for, so I'm going to remove the Need tests tag, see #58 for more information.

Also removing the Needs summary update based on #62.

This is issue is one of the blockers for #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

I think this combination of this and one of the solutions from #2888838: Optimize render caching is good (i.e. what #60 says), but I'm not sure about committing this to a branch when #2888838: Optimize render caching is still pretty far from a committable patch.

So should we postpone this on #2888838: Optimize render caching?

Yes let's do that.

Updated summary accordingly. Lets RTBC this as soon as that issue is also fixed. All others are already committed.

Just an aside comment, this patch wasn't working for me OOTB, and what I had to do is in a custom block extending the LanguageBlock one.

Rather than using the render block, use the getCacheContexts method:

  /**
   * {@inheritdoc}
   */
  public function getCacheContexts() {
    return Cache::mergeContexts(parent::getCacheContexts(), [
      'user.permissions',
      'url.path',
      'url.query_args',
      'languages:language_content',
      'languages:language_interface',
    ]);
  }

And rather than removing the getCacheMaxAge method, override it by:

  /**
   * {@inheritdoc}
   */
  public function getCacheMaxAge() {
    return $this->cacheMaxAge;
  }

You need to include use CacheableDependencyTrait; in your custom block.

This is blocked on #2888838: Optimize render caching curretnly. But im not sure thats needed. Perhaps we can find another solution that allows us to fix this issue. That would unblock #2352009: Bubbling of elements' max-age to the page's headers and the page cache more, and i feel like the whole render cache issue is a bit to big and complicated.

Many years later, there's actually progress here.

#3500683: Allow access policies to opt out of caching introduced a CacheoOptionalInterface, which is very close to what I've imagined back in 2016.

Implemented that and made BlockViewBuilder respect that.

The result of that is interesting.

--- a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php
+++ b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php
@@ -51,18 +51,18 @@ public function testFrontPageAuthenticatedWarmCache(): void {

     $expected = [
       'QueryCount' => 4,
-      'CacheGetCount' => 40,
+      'CacheGetCount' => 34,
       'CacheGetCountByBin' => [
-        'config' => 22,
-        'discovery' => 5,
-        'data' => 7,
+        'config' => 18,
+        'discovery' => 4,
+        'data' => 6,
         'bootstrap' => 4,
         'dynamic_page_cache' => 2,
       ],
       'CacheSetCount' => 0,
       'CacheDeleteCount' => 0,
       'CacheTagChecksumCount' => 0,
-      'CacheTagIsValidCount' => 10,
+      'CacheTagIsValidCount' => 9,
       'CacheTagInvalidationCount' => 0,
       'CacheTagLookupQueryCount' => 5,
       'ScriptCount' => 1,

On HEAD, LanguageBlock has a max-age 0 and gets placeholdered. That means on a dynamic page cache hit on umami, we need to load the block + language including translation overrides, block plugins and load routes. With this, the block isn't cached on its own, so we have one cache get less, but it's just part of the dynamic page cache. The downside of this is that we end up with url.query_args cache context in dynamic page cache. That didn't seem to cause any test fails so far, so we don't seem to have explicit test coverage for the cache contexts on dynamic page cache on a multilingual site with language switcher. Earlier attempts in this issue attempted to move thing into javascript. maybe we could just do that with the query string, unsure, or we placeholder just the query string or something like that.

placeholder just the query string

That sounds like a good thing to start with.

It's easier said than done though. We have placeholders for single query string arguments, but this is different. Note that some language switch links can also add a langcode parameter, so it needs to deal with appending to that as well. maybe the whole URL could be a parameter for the placeholder? seems awkward.

It's also something that tends to be heavily customized/themed (icons, shortened languages, all kinds of completely custom designs), there's an alter hook and specific template suggestions for a link template. There's a fairly high chance that we'll break those customizations if we no longer pass in what we do now.

Oh I didn't realise it can have two query strings to individual placeholder and which also aren't guaranteed to exist, that does indeed sound painful.

We can set #create_placeholder explicitly on the whole block, per #3437499: Use placeholdering for more blocks. And then #3493911: Add a CachedPlaceholderStrategy to optimize render cache hits and reduce layout shift from big pipe will mean it's no-longer rendered via big pipe when it's cached. That will keep the cache contexts isolated from the main dynamic_page_cache entry then but it'll still be render cached.

Sounds good, I think it makes sense to wait on those two issues then.

Note: views also adds query_args as cache context to any view due to filters, sort and so on, realized that in the common cache tags issue with the new test. I was recently wondering on a project why dynamic page cache didn't kick in on a project where a bot (GPTBot) sent tens of thousands of requests that only varied in some tracking query string, that would be why). Might have some potential to limit dynamically based on having exposed filters/sorts/pager.

I don't think we need to wait on #3437499: Use placeholdering for more blocks, we can just copy the same pattern from that issue to here for the language switcher block. That issue has taken various twists and turns when I realised what it does and doesn't solve (completely different things to when I opened it), but the code changes are minimal/trivial. We don't necessarily need to wait for the cached placeholder strategy either although it will probably result in some test churn in either direction.

Seems like a good issue to open against views to add that conditionally.

The one out of scope change if it's being suggested it fixes #3349201: Big Pipe, logged-in users and 4xx pages then think test coverage here should be expanded to include that ticket scenario too. And if that's the case we can close the other issue and move over credit.

3349201 also mentions once it's solved #2613044: Requests are pushed onto the request stack twice, popped once would be solved too so curious if this hits 3 issues actually?

If not then think we should revert the one change.

Trying to go back to keeping it cacheable but using the new forced placeholder ability. That kind of has the same benefits to cache, although that's mostly FastChained lookups anyway. twig debug shows that it's pretty fast, but it can get more complex with multiple languages I suppose.

I could also experiment with CacheOptional + placeholder and maybe also skipping big pipe (

@smustgrave: We're not fixing those issues. This just changed the render context/time so we're not longer applied by it. In that mode, it also can't be reverted, because that was the whole point of this change. That said, the new placeholder approach might again change how this works, we'll see, didn't run that test.

Created a second MR that uses a forced placeholder and the CacheOptionalInterface. This makes it kind of similar to HEAD, no impact on performance tests, but unlike max-age 0, this doesn't bubble up and shouldn't break page caching once that looks at that. It could maybe also use #3508299: Support a placeholder strategy denylist for #lazy_builders once that's in, but would require an alter hook I think to set it on the right level.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased.

Leaving at needs work, need to update the issue summary with the options and questions.

Issue summary updated

#3011426: Page cache size is unlimited when arbitrary query parameters are requested is getting more attention recently (maybe more bad crawlers around?), and the dynamic_page_cache not varying by query parameters unless it actually has to is one of the mitigations for that issue. Both varying the dynamic page cache by query params, or introducing a separate cache item that does, would make that particular issue worse for people I think. So for me that leads strongly towards option #c.

I did think of one other possible approach when reading the IS:

What if we made a special placeholder (similar to CSRF placeholder handling) for only the query string and replace only that? Then we could remove the query string cache context but handle it all in PHP still.

This also made me wonder how bad would it be if we dropped the query string entirely - usually when I change language on a site I'm either testing, or it's the first thing I do when I land on the site, not 5 pages into a pager (which might not show the same results anyway). But probably if we did that someone would be relying on it somewhere.

I think the only reason to do that over option #c would be if the language switcher ends up being the only uncacheable element on the page, then it may end up initialising things (like Twig) that otherwise wouldn't be - we saw this when Navigation caching successfully hid an SDC performance issue recently. But also we could open a small follow-up for that and do #c here.

As quickly discussed on slack, changing how the links are build is challenging because it's a pluggable system depending on your language negotiation settings. See \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl::getLanguageSwitchLinks, session/cookie based language adds its own query parameter that it then uses on the given request to put it back into a cookie. It's not impossible but tricky with BC and relying on this happening and so on.

FWIW, I'm reasonably certain that building this is faster than fetching from cache, based on renderering time debug output (had to hack it to show times also for non-cacheable elements), it's also the same right now in HEAD. But there might indeed be edge case such as it being the only thing it has to render with twig.

I also verified that this fixes OpenTelemetryFrontPagePerformanceTest.php with #2352009: Bubbling of elements' max-age to the page's headers and the page cache, which currently fails as this block prevents it from being a page cache hit.

It's also no change compared to HEAD. The block is currently being autoplaceholdered and not cached as well, just also in a way that that information up.

I also created #3516051: Add CacheOptionalInterface to more blocks as a follow and want to add a change record for this.

Attempt at a new title

1 small nit on MR11571.

And this is a question to make sure I understand the render system correctly, not really a concern:

The language switcher block is not cached on its own, but it is placeholdered with the appropriate cache metadata. Assuming the block is placed globally within a region, because of the placeholdering, the cache contexts do not bubble to dynamic page cache entry, and the cached markup in the dynamic page cache entry contains the non-replaced placeholder markup.

But if the language switcher blocked were placed in a node layout via layout builder, or rendered inside another block template with twig_tweak or similar, then the node or wrapping block could be rendered cached with all the variations for query strings, etc.? The difference being that in HEAD, with max-age 0 being bubbled, the node or wrapping block would not be cached.

@godotislate #107 is exactly right, however there's an existing layout builder issue to address this, see #3244755: Not possible to use render placeholdering with Layout Builder blocks. I've also opened an equivalent issue for experience builder here more recently: #3518656: Add Dynamic Page Cache and BigPipe support: `block` ComponentSource does not use placeholders/#lazy_builder - so for me these are bugs in those modules not fully supporting the block plugin API.

@catch Thanks for those issue links. And yeah, my question is not a blocker here.

One thing about this issue, though, is that maybe there should be at least 1 if not 2 tests?

1. A test that any block with a plugin implementing CacheOptionalInterface is not render cached
2. A test that a page with the language switcher block on it will be cached in the dynamic page cache

The first one at least since an API change is being made to BlockViewBuilder.

Added the first test mentioned in #109.

Thanks for 1, makes sense, I added one small note on that.

For 2, it's not 100% explicit on the intent, but we do have performance tests that explicitly assert the page cache and dynamic cache page on umami, which is multilingual with language switcher.

We no longer have any changes in those tests because the behavior for them is unchanged in regards to the language switcher block, but earlier merge requests had changes there.

Made change to test based on feedback. Also, I agree that "A test that a page with the language switcher block on it will be cached in the dynamic page cache" probably doesn' t need an explicit test.

Quoting option B:

B) Use the new \Drupal\Core\Block\BlockPluginInterface::createPlaceholder() to force this to be a placeholder and cache it separately. In our projects, we're seeing 10k+ variants of this block, very rarely reused. That's MR !9798.

However, that MR seems to also involve CacheOptionalInterface? And I'm not sure we want to be using that here.

My reasoning is this: CacheOptionalInterface was introduced with access policies, where multiple smaller pieces will be cached together as a whole. In that context the interface meant: "If all of the smaller pieces indicate they are insignificant, we need not cache the whole."

However, render arrays with cache keys (including the language switcher block) aren't necessarily like that. They get cached individually and as part of a whole (DPC, when not placeholdered). So using the interface there seems ambiguous as it's not clear in which caching layer they would be considered optional.

Now, with that in mind, LanguageBlock should already be auto-placeholdered by virtue of its max age being 0 and BlockViewBuilder making it use a lazy builder. So setting createPlaceholder() should effectively change nothing about the current situation. The block will be placeholdered and DPC will not cache it.

So it seems the goal is to rid LanguageBlock if its max age of 0 and then make sure it doesn't poison the page. That part is not fully clear from the issue title and summary.

In that case, option B seems like the most straightforward fix, but we would indeed not want to individually cache all variations of LanguageBlock because it varies too much. So removing it's max-age 0 is a good thing, as it would otherwise kill all page caching when combined with #2352009: Bubbling of elements' max-age to the page's headers and the page cache.

Then again, if we merely want to placeholder it and we don't want the placeholdered HTML to be cached, we have more options than to resort to max age 0. I'm wondering if this was ever explored. Let's look at renderer.config:

  renderer.config:
    required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']
    auto_placeholder_conditions:
      max-age: 0
      contexts: ['session', 'user']
      tags: []
    debug: false

As you can see, we could achieve the very same result of having LanguageBlock auto-placeholdered by providing a cache tag that indicates the desired behavior. Much like the special "CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:" cache tag we have. This tag would also bubble up to FinishResponseSubscriber, but it would have zero effect on the max-age of the page.

So rather than use CacheOptionalInterface in a way that might confuse people, why don't we introduce a "SHOULD_AUTO_PLACEHOLDER_BUT_NOT_CACHE" cache tag, add it to LanguageBlock and renderer.config.auto_placeholder_conditions.tags?

Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag and we'd have a system that works for ALL render arrays (with a lazy builder) rather than having to figure out ways to apply an interface to them.

Just adding this: I like how clean the current MR looks and I can see the reasoning behind it. But flagging the LanguageBlock as "cache optional" does not cover the load here. Because we want it to not be cached, ever. So it's hardly optional. Furthermore, we also want to placeholder it. And I can see that (placeholder & no cache) being the case for more pieces of content in the future.

Hence my suggestion for something that would both indicate we do not want to cache it and we want to placeholder it.

The autoplaceholder config is config. We can change the defaults, but it wouldn't really apply to all existing sites that have customized this. IMHO that makes this very hard to control and those sites will then have negative effects from not having that.

I also think that cache tag bubbling is confusing with that special cache tag. I'm not exactly sure about exact behavior, but it seems unpredictable. As we learned recently, before #3512762: Optimize placeholder retrieval from cache, placeholdered elements were still included if their element was already render cached, there might be other unexpected side effects.

Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.

It might not have been clear in the issue where we added it, but I already had this issue in the back of my head to use it here as well, that's why I moved it to the cache namespace and made it more generic. We should extend the docs to mention block plugins as well. In my mind, it's less about supporting CacheOptionalInterface in render arrays (we don't), it's supported specifically for block plugins and makes them not cacheable in the initial definition, but that's an implementation detail of BlockViewBuilder. Other places that support block plugins, such as layout builder, twig or block_field do not currently support per-block caching or placeholdering at all. Which, to reply to #107 as well, is IMHO OK, because it says "cache optional", not "must not be cached". In other words, "not worth caching on its own", which I think fits the use case in access policies.

Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.

Which is why I added:

Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag

But aside from that detail, I agree that making this rely on config might not be such a great idea. Your further clarifications I also tend to agree with. The suggested implementation here is one of "do not cache", but it doesn't mean that other implementations have to go down that same path.

Okay in that spirit I'll go over the MR again, but I would cautiously agree with the approach. We need to be very aware of #108, though. Ideally those issues get fixed sooner rather than later so the changes introduced here don't run havoc there, even if it's technically not "our fault" if it does.

Two remarks, but I don't see why this MR can't be RTBC once they are discussed/resolved.

Leaving at NR as there are no actionable items, yet. Depends on where the discussion leads us.

I think this is at NW based on latest comments.

I don't pretend to understand the nuances of this issue, but in case some real-word experiences are useful, here is our situation: We are using the twig_tweak module to render the language block inside of Twig templates, like this:

{{ drupal_block('language_block:language_interface', wrapper=false) }}

We are doing this on a node type that uses a formatted text field. Every time we view a node of this type, all of the input filters for the text format process. Ie, it's re-processing the formatted text content on every node load. Since we have input filters that parse HTML and strip certain tags, this is pretty hard on the server.

We have tried applying the MR 11571 as a patch, but the result is that we get this message instead of the language block content:

"This block is broken or missing. You may be missing content or you might need to install the original module."

I'm happy to test anything and report back if that would be helpful.

We have tried applying the MR 11571 as a patch, but the result is that we get this message instead of the language block content:

At the very least you'd need to have the commit for #3500683: Allow access policies to opt out of caching, which introduces CacheOptionalInterface, for the MR diff here to work correctly. It won't be available in any stable release until 10.5.0, 11.2.0, or the next release of 11.1.x.

Thanks for that info! After applying the other patch as well, this is working like a charm. The input filters are no longer getting processed on every page load, and the language block appears to be rendering correctly in a variety of scenarios (logged in, anonymous, different languages).

@brockfanning: So people are in fact doing what we feared they would, "interesting". Note that this improves it quite a bit, but it will still cause cache variations for every query string variation, which could be an issue with random things like utm/social media identifiers like fclid from facebook. And this will bubble up into the dynamic page cache. How much of an issue that is for you I can't say.

The neat part about how it works for regular blocks is that it's combined with the also new createPlaceholder feature, see https://www.drupal.org/node/3512518. What this means is that it just adds a placeholder and then actual language switcher links will then be rendered later and the wrapping element (node or dynamic page cache element) doesn't need to vary by those things. I'd recommend creating a twig tweaks issue for this, it might be possible to support this there as well and then placeholder the block there as well. No idea how feasible it is as it obviously doesn't operate on render arrays but within twig rendering.

I went ahead and created #3528854 in the twig_tweak queue.

We did notice an issue come up, even with these patches, via our automated tests. It seems that if we translate a node into a new language, this this doesn't purge the cache for the language block. I'm not sure if this is core behavior, but our language block only shows links for those languages that have saved translations for the node. So for example, if I add a Spanish translation, I'm not seeing a link to "Espanol" on the default language. If I do a cache clear then that link to "Espanol" appears.

Since it sounds like twig_tweak is not recommended for the language block, we're going to try to move our language block into the regular block layout. I've confirmed that this resolves the uncached-text-format problem I described above.

Addressed the outstanding MR comments. I think the cache contexts for the LanguageBlock might not be as complex as thought, unless I'm missing something, but I also think it's fine to handle in a follow up if not adequately addressed.

Tagging "Needs change record" to document the use of CacheOptionalInterface when implementing block plugins. Along the lines of MR comment from catch:

Agreed that documenting this is a good idea, but yeah the idea here is that:

• cache optional and placeholdered blocks are still cached by the internal page cache or proxies (so not max-age 0), but not anywhere else.
• cache optional blocks that aren't placeholders are still cached by dynamic page cache, just not individually - when we think the cost of rendering them directly on a dynamic render cache miss might be less than cache retrieval.

@berdir wonder if you have a second to look at the responses on the open threads?

Added the CR: https://www.drupal.org/node/3554070. It's a bit verbose, so any edits, suggestions, or corrections are welcome!

Made some minor edits to the CR.

I think the code generally looks good. I'm not sure the cache optional name exactly describes what is happening here, but this is what it is for.

There was this note in A that I'm not sure about: language switcher blocks includes the current query string, so that would always bubble up to dynamic page cache as a cache context..

The issue summary could be updated with just the selected path, I think C makes the most sense and the MR looks good.

There was this note in A that I'm not sure about: language switcher blocks includes the current query string, so that would always bubble up to dynamic page cache as a cache context.

That applies when the block is not placeholdered.

I have updated the IS to show the consolidation to what was formerly option "C", which combines the use of CacheOptionalInterface and enforced placeholdering.

Issue summary is clearer thanks!
CR is a bit complex but good.

Code looks good and all comments have been addressed.

Read through the comments to and the direction change seems reasonable

Edit, took a pass at credit, this has quite the history so apologies if I missed someone.

This block makes things a little bit complicated for #3508508: Allow big pipe to run for session-less users, but that was also true for when it was max-age: 0, so not introduced here.

MR looks really good now and this should help cache hit rates quite a lot.

Committed/pushed to 11.x, thanks!

Forgot that was already did #3554414: Use #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe several months ago specifically for that kind of thing, so opened a follow-up to use that too.