Allow themes to contain companion modules and depend on them

Patch working on tests atm, I will get them up tomorrow. Note we can't inject due to circular reference in services and registry->int() already deals with the themehandler in same manner

Just a tip: you can easy get around that:

  theme.registry:
    class: Drupal\Core\Theme\Registry
    arguments: ['@app.root', '@cache.default', '@lock', '@module_handler']
    tags:
      - { name: needs_destruction }
    calls:
      - [setThemeManager, ['@theme.manager']]

This is basically what you need to do in order to solve that circular dependency,

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -350,9 +350,9 @@ protected function build() {
-    // Let modules alter the registry.
+    // Let themes and modules alter the registry.
     $this->moduleHandler->alter('theme_registry', $cache);
-    // @todo Do we want to allow themes to take part?
+    \Drupal::theme()->alter('theme_registry', $cache);
 

Inject theme.manager?

Meh... sorry I missed comments above.

@marcingy
Let's at least encapsulate the call to it, like for example ,code>HandlerBase::getModuleHandler() does.

Themes used to be called explicitly for *all* alter hooks by default in D7. I noticed that recently somewhere because there was similar double-call to Themehandler. I was wondering where and why that happened...

Sure, it can be misused for crazy stuff, but I'm worried that we're going to get tons of issues like this about each alter hook that could be useful for themes to override or that they did in 7.x... (where it was announced as a feature)

wtf we lost #591794: Allow themes to alter forms and page and #812016: Themes cannot always participate in drupal_alter() ??

.

I thought we had really broad support for introducing alters on themes? One of the former initiative leads in the original issue https://www.drupal.org/node/591794#comment-2130538 the current branch maintainer in the same https://www.drupal.org/node/591794#comment-2099520 then Moshe https://www.drupal.org/node/591794#comment-2135980 current initiative leads/CSS/theme maintainer JohnAlbin rolled the first RTBC patch https://www.drupal.org/node/591794#comment-2148496 then webchick https://www.drupal.org/node/591794#comment-2197964 approves. OTOH I fail to see a wide discussion in the issue that removed this functionality -- certainly no themers and it seems alexpott didn't even comment on this rather serious change.

Let's first link to the actual issue which changed it, so other people know what we talk about #2228093: Modernize theme initialization

• https://www.drupal.org/node/2228093#comment-9025883 (review point 5)
• https://www.drupal.org/node/2228093#comment-9042449 is also in favour of it
• https://www.drupal.org/node/2228093#comment-9010985

Just to be clear, generic alter hooks for all hooks is really problematic. If you think about it, you need to determine the active theme, in order to do it,
which itself depends on the module system, as you need to load users potentially for example.

fwiw I completely disagree with myself 5 years ago on themes implementing any alter.

There can be multiple themes on a site, and only one can be active at a time, this is very common now with admin themes. So having logic in alter hooks in themes can lead to very unpredictable behaviour. The menu example here is completely broken, because the router did not have a different version per-theme - presumably that theme was only used on a site with a single theme.

I'd rather see us allow themes to ship actual modules as part of the project (since .info.yml structure allows this now, we'd just need to add the directories to discovery I think) - then the module can implement anything not available to the theme.

To be clear, contrib can participate in it, as its not a whitelist but a opt in process.

Cross-posted with Daniel and said very similar things.

Let's just fix the contributed project blocker here, retitling and moving back to task.

I'm fine with a new (major) issue to discuss bringing back the 7.x behaviour or otherwise accommodating some version of it per #22, but that's not blocking any specific theme port at the moment and it's a much bigger change.

Then dropping back to major.

This was from #2024217-7: Should themes be able to implement hooks, e.g. hook_system_info_alter()?:

---

Somehow, this was actually removed by #2228093: Modernize theme initialization.

With the sentiments of the above two comments and now mine, I think it's safe to say that this is actually a regression from drupal_alter().

I'm currently in the process of porting the Bootstrap base-theme and it implements hook_element_info_alter in 7.x, but is never called in 8.x.

The notes from that issue state:

