Postponed on #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE and #2888838: Optimize render caching.
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 #2888838: Optimize render caching.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#56 | 2232375-54.patch | 796 bytes | Berdir |
#54 | 2232375-54.patch | 723 bytes | Berdir |
#53 | 2232375-53.patch | 884 bytes | mErilainen |
#32 | 2232375-cache-block-switcher-32.patch | 14.42 KB | penyaskito |
Comments
Comment #1
Wim LeersHere's a very rough initial version that shows the direction this patch should take.
Comment #2
Gábor HojtsyThe block cache tags should also depend on language of the interface which was used to render the block.
Comment #3
Gábor HojtsyIs this not same / similar to #2318437: Replace the hardcoded langcode key on blocks with the 'language' cache context?
Comment #4
Gábor HojtsyTo 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.
Comment #5
Berdir@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.
Comment #6
Gábor HojtsyHa, right. I guess caching per language per page is not that much values / too many cache entries.
Comment #7
Wim Leers#6: Exactly!
Once #2107427: Regression: Language names should display in their native names in the language switcher block lands, I'll finish what I started in #1.
Comment #8
Gábor HojtsyYay, #2107427: Regression: Language names should display in their native names in the language switcher block now landed, this may be continued.
Comment #9
Gábor HojtsyComment #10
kfritscheComment #11
Wim LeersI strongly recommend against working on this. To fix this, we'll need outbound route processors and outbound path processors to return cacheability metadata. See #2335661: Outbound path & route processors must specify cacheability metadata.
You can probably already fix part of the problem, but definitely not the entire problem. At least not with full reliability/correctness.
Comment #12
kfritscheComment #13
kfritschePosting 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.
Comment #14
Gábor HojtsyComment #15
Alan D. CreditAttribution: Alan D. commentedUpdated the summary as per comment #11
Comment #16
Wim LeersBlocked on #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance in general, and specifically on #2448823: Add languages:<type> cache contexts.
Comment #17
Wim Leers#2448823: Add languages:<type> cache contexts got marked as a duplicate of #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE, so blocked on that instead now.
Comment #18
Gábor HojtsyPut that at the top of the summary.
Comment #19
Wim Leers#2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE landed, unblocking this.
Comment #20
penyaskitoI'm going to work on this @ drupaldevdays Montpellier
Comment #21
Wim Leers@penyaskito and I just discussed this. Thanks so much for taking this on btw, @penyaskito! :)
We concluded that the best approach was to apply the same strategy as the one we use for active links:
#post_render_cache
callback to insert the current path (see\Drupal\Core\Path\CurrentPathStack
) in the redirect linksComment #22
penyaskitoUploading 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.
Comment #24
Wim LeersThis is definitely on the right track, great job! :)
A few nitpicks, but also a few actual bugs:
Oh, darn, sorry.We actually already have a cache context for this.
See https://www.drupal.org/developing/api/8/cache/contexts.
Instead of
user.is_anonymous
, you want to use theuser.roles:anonymous
cache context.The use of
str_replace()
is fine.But the placeholder is not, because it's not dynamic/random. So if anybody writes a blog post about this functionality, for example, then any mention of
'data-language-switch-link-placeholder'
would also be replaced :)Use
drupal_render_cache_generate_placeholder()
/RendererInterface::generateCachePlaceholder()
to generate such a random placeholder.See
\Drupal\comment\CommentPostRenderCache::renderForm()
and\Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::viewElements()
for an example.You injected the current user service, but aren't using it yet :)
All of this information should be gathered in the
#post_render_cache
callback.Because we want to be able to cache this for all authenticated users, for use on any route. The
#post_render_cache
callback is executed on every request by an authenticated user. So we want the information for that request, not for the request that caused the initial version of the language switcher block to be generated.Not per language, but per interface language. Since not very long ago, you can specify that. So not
'languages'
, but'languages:' . LanguageInterface::TYPE_INTERFACE
.See https://www.drupal.org/developing/api/8/cache/contexts.
We have an issue for that :) #2428837: Adding/updating interface translations should invalidate page & render caches, it's currently RTBC.
Comment #25
penyaskitoThanks 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.
Comment #27
Wim Leers6. Good point!
While you're fixing those test failures, some additional feedback:
We can do this by manually re-invoking
Drupal.behaviors.activeLinks()
on the affected elements.Note that we also need to declare a dependency on the library that provides that behavior then.
s/image/language/
These can be simplified to
'language.redirect:setLanguageSwitchDestination'
.Comment #28
penyaskitodata-drupal-link-system-path
so active-links detects it, or do it ourselves in JS for registered +#post_render_cache callback
.Also renamed the controller to
LanguageSwitcherController
, which makes more sense.Comment #30
penyaskitoRerolled.
Comment #32
penyaskitoAnother reroll.
Comment #34
penyaskitoThe @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?
Comment #35
XanoThe 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?Comment #36
Gábor Hojtsy@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?
Comment #37
Gábor HojtsyAlso #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache is removing post render cache.
Comment #38
Xano@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.
Comment #39
Gábor Hojtsy@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.
Comment #40
mgiffordUnassigning stale issue. Hopefully someone else will pursue this.
Comment #44
andypostComment #45
Mirroar CreditAttribution: Mirroar at werk21 commentedSo 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.
Comment #46
Mirroar CreditAttribution: Mirroar at werk21 commentedComment #47
BerdirPage 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.
Comment #49
alexej_d CreditAttribution: alexej_d commentedBTW @Mirroar returning a render array from getCacheMaxAge is invalid.
Comment #50
Mirroar CreditAttribution: Mirroar at werk21 commented@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.
Comment #51
BerdirYes, 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.
Comment #53
mErilainen CreditAttribution: mErilainen at Wunder commentedFixed the patch, but it doesn't seem to help, still getting Cache-Control: must-revalidate, no-cache, private in the response headers.
Comment #54
Berdir@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.
Comment #56
BerdirOops.
Comment #57
claudiu.cristeaNeeds IS update and probably a regression test.
Comment #58
BerdirA 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.
Comment #59
Wim LeersI haven't read the original work/patches from years ago, but #45/#56 seems feasible indeed :) My only question is why the
user.permissions
cache context is in there.Comment #60
Wim LeersLooking back at the original "this direction please"-patch I proposed in #1, the reason I had that extra complexity in there was to allow for fewer variants to be cached.
The patch in #45/#56 allows for many (MANY!) variations to be cached. There's not wrong, it just requires more cache space. But @Berdir is suggesting in #54 that if #2888838-11: Optimize render caching lands, we could just mark this block as being "cacheable but not worth caching". This would allow it to be cached as part of Dynamic Page Cache, but wouldn't cache the individual rendered block. That makes the approach in #45/#56 a lot more appealing of course!
Comment #62
mpp CreditAttribution: mpp at AmeXio for District09 commentedFrom 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)
Comment #63
borisson_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.
Comment #65
catchI 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.
Comment #67
larowlanSo should we postpone this on #2888838: Optimize render caching?
Comment #68
catchYes let's do that.
Comment #69
AnybodyComment #70
AnybodyUpdated summary accordingly. Lets RTBC this as soon as that issue is also fixed. All others are already committed.
Comment #79
pcambraJust 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:And rather than removing the
getCacheMaxAge
method, override it by:You need to include
use CacheableDependencyTrait;
in your custom block.