Blocking core issue: #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system


mkalkbrenner’s picture

Issue summary:View changes

Updated issue summary.

mkalkbrenner’s picture

Issue summary:View changes


mkalkbrenner’s picture

Issue summary:View changes

added blocker

mkalkbrenner’s picture

mkalkbrenner’s picture

'theme callback' and hook_custom_theme() got replaced by theme negotiators

markcarver’s picture

mkalkbrenner’s picture

We need to figure out how to deal with cache tags and cache keys. Do we need to set or invalidate something when a rule matches, for example based on the user agent?
How do we deal with #lazy_builders, placeholders and big pipe?
We have to ensure that a lazy builder uses the same theme that was used to generate the page that contains the placeholder.

mkalkbrenner’s picture

Version:7.x-3.x-dev» 8.x-1.x-dev

Hi Markus,

Sorry for the delay.

today we brainstormed about the D8 port of ThemeKey:
Basically it triggers a selection of a theme based on a rule chain.

Okay :) Interesting. This screams ‘cache contexts’, but more about that later.

Now we wonder how these lazy builder stuff and big pipe will deal with that?

If ThemeKey decides to use a theme render a page it will consist of various
placeholders, right?

Big pipe will then replace these placeholders by calling the corresponding
lazy builder callback url. For this call we have to ensure to things:
1. The theme negotiator of ThemeKey must be bypassed. How can we identify
these requests?
2. Another theme negotiator in core must ensure that the same theme is used
again as was used to originally render the page. Does such a theme negotiator
already exist in core?

BigPipe works in the *same* request. Given that there are not multiple requests going on, ThemeKey’s negotiated theme will continue to be active.

It’s only once you start using ESI that this becomes potentially tricky, depending on how ESI is implemented. But let’s address that in the future :)

In addition I wonder if and how we have to manually have to deal with cache
keys or tags when ThemeKey selects a theme, for example based the day of the
week. In D7 we implemented hook_cron() to figure out if the rule chain
consists of "time-based" properties and cleared the page cache if such a rule
toggles to a new result.

If a theme is selected based on part of the request context (path, language, IP address, country …), then it needs to set a matching cache context:

If a theme is selected based purely on the day of the week, then it needs to set a matching max-age: (Assuming this is determined from the POV of the server, not from the user; if it also takes into account the user’s timezone, then you’ll also need the ‘timezone’ cache context.)

If the “rule chain” is modified and you want changes to take effect immediately, you will also need to associate the cache tag for the config object that contains this rule chain.

In other words: it looks like you’ll need cache contexts, max-age and tags: i.e. all bits of cacheability metadata.

The question is: *where* to associate this? All your logic must run in a ThemeNegotiatorInterface. So that would also be the natural place to apply the corresponding cacheability metadata to the response, right? Except, at that time, there’s no Response object yet!
For cases like this, we use request attributes. There is a very similar example in core: access checking performed during routing. An access check result also has cacheability metadata. We need that to be associated with the Response object, but it doesn’t exist yet. So what the AccesssAwareRouter does is store an “_access_result” attribute on the request that contains the access result object (which includes cacheability metadata). Then there is a RouteAccessResponseSubscriber that applies the cacheability metadata to the Response object when the response is ready.
The final challenge is that theme negotiators do not get the Request object by default. But you can just inject the RequestStack and get the current request, then modify it. An example of that can be found in the BatchNegotiator theme negotiator.

I think/hope this answers all your questions :)


Wim Leers’s picture

Now also following this issue :)

Wim Leers’s picture

In e-mail, you replied to #5:

Even if I understand what I will have to implement, I’m convinced that Drupal 8 raises the barrier for new developers a lot. I wonder if we’ll see a lot of erroneous contrib modules in the future :-(

To which my answer is this.

That’s not really true.

Drupal 7 just ignored tons of dependencies, it used lots of globals. It was very easy to cache things incorrectly. Consequently, the majority of Drupal 7 sites disable all caching altogether. Because once they enable caching, they’re broken.

Drupal 8 just strongly encourages you to think about dependencies, so that it *can* cache things. See for a more comprehensive explanation.

You could make the ThemeKey module ignore caching or let it continue to break caching like in Drupal 7.

Interestingly, the project page says this:

And unlike other theme switching modules, ThemeKey should play well with internal and external page caches, like Boost or Varnish, even for anonymous users.

So I took a look at the 7.x-3.x branch. It has HOOK_themekey_properties(), which is essentially *exactly* what cache contexts are. For each, you have to state either THEMEKEY_PAGECACHE_SUPPORTED, THEMEKEY_PAGECACHE_UNSUPPORTED or THEMEKEY_PAGECACHE_TIMEBASED. The first two basically mean “this is determined solely based on the URL, or a subset of it”. So, that’s what the “url.*” cache contexts are for. You don’t need the first two anymore. The latter is to let it affect the max-age. That’s what cache max-age is for.

In other words: instead of modules like ThemeKey having to invent their own concepts, abstractions and implementations, they can now be expressed using the three concepts that Drupal 8 core provides. So, yes, *you* as a module developer will have to learn these concepts, but it means that users of your module don’t have to learn the slightly different concepts in different modules anymore, and will greatly simplify/improve the debuggability of cached things.