Theme::alter() removes support for themes to participate in all alter hooks from ModuleHandler::alter().

Instead, Theme::alter() will be specifically invoked manually for the few hooks in which themes are supposed to participate. That list is very limited; e.g.:

I'm certainly not disagreeing with this at all, however we really haven't implemented a better alternative IMO. Instead, we're limited to the current "list" which literally consists of just the following:

• hook_css_alter
• hook_form_alter & hook_form_FORM_ID_alter
• hook_js_alter
• hook_js_settings_alter
• hook_page_attachments_alter
• hook_theme_suggestions & hook_theme_suggestions_HOOK_alter

I'm definitely not saying what we had in 7.x was "ideal", but for themes to be able to port easily, I can see this being a huge PITA in the next year or so. We'll likely have a lot of unknowns until 8.x is actually released, production ready and existing sites (themes) are ported.

Ripping this out in 8.x entirely is a horrible idea, maybe we can do that in 8.1.x once we have a more solid list of what is "acceptable" for themes to alter. If not, definitely 9.x.

Surely hook_element_info_alter must be one, it's part of the theme system as it can add/replace callbacks that are invoked on elements using #process and #pre_render. I can only imagine the many others that we'll encounter over time, the theme system is definitely too integrated with this ATM.

---

From second second comment below it:

And even more from menu.api.php which I've altered in themes before:

• hook_menu_local_tasks_alter
• hook_menu_local_actions_alter
• hook_local_tasks_alter
• hook_system_breadcrumb_alter (was hook_menu_breadcrumb_alter in 7.x, I'm guessing)

This is going to be a huge rabbit hole... just saying.

To be clear, I did read this entire issue, btw.

#20:

OTOH I fail to see a wide discussion in the issue that removed this functionality -- certainly no themers and it seems alexpott didn't even comment on this rather serious change.

No, that was definitely pushed through quickly (I vaguely remember it) and definitely wasn't involved (due to work) otherwise I would have raised a HUGE red flag.

#22:

I'd rather see us allow themes to ship actual modules as part of the project (since .info.yml structure allows this now, we'd just need to add the directories to discovery I think) - then the module can implement anything not available to the theme.

Sure, but this is definitely a feature request/nice to have. Would I love to see that make it in, hell yeah! It sure would make a lot of things easier down the road. However, by making this the "solution" for the "alter" problem in themes, this would require them to break out the code into "theme" and "module"... making the porting process even more difficult than it already is.

We need to transition this in 8.x... not remove them. In 9.x, this might not even be an issue.

So for hook_element_info_alter() there seems to be for me a clear usecase which we missed to enable. Let's do that.

Ripping this out in 8.x entirely is a horrible idea, maybe we can do that in 8.1.x once we have a more solid list of what is "acceptable" for themes to alter. If not, definitely 9.x.

The API change has to happen now and actually happened a long time ago.

No, that was definitely pushed through quickly (I vaguely remember it) and definitely wasn't involved (due to work) otherwise I would have raised a HUGE red flag.

Its kinda funny, but from my point of view, people agreed with it for a long long time. Yes, this was probably a biased observation.

