Make language switcher block cacheable

Created on 3 April 2014, about 11 years ago
Updated 13 February 2023, about 2 years ago

Postponed on #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE โ†’ and ๐ŸŒฑ Optimize render caching Needs work .

Problem/Motivation

The language switcher block is currently not cacheable at all.

Proposed resolution

If we have a "language redirect" route, i.e. /language-redirect/<language code>, then e.g. "franรงais" would link to /language-redirect/fr?destination=node/42. Then everything except the destination parameter could be cached globally. The destination parameter could be set in JS, just like contextual.js already does today:

    // Set the destination parameter on each of the contextual links.
    var destination = 'destination=' + Drupal.encodePath(drupalSettings.path.currentPath);
    $contextual.find('.contextual-links a').each(function () {
      var url = this.getAttribute('href');
      var glue = (url.indexOf('?') === -1) ? '?' : '&';
      this.setAttribute('href', url + glue + destination);
    });

To remain JS-less for anonymous users, we could still generate the entire block dynamically for anonymous users, because Drupal assumes that anonymous users get served cached pages anyway. This would then be in line with what we do for active link handling: we set the active class in PHP (on the server side) for anonymous users, but in JS (on the client side) for authenticated users.

That's the best of both worlds.

Remaining tasks

Blocked on #2107427: Regression: Language names should display in their native names in the language switcher block โ†’ and #2335661: Outbound path & route processors must specify cacheability metadata โ†’ and ๐ŸŒฑ Optimize render caching Needs work .

User interface changes

None.

API changes

None.

๐Ÿ“Œ Task
Status

Postponed

Version

10.1 โœจ

Component
Language system  โ†’

Last updated 1 day ago

  • Maintained by
  • ๐Ÿ‡ฉ๐Ÿ‡ชGermany @sun
Created by

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Live updates comments and jobs are added and updated live.
  • D8MI

    (Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

  • Performance

    It affects performance. It is often combined with the Needs profiling tag.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain pcambra Asturies

    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.

  • last update almost 2 years ago
    Composer error. Unable to continue.
  • last update over 1 year ago
    30,335 pass, 1 fail
  • Status changed to Active 6 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands bbrala Netherlands

    This is blocked on ๐ŸŒฑ Optimize render caching Needs work curretnly. But im not sure thats needed. Perhaps we can find another solution that allows us to fix this issue. That would unblock ๐Ÿ› [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed more, and i feel like the whole render cache issue is a bit to big and complicated.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia samit.310@gmail.com

    samit.310@gmail.com โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    6 months ago
    Total: 1220s
    #305885
  • Pipeline finished with Success
    6 months ago
    Total: 409s
    #305937
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    Many years later, there's actually progress here.

    ๐Ÿ“Œ Review cache bin and cache tags of access policy caching Active 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.

  • Pipeline finished with Success
    2 months ago
    Total: 901s
    #414100
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    placeholder just the query string

    That sounds like a good thing to start with.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    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 ๐Ÿ“Œ Create placeholders for more things Active . And then ๐Ÿ“Œ Render the navigation toolbar in a placeholder Active 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.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I don't think we need to wait on ๐Ÿ“Œ Create placeholders for more things Active , 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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    The one out of scope change if it's being suggested it fixes ๐Ÿ› Big Pipe, logged-in users and 4xx pages Active 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 ๐Ÿ› Requests are pushed onto the request stack twice, popped once Needs work would be solved too so curious if this hits 3 issues actually?

  • Pipeline finished with Failed
    15 days ago
    Total: 151s
    #454404
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • Pipeline finished with Failed
    15 days ago
    Total: 531s
    #454410
  • Pipeline finished with Canceled
    15 days ago
    Total: 425s
    #454421
  • Pipeline finished with Success
    15 days ago
    Total: 362s
    #454425
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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 ๐Ÿ“Œ Allow the messages block to skip placeholdering Active once that's in, but would require an alter hook I think to set it on the right level.

  • Pipeline finished with Failed
    14 days ago
    Total: 536s
    #454798
  • Pipeline finished with Success
    13 days ago
    Total: 646s
    #455341
  • 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.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    Rebased.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

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

  • Pipeline finished with Success
    9 days ago
    Total: 441s
    #459085
  • Pipeline finished with Success
    9 days ago
    Total: 452s
    #459088
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    Issue summary updated

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    ๐Ÿ› Page cache creates vast amounts of unneeded cache data Active 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.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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 ๐Ÿ› [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed , 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 ๐Ÿ“Œ Add CacheOptionalInterface to more blocks Needs review as a follow and want to add a change record for this.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    Attempt at a new title

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    berdir โ†’ changed the visibility of the branch 2232375-make-language-switcher to hidden.

  • Pipeline finished with Success
    5 days ago
    Total: 6427s
    #461893
Production build 0.71.5 2024