Make language switcher block cacheable

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: [pp-3] 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: [pp-3] 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: [pp-3] 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.