hook_menu_local_tasks_alter
hook_menu_local_actions_alter
hook_local_tasks_alter
hook_system_breadcrumb_alter (was hook_menu_breadcrumb_alter in 7.x, I'm guessing)

Can you give examples, why you need to alter them? For me, these hooks are kinda like logic hooks, which should, from my point of view, not be part of a theme, like written from catch earlier.
I would rather allow dependencies of themes onto modules, which then can reliably alter things, than having a sorta working system for altering everything in themes.

As mentioned above, theme alter depends on *the currently active theme*. So it only makes sense to allow to alter things which are either not cached or cached by theme.

I just checked and element info is cached globally. So altering that, or any of the other hooks you mentioned is guaranteed to bring you in trouble, because it will depend on whether the backend of frontend theme is active while that cache is rebuilt.

In 7.x., it *looked* like you could alter all those things, but they're very unreliable as written above. I was actually for restoring this as well initially, but since learning that, I'm OK with only having alter for very specific things. hook_theme_registry_alter() is one of them, hook_element_info() is not, unless we also cache it based on the current them, which we probably should.

Adding an issue of such a potential reliability problem.

#29 That is a great point. Yes, once we whitelist a hook for theme altering we need to ensure there are no cache- or other side effects.

That does not mean we cannot whitelist more hooks, just that we need to be careful about it.

That is the great part of having the theme alters specific, because you will always see in the code explicitly the dependencies to themes.

Question: Which one of you is going to be porting themes?

@Mark I've worked on sites with more than one theme and seen issues where alters were mis-applied resulting in criss-crossed cache entries between the admin and front-facing theme. You don't need to be porting themes to run into those bugs, just using them.

Unless you have a theme that takes into account caching said information properly...

Believe me, I'm well aware of how "inconsistent" theme alters can be, but it doesn't mean that they always are. This is just the nature of the render/theme system in general, but we really shouldn't be imposing "rules" for something that 1) hasn't been fully realized yet and 2) is fluid and arbitrary at best anyway...

Like I said, what we had in 7.x isn't "ideal", but it was certainly used far more than "people" think... mainly because themes could not implement module hooks and the only way to tie into module provided information before preprocess/process/pre_render callbacks are invoked is to alter information from the beginning. The fact that themes cannot declare module dependencies is probably what started all this to begin with. I'm not saying that any of this is "right", but it does exist in the current ecosystem.

#35:

So I am all for empowering themes, but the problem is that something that just runs:

- sometimes - dependent on theme negotiation

is something that is inherently incompatible with most caches in Drupal 8.

Compared to 7.x D8 relies a lot more on caching and for a lot of caches the theme can't even influence.

I just remind of mail system and that hook_theme_registry_alter (which here even is allowed ...).

---

On the other hand if a theme would ship a [themename]_module and that would do something like:

function mymodule_foo_alter(&$data) {
  // pseudo code
  if (\Drupal::container('theme_manager')->getActiveTheme() == 'foo') {
  }
}

then nothing is won ... as the result is the same as the theme having the hook in the first place ...

--

#35: What are the theme hooks currently missing and are they dependent on the theme being active or not?

I discussed this in length with @catch in IRC. Re-purposing this issue to discuss and implement the inclusion of modules in themes since theme alters were severely restricted and most of the discussion here is about why we shouldn't allow system wide theme alters again.

As for the following two hooks, these are clear and obvious regressions that need to be added back in. The main reason being that they ultimately pertain to the final output of a theme:

• hook_element_info_alter()
• hook_registry_theme_alter()

I will be creating an issue for each of these hooks. I'm also attaching the related issue as this will go hand in hand with this issue.

Forgot to mention where this issue was introduced.

@Fabianx
Its not only that, its the general problem of a circular dependency between the theme system and the module system.

#39:

- Themes can require modules (when installed, the module is enabled, too)
- Modules cannot depend on themes (by definition that makes no sense)

Therefore => No circular dependency possible.

- Themes can require modules (when installed, the module is enabled, too)
- Modules cannot depend on themes (by definition that makes no sense)

Therefore => No circular dependency possible.

Its not that simply.

So here is the circular dependency.
Let's assume the theme system is not yet active.
Let's assume something fires an alter hook, your code is executed:

function mymodule_foo_alter(&$data) {
  // pseudo code
  if (\Drupal::container('theme_manager')->getActiveTheme() == 'foo') {
  }
}

getActiveTheme() is called, which initializes a theme, let's determine the active theme.
Let's hope that your alter hook is not fired during determine the theme, which itself, can depend on a lot of things.

Note: It used to be worth, when drupal_alter() needed the active theme, because then every alter hooks fires back.

Technically speaking, themes (in 8.x) can include sub-modules and core can discover them in the module list to be enabled/installed.

What's missing, however, is for themes to have the ability to "depend" on them. This is an entirely separate issue: #474684: Allow themes to declare dependencies on modules.

So closing this one.