Problem/Motivation

ThemeManager has a setActiveTheme() method to switch the currently active theme.

That is not fully working because the theme.registry service keeps the registry information in $this->registry/$this->runtimeRegistry and doesn't support varying the active them.

This makes something like a mail theme so that mails always use the same theme no matter which theme the user is currently using (e.g. admin vs. frontend theme) mucher harder than it should be. See #2498265: Reimplement mail theme functionality. I need to register a duplicate service so I can switch out the whole service, for which I can't rely on the interface.

Proposed resolution

Change $this->registry to an array, keyed by active theme name.

There's also some weird code in the Registry class, like calling init() on every call which checks a $this->inititialized property that is never set to TRUE. I'm also not sure what the code there is supposed to do, really. The theme name argument of the constructor is only used in unit tests, it doesn't seem to be the job of the registry to initialize it, ThemeManager does that. But I guess we should clean that up in a separate issue.

Remaining tasks

User interface changes

None.

API changes

None, setActiveTheme() works as expected.

Data model changes

CommentFileSizeAuthor
#13 theme-registry-2640962-13-interdiff.txt4.17 KBberdir
#13 theme-registry-2640962-13.patch12.46 KBberdir
#12 theme-registry-2640962-11-interdiff.txt5.23 KBberdir
#12 theme-registry-2640962-11.patch9.35 KBberdir
#5 theme-registry-2640962-4-interdiff.txt561 bytesberdir
#5 theme-registry-2640962-4.patch4.79 KBberdir
#2 theme-registry-2640962-2.patch4.69 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new theme-registry-2640962-2.patch4.69 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Needs tests

Status: Needs review » Needs work

The last submitted patch, 2: theme-registry-2640962-2.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new theme-registry-2640962-4.patch4.79 KB
new theme-registry-2640962-4-interdiff.txt561 bytes

Missed on, another example of the weird code in there, build() already sets $this->registry.

mbaynton’s picture

mbaynton
he/him
Location Minneapolis, US
commented
Related issues: +#2621018: Support rendering from a manually specified theme

@Berdir encountered the same issue that prompted my #2621018: Support rendering from a manually specified theme. I believe these two issues are alternative attempts at solving the same problem.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Oh, I forgot about your issue, sorry.

I think my change here is more contained, but lets wait for you actually testing my mailsystem patch, since I didn't really test it apart from some debugging :)

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -79,7 +80,7 @@ class Registry implements DestructableInterface {
    */
-  protected $registry;
+  protected $registry = [];

    So what this patch is basically doing is to make the theme registry less statefull, which is great, IMHO! 

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -719,12 +720,14 @@ protected function postProcessExtension(array &$cache, ActiveTheme $theme) {
+    foreach ($this->runtimeRegistry as $runtime_registry) {
+      if ($runtime_registry instanceof ThemeRegistry) {
+        $runtime_registry->clear();
+      }
     }

    What else could be stored in there? I don't get the instanceof here, to be honest

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I agree with #8.2 as well because we don't check it in other cases like destruct expects a destruct() method. The setter likely just needs to be typehinted if there is one otherwise yeah that can be removed.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

As I said, the code there is very inconsistent. There is an instanceof check in the old line of reset() too. Happy to remove that everywhere.

dawehner’s picture

dawehner
Primary language German
commented

Yeah IMHO we should just remove that

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -Needs tests
StatusFileSize
new theme-registry-2640962-11.patch9.35 KB
new theme-registry-2640962-11-interdiff.txt5.23 KB

Fair enough.

Updated RegistryTest, there was a whole bunch of methods overridden that we no longer need, in some cases even don't exist anymore. We can actually rely on returning the active theme from the theme manager now and don't need to inject it anymore, with a second code path that nothing but the unit test uses, except not really :)

We could remove that completely I guess but that seems like too much change for this issue and 8.0.x.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new theme-registry-2640962-13.patch12.46 KB
new theme-registry-2640962-13-interdiff.txt4.17 KB

For fun, this is how much dead code is in Registry now that was only half-used by the unit test :)

As I said, we might not want to do this in 8.0.x.

Status: Needs review » Needs work

The last submitted patch, 13: theme-registry-2640962-13.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Ok, there are two more tests that apparently use it ;)

As I said, probably not viable for 8.0.x anyway. Lets continue with the patch in #12.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

This is a nice and IMHO precise solution for the problem. Alternative could be of course to provide basically a Registry stack, but that seems to be too much at that given point.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -79,7 +80,7 @@ class Registry implements DestructableInterface {
-  protected $registry;
+  protected $registry = [];

@@ -96,11 +97,11 @@ class Registry implements DestructableInterface {
-  protected $runtimeRegistry;
+  protected $runtimeRegistry = [];

I wonder what this means for anything extending from this class to provide an alternate implementation. Going to ping to the other maintainers to get an opinion of bc policy and this change.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I thought about that. We *could* add separate properties and also set those to the last requested registries.. but I'm not sure if that would really help.

If someone has actually replaced that service, then what they're doing might be incompatible anyway.

That said, I doubt that's a service that's actually replaced by anyone (I mean, I didn't, so... :p). There's no storage to replace, there's no interface...

The last submitted patch, 12: theme-registry-2640962-11.patch, failed testing.

The last submitted patch, 12: theme-registry-2640962-11.patch, failed testing.

  • catch committed 16d9e20 on 8.1.x 
    Issue #2640962 by Berdir: Theme Registry does not support switching the...
catch’s picture

catch
he/him
Primary language English
commented

Committed/pushed #12 to 8.1.x, thanks!

I think the @internal changes are small enough, and the bug reproducible enough that it's worth doing this in 8.0.x, but leaving RTBC there a bit longer.

  • catch committed 08938d7 on 
    Issue #2640962 by Berdir: Theme Registry does not support switching the...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Cherry picked to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.