Refactor ThemeRegistry and Theme\Registry

Created on 30 March 2018, over 6 years ago

Updated 18 March 2023, over 1 year ago

Background / current situation

There are two classes in Drupal 8 core which deal with the theme registry:

Drupal\Core\Theme\Registry
Drupal\Core\Utility\ThemeRegistry - the "runtime" theme registry.

The latter, ThemeRegistry, is created in Drupal\Core\Theme\Registry::getRuntime(), and only there.

On the other hand, ThemeRegistry calls Theme\Registry->get() on cache miss.
To do this, it gets the Theme\Registry object via Drupal::service(), instead of having it injected.
Among other problems, this causes this flaw: 📌 ThemeRegistry::resolveCacheMiss() might look at wrong theme in some cases Active

Theme\Registry holds registry arrays for (potentially) all themes.
However, it also has a Registry->themeName and Registry->theme.
This means some of its functionality is for one specific theme, other functionality is for any theme.

The "specific theme" for Theme\Registry is set in the constructor, but it can be modified via ->init($theme_name).

So, in a way, the kind of hypermutable or shapeshifting object that is typical for Drupal.

ThemeRegistry (the "runtime" registry) inherits from CacheCollector, and overwrites half of the methods.
It also inherits CacheCollector::set() and CacheCollector::delete(), which are never called on ThemeRegistry objects (from what I found).
All non-public members of CacheCollector are protected, which means they leak into all inheritors. Protected properties of CacheCollector can be and are read and written in ThemeRegistry methods, which means you really need to look at both classes (parent and child) to see what is going on.

I get the idea that inheriting from CacheCollector and implementing CacheCollectorInterface makes things more complicated rather than simpler.

Proposed change

I am not sure yet, but here are some ideas.
In the spirit of #2956549: Policy: Atomic behavior objects → .

Introduce a ThemeRegistryInterface, with methods ->has($key) and ->get($key).

All relevant implementations should be for one specific theme, which is chosen in the constructor and then not changed again.
If you need the same thing for a different theme, you create a new one.

Perhaps there can be one toplevel implementation with a mutable $this->themeName property, which maps all calls to $this->registries[$this->themeName]->FOO();. This will be necessary, if we want to register this thing as a service. Because services cannot be "thrown away" during a request, if the active theme name changes.

The following implementations and other classes could be useful, which would be combined as layers in a decorator stack:

ThemeRegistryDiscovery.
I am not even sure if this should implement the same interface. Because here we don't need ->has($key) and ->get($key), but rather a -->getAll() method.
ThemeRegistryCache
A decorator which caches the full registry for the specified theme, with $cid = 'theme_registry:' . $this->themeName.
ThemeRegistryRuntimeCache
The runtime registry, which only remembers those theme hooks which are actually used.
ThemeRegistryProxy
This can lazy-create another ThemeRegistry implementation with heavy dependencies or initialization logic, and delegate all methods onto this lazy-created proxy instance.
I am not sure yet at which layer we would use a proxy like this.
ThemeRegistryDispatcher
This does NOT implement ThemeRegistryInterface. It has a method ->getForTheme($theme_name) which returns a ThemeRegistryInterface instance for the specified theme.
ThemeRegistryCurrent
This implements ThemeRegistryInterface, has a mutable ->currentThemeName variable, and an ->dispatcher object, and maps every call FOO() to $this->dispatcher->getForTheme($this->currentThemeName)->FOO().
Or, maybe smarter: It subscribes to a ThemeChangeEvent, which causes it to change the $this->currentRegistry. Every call ->FOO() is then delegated to $this->currentRegistry->FOO().
This last one can be registered as a service.

This still leaves some open questions. I simply have not thought about it long enough.

We need some objects that return a full array of theme implementations, and other objects which return specific entries for specific keys. So maybe we need two interfaces: ThemeRegistryAllInterface and ThemeRegistryLookupInterface (names open for debate).

I think trying to implement this would bring clarity.

Why not plugins?

See #2869859: [PP-1] Refactor theme hooks/registry into plugin managers →

A lot of the above looks like functionality that already exists for the plugin system.
But the plugin system might also introduce unnecessary clutter and overhead, and some functionality might not be suitable for the theme registry.

We could try this with and without the plugin system, and then see which looks and works better.

Theme hooks as objects?

Currently, theme registry entries are arrays.

it was proposed in 📌 Convert theme hooks (defined by hook_theme()) to be objects Needs work to replace those arrays with objects.
I am personally skeptical about this.
It would mean that we pass mutable objects to various places where they can be modified (but shouldn't), and also serialize those objects into the database.

If we do something like this, or something similar, then the ThemeRegistryInterface would have to be modified to return those objects instead of arrays.
The basic implementation would not change.

What about BC?

The existing class Drupal\Core\Theme\Registry is marked as "@internal", so in theory we are allowed to mess with it.
Unfortunately, this does not stop some contrib modules from accessing it. E.g. devel via theme_get_registry().

We could turn the class into a shallow adapter for BC support.

The Drupal\Core\Utility\ThemeRegistry is not marked as "@internal", but is (afaik) only used in a few places in core.
So, can we modify this?
And are we allowed to strip interfaces from this class?
Those places that I found do not depend on ThemeRegistry being an implementation of CacheCollectorInterface. But can we rely on that?

📌 Task

Status

Active

Version

10.1 ✨

Component

Theme →

Last updated 1 day ago