Modernize theme initialization

Note #2016629: Refactor bootstrap to better utilize the kernel touches parts of theme init. Reviews needed.

I really like the proposal so far.

If the theme service is Drupal\Core\Theme\Theme, where will the Theme domain object live?

ViewExecutable::render() uses $GLOBALS['base_theme_info'] and to invoke a couple other "hooks", we should consider those as well.

This property is just a place to store the value, there is no other usage of it in core.

While the service ID is 'theme', its class is ThemeManager. Not sure whether that (re)name was intentional and/or a good idea, but that's most definitely the least of our problems and to be ignored for now.

We both agreed that we should not expose the underlying separate theme services, (init/negotiation etc.) so using 'theme' is probably a good idea in the first place. As a class name, though I really disliked 'theme' so I came up with 'ThemeManager' but yeah I also dislike that name.

One thing I really not sure yet are the following conglomerate of functions:

• ThemeHandler::listInfo() :: uses information from ThemeHandler::rebuildThemeData() and some additional stuff,
  part of that logic is already in Initialization::buildActiveTheme()
• ThemeHandler::rebuildThemeData :: takes the extension objects and parses out the info files and puts it into some object.
  Sadly there are quite some calls to listInfo() which require the parsed information, so I am not sure whether replacing that with a call to the Init service is a good idea (actually I think it is not a good idea).
• system_rebuild_theme_data: Afaik this should move partly into the theme handler (for the simple extension objects)
  and the init service, for the full initialized objects

This is actually no meta anymore.

Fixed mostly all of the failures, some high level review would be cool.

Went through this patch. Surprisingly most of the changes are very intuitive to understand even though I'm not exactly an expert on the current theme initialization system. Great work! It's quite awesome to be able to remove all these globals. I especially like that the ThemeManager stuff is its own class and not stuffed into ThemeHandler like it is done with ModuleHandler. I do have to say however that Drupal\Core\Extension\ThemeHandler vs. Drupal\Core\Theme\ThemeManager is very confusing - both the name and the namespace.

I can sort of see the reasoning for the namespaces as Drupal\Core\Extension seems to be more about (un)installation-related stuff, but still. This is also probably the reason why ActiveTheme does not extend Extension. It's still kind of strange, though. Maybe as a follow-up we can have a ThemeInterface and then Theme extends Extension implements ThemeInterface and ActiveTheme implements ThemeInterface or something like that.

Also the issue summary needs a slight update. It's pretty up-to date. Some class names are incorrect and also I'm not sure about the Negotiator stuff as that is not really in the patch, unless I missed it.

Detailed review:

1. +++ b/core/core.services.yml
   @@ -818,6 +814,12 @@ services:
   +  theme:
   +    class: Drupal\Core\Theme\ThemeManager
   +    arguments: ['@theme_handler', '@theme.registry', '@theme.negotiator', '@theme.initialization', '@request_stack']
   

   Don't really have a better suggestion as I'm reading this top to bottom but noting that a service named theme with a class named ThemeManager that gets a thing called theme_handler injected is not really self-documenting on who does what.

   Also it's kind of weird that ThemeHandler is in Drupal\Core\Extension and Drupal\Core\Theme.

2. +++ b/core/includes/common.inc
   @@ -1197,24 +1210,25 @@ function drupal_get_css($css = NULL, $skip_alter = FALSE) {
        \Drupal::moduleHandler()->alter('css', $css);
   +    \Drupal::theme()->alter('css', $css);
   

   Wow, this looks exciting! :-)

   (Although again the discrepancy between moduleHandler and theme is a bit strange.)

3. +++ b/core/includes/common.inc
   @@ -1847,7 +1861,16 @@ function drupal_js_defaults($data = NULL) {
   +function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALSE, $is_ajax = FALSE, $theme_add_js = TRUE) {
   

   Super minor but it's sort of non-standard to start a function with a newline.

4. +++ b/core/includes/form.inc
   @@ -3184,7 +3183,7 @@ function batch_process($redirect = NULL, $url = 'batch', $redirect_callback = NU
   -      'theme' => $GLOBALS['theme_key'],
   +      'theme' => drupal_installation_attempted() ? 'seven' : \Drupal::theme()->getActiveTheme()->getName(),
   

   Didn't dig into this code so this comment might be moot, but AFAIK it is possible for install profiles to specify alternate installation themes. Does this code conflict with this?

5. +++ b/core/includes/theme.inc
   @@ -765,7 +534,7 @@ function drupal_find_theme_templates($cache, $extension, $path) {
   -  global $theme;
   +  $theme = \Drupal::theme()->getActiveTheme()->getName();
   
   @@ -867,7 +636,7 @@ function theme_get_setting($setting_name, $theme = NULL) {
   -    $theme = !empty($GLOBALS['theme_key']) ? $GLOBALS['theme_key'] : '';
   +    $theme = \Drupal::theme()->getActiveTheme()->getName();
   

   Wow, I never knew these two globals are identical. Good thing you're removing all that crap!

6. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -389,6 +395,7 @@ public function refreshInfo() {
        $this->systemListReset();
   +    \Drupal::cache('bootstrap')->delete('system_list');
   

   Is there a reason the system_list cache cannot be cleared by $this->systemListReset()? Or at least by some method on SystemListing itself?

7. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -0,0 +1,141 @@
   + * Defines an theme and its information needed on runtime.
   

   an -> a
   on -> at

   That said "Defines a theme" is not very clear IMO. However, Extension has the same comment, so I suppose it's fine for now.

8. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -0,0 +1,141 @@
   +class ActiveTheme {
   

   Lot's of docs missing in this class not noting each case here.

9. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -0,0 +1,141 @@
   +   * The path to the string.
   

   I assume that's supposed to be "The path to the theme."

10. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -0,0 +1,141 @@
    +  /**
    +   * Contains a reference to its parent theme or NULL if there is none.
    +   *
    +   * @var self|NULL
    +   */
    +  protected $parent;
    

    What is a parent theme? I've never heard of that before.

    Also I think "The parent theme, if there is one." is sufficient as documentation.

11. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -0,0 +1,141 @@
    +   * Contains the engine of the theme.
    

    Super minor but maybe drop the "Contains"

12. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -0,0 +1,141 @@
    +    if (isset($values['engine'])) {
    +      $this->engine = $values['engine'];
    +    }
    

    It seems strange that I can have a theme without an engine. Why is that not required?

13. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -0,0 +1,141 @@
    +    if (isset($values)) {
    +      $this->owner = $values['owner'];
    +    }
    

    This presumably should be isset($values['owner'])

14. +++ b/core/lib/Drupal/Core/Theme/DefaultNegotiator.php
    @@ -43,7 +43,7 @@ public function applies(RouteMatchInterface $route_match) {
    -    return $this->config->get('default');
    +    return $this->config->get('default') ?: 'stark';
    

    In which case is $this->config->get() not sufficient? Maybe we should move the default theme to core.extension.yml?

15. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
    @@ -0,0 +1,219 @@
    +class Initialization {
    

    Again, there's lots of docs missing here.

    I might be old school but to me a class name of Initialization is not really self-explaining, to be honest. (Yes, I have heard of namespaces... :-)) Feel free to ignore.

16. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
    @@ -0,0 +1,219 @@
    +  /**
    +   * Build active theme objects for all themes.
    +   */
    +  protected function buildActiveThemes() {
    +  }
    

    ?

17. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
    @@ -0,0 +1,219 @@
    +  public function loadActiveTheme(ActiveTheme $active_theme) {
    +
    +    // Initialize the theme.
    

    Again, maybe remove the newline there.

18. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
    @@ -0,0 +1,219 @@
    +    if ($theme_engine = $active_theme->getEngine()) {
    +      // Include the engine.
    +      include_once DRUPAL_ROOT . '/' . $active_theme->getOwner();
    +
    +      if (function_exists($theme_engine . '_init')) {
    +        foreach ($active_theme->getBaseThemes() as $base) {
    +          call_user_func($theme_engine . '_init', $base->getExtension());
    +        }
    +        call_user_func($theme_engine . '_init', $active_theme->getExtension());
    +      }
    +    }
    +    else {
    +      // include non-engine theme files
    +      foreach ($active_theme->getBaseThemes() as $base) {
    +        // Include the theme file or the engine.
    +        if ($base->getOwner()) {
    +          include_once DRUPAL_ROOT . '/' . $base->getOwner();
    +        }
    +      }
    +      // and our theme gets one too.
    +      if ($active_theme->getOwner()) {
    +        include_once DRUPAL_ROOT . '/' . $active_theme->getOwner();
    +      }
    +    }
    +
    +    // Always include Twig as the default theme engine.
    +    include_once DRUPAL_ROOT . '/core/themes/engines/twig/twig.engine';
    

    I see now that this is just being copied, but nevertheless it's rather strange.

    • Again, how can themes not have an engine?
    • It seems theme engines are being treated as themes themselves (l. 84)? Why does that not pop up anywhere else?
    • Why is it required to hardcode Twig?
    • I know this is basically a "(anti-)feature request" as this is just how it works currently but it's sheer madness to have a base theme with a different theme engine than yourself.

    I'm mostly just documenting my thought process here, do not feel obligated to change any of this. (Although I will buy you cookies if you do! :-))

19. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
    @@ -0,0 +1,219 @@
    +   *    An optional array of objects that represent the 'base theme' if the
    +   *    theme is meant to be derivative of another theme. It requires
    +   *    the same information as the $theme object. It should be in
    +   *    'oldest first' order, meaning the top level of the chain will
    +   *    be first.
    

    This documentation is not very clear to me. I do not understand how a theme can be "meant to be derivative of another theme". I would have thought either it has a base theme (or multiple) or it doesn't, end of story. Can you elaborate and possibly revise the documentation?

20. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
    @@ -0,0 +1,219 @@
    +   * @return \Drupal\Core\Theme\ActiveTheme
    +   *   The active theme instance for the passed in $theme.
    +   */
    +  public function getActiveTheme(Extension $theme, array $base_themes) {
    

    I'm sure that there's a reason for it, but I wanted to note that the fact that ActiveTheme does not extend Extension is rather confusing. Especially as most of this code basically just "copies" values from one data structure to the other.

    I see this method being called quite a bunch above. Is there no value in adding some sort of static caching here? drupal_theme_initialize() had that in the form of checking whether $GLOABLS['theme'] was set already. That might be the wrong comparison, though.

21. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -115,6 +115,15 @@ class Registry implements DestructableInterface {
    +  protected $initialized = FALSE;
    

    Missing docs.

22. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -115,6 +115,15 @@ class Registry implements DestructableInterface {
    +   * (optional) The name of the theme for which to construct the registry.
    

    I don't think we do "(optional)" for @var documentation. Maybe just append a ", if given." to the end (or leave it out entirely).

23. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -115,6 +115,15 @@ class Registry implements DestructableInterface {
    +   * @var null|string
    

    Super minor but I think the other way around would be more intuitive?

24. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -144,19 +153,17 @@ public function __construct(CacheBackendInterface $cache, LockBackendInterface $
    +      if ($parent = $active_theme->getParent()) {
    +        $this->baseThemes = array($parent);
           }
    

    Ahh, so this is that parent thing. So why would I specify parent instead of specifying the base themes directly? Unless I missed it Initialization::getActiveTheme() never uses parent either.

25. +++ b/core/lib/Drupal/Core/Theme/ThemeAccessCheck.php
    @@ -38,7 +38,7 @@ public function access($theme) {
    -    return !empty($themes[$theme]->status);
    +    return !empty($themes[$theme]);
    

    Can you explain this change? I do not understand it at all.

26. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,148 @@
    +class ThemeManager implements ThemeManagerInterface {
    

    Again, there's some docs missing here.

27. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,148 @@
    +  public function render($hook, $variables) {
    +    return _theme($hook, $variables);
    +  }
    

    Yay!

28. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,148 @@
    +  public function getActiveTheme(RouteMatchInterface $route_match = NULL) {
    

    Ahh so scratch the comment about the static cache above. The code is in fact calling this getActiveTheme() method not the Initialization one. Makes sense. Although the duplicated method names are slightly confusing. Don't have any suggestions, though.

29. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,148 @@
    +    }
    +    }
    

    Weird indentation here.

30. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,148 @@
    +
    +
    

    Could remove the newlines here.

31. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -139,14 +139,6 @@ function testFrontPageThemeSuggestion() {
    -  function testAlter() {
    -    $this->drupalGet('theme-test/alter');
    -    $this->assertText('The altered data is test_theme_theme_test_alter_alter was invoked.', 'The theme was able to implement an alter hook during page building before anything was rendered.');
    -  }
    

    I can't say that I really understand this test, but is there a specific reason for removing it?

Thank you for your really detailed review!

Don't really have a better suggestion as I'm reading this top to bottom but noting that a service named theme with a class named ThemeManager that gets a thing called theme_handler injected is not really self-documenting on who does what.

Maybe ActiveThemeManager (theme.active) would be a better name? In this case moving the creation process of these objects should be moved away from the initializer ...

(Although again the discrepancy between moduleHandler and theme is a bit strange.)

I do totally agree. A good name could be similar to the suggestions before: \Drupal::activeTheme() maybe?

Didn't dig into this code so this comment might be moot, but AFAIK it is possible for install profiles to specify alternate installation themes. Does this code conflict with this?

http://cgit.drupalcode.org/commerce_kickstart/tree/commerce_kickstart.in... shows an example how a profile does it. Given
that, I guess drupal_maintenance_theme() has to be converted to use the new machinery properly. A new theme negotiator for the maintenance mode would be introduced.

Is there a reason the system_list cache cannot be cleared by $this->systemListReset()? Or at least by some method on SystemListing itself?

Is there a reason the system_list cache cannot be cleared by $this->systemListReset()? Or at least by some method on SystemListing itself?

Interesting ... actually the same step is done by systemListReset()->system_list_reset() already. Good catch!

What is a parent theme? I've never heard of that before.

Oh right, we should call it base instead.

It seems strange that I can have a theme without an engine. Why is that not required?

Yeah, this was more a leftover while writing stuff.

In which case is $this->config->get() not sufficient? Maybe we should move the default theme to core.extension.yml?

Yeah these are workarounds at the moment to get a proper working installer. Having a separate maintenance negotiator hopefully
resolves that situation

I might be old school but to me a class name of Initialization is not really self-explaining, to be honest. (Yes, I have heard of namespaces... :-)) Feel free to ignore.

Would you go directly with ThemeInitialization? I would be fine with that.

Again, how can themes not have an engine?

Maybe now this is no longer the case, but at least in 4.7 there is chameleon which just has a .theme file with theme_page and whatnot.

It seems theme engines are being treated as themes themselves (l. 84)? Why does that not pop up anywhere else?

All that code after line 80 is indeed just for themes without a theme engine.

Why is it required to hardcode Twig?

No idea, tbh!

I know this is basically a "(anti-)feature request" as this is just how it works currently but it's sheer madness to have a base theme with a different theme engine than yourself.

Totally. To be honest I can't imagine a usecase for that but probably someone out there misuses that feature somehow.

This documentation is not very clear to me. I do not understand how a theme can be "meant to be derivative of another theme". I would have thought either it has a base theme (or multiple) or it doesn't, end of story. Can you elaborate and possibly revise the documentation?

Rewrote the documentation to make it a little bit less verbose.

I'm sure that there's a reason for it, but I wanted to note that the fact that ActiveTheme does not extend Extension is rather confusing. Especially as most of this code basically just "copies" values from one data structure to the other.

Our goal here was to decouple the concept of active themes from the concept of the discovery side. I can understand that an extension shares a lot with the active theme object, though an active theme has a couple of more information than just the extension. Therefere composition might be a good idea we could use instead copying all the information from one place to the other. Note: On runtime the information is cached in state anyway, so we don't have to worry about it.

I see this method being called quite a bunch above. Is there no value in adding some sort of static caching here? drupal_theme_initialize() had that in the form of checking whether $GLOABLS['theme'] was set already. That might be the wrong comparison, though.

Yeah, as written before, maintenance is the killer. On a normal drupal request though, nothing beside the theme manager will ever call it.

Super minor but I think the other way around would be more intuitive?

I do totall agree here.

Ahh, so this is that parent thing. So why would I specify parent instead of specifying the base themes directly? Unless I missed it Initialization::getActiveTheme() never uses parent either.

You are totally right here, added a todo for now, working on it.

Can you explain this change? I do not understand it at all.

This change is a bit tricky. When the active theme is determined each theme negotiator is asked. If that one returns a theme, the access
checker is taken into account, which ensures that the theme is enabled. (HEAD). Previously this system was bypassed completly in the installer, (drupal_theme_maintenance()), so the global got set and what not. On install time no theme is actually enabled, so the status checks now always fails. If no config was installed yet, there is a workaround in ThemeHandler::refreshInfo()

    $extension_config = $this->configFactory->get('core.extension');
    // @FIXME: this is just a workaround
    $enabled = $extension_config->get('theme') ?: array('stark' => 1, 'seven' => 1);

So once the config is in place, ThemeHandler::rebuildThemeData() will set the status, so the result is basically the same. I guess
one "workaround" would be the follwing: add seven and stark to core.config.yml. Allow install profiles specify a theme in their .info.yml file, though then there is no way yet, to store the config, as there is no DB, neither the file system ready yet. Any ideas?

I can't say that I really understand this test, but is there a specific reason for removing it?

Well, this tests generic alter functionality (so we need to explicit call the theme alter now) (as in other places) but yeah I am fine with adding it back in.

The seems indeed mostly a random failure. Let's fix the schema issue.

• Fixed the last failure
• Replaced hopefully all globals now.

Sort of a distracted skim of the patch.

1. +++ b/core/includes/common.inc
   @@ -1187,8 +1187,21 @@ function _drupal_add_css($data = NULL, $options = NULL) {
   +  // @todo There are probably better place to add the CSS from themes.
   +  if ($theme_add_css) {
   +    foreach ($theme_info->getStyleSheets() as $media => $stylesheets) {
   +      foreach ($stylesheets as $stylesheet) {
   +        _drupal_add_css($stylesheet, array(
   +          'group' => CSS_AGGREGATE_THEME,
   +          'every_page' => TRUE,
   +          'media' => $media
   +        ));
   +      }
   +    }
   

   Eww, I agree with this @todo. If for no other reason, how do I know where this is being called from?

2. +++ b/core/includes/common.inc
   @@ -1847,7 +1861,15 @@ function drupal_js_defaults($data = NULL) {
   +  if ($theme_add_js) {
   +    // Add libraries used by this theme.
   +    foreach ($active_theme->getLibraries() as $library) {
   +      _drupal_add_library($library);
   +    }
   +  }
   

   Seems this deserves the same @todo as add_css.

3. +++ b/core/includes/form.inc
   @@ -3056,9 +3056,8 @@ function batch_set($batch_definition) {
   +    $a = 123;
   

   what?

4. +++ b/core/includes/theme.maintenance.inc
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Theme\ActiveTheme;
   

   Not seeing where this is used.

5. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -482,30 +482,6 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   -      // Allow the theme to alter variables after the theme system has been
   -      // initialized.
   -      global $theme, $base_theme_info;
   -      if (isset($theme)) {
   -        $theme_keys = array();
   -        foreach ($base_theme_info as $base) {
   -          $theme_keys[] = $base->getName();
   -        }
   -        $theme_keys[] = $theme;
   -        foreach ($theme_keys as $theme_key) {
   -          $function = $theme_key . '_' . $hook;
   -          if (function_exists($function)) {
   -            $this->alterFunctions[$cid][] = $function;
   -          }
   -          if (isset($extra_types)) {
   -            foreach ($extra_types as $extra_type) {
   -              $function = $theme_key . '_' . $extra_type . '_alter';
   -              if (function_exists($function)) {
   -                $this->alterFunctions[$cid][] = $function;
   -              }
   -            }
   -          }
   -        }
   -      }
   

   yes. yes. yes.

6. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -0,0 +1,180 @@
   +  /**
   +   * @todo We probably want to switch to use named parameters.
   +   */
   +  public function __construct($values) {
   +    $this->name = $values['name'];
   +    $this->path = $values['path'];
   +    if (isset($values['parent'])) {
   +      $this->parent = $values['parent'];
   +    }
   +    $this->engine = $values['engine'];
   +    $this->owner = $values['owner'];
   +    $this->styleSheets = $values['stylesheets'];
   +    $this->styleSheetsRemove = $values['stylesheets_remove'];
   +    $this->styleSheetsOverride = $values['stylesheets_override'];
   +    $this->libraries = $values['libraries'];
   +    $this->extension = $values['extension'];
   +    $this->baseThemes = $values['base_themes'];
   +  }
   

   Wow this is weird...

7. +++ b/core/lib/Drupal/Core/Theme/DefaultNegotiator.php
   @@ -43,7 +43,7 @@ public function applies(RouteMatchInterface $route_match) {
   -    return $this->config->get('default');
   +    return $this->config->get('default') ?: 'stark';
   

   Had a comment but apparently this is a workaround. Should we make that clear for review/ongoing work on this patch?

8. +++ b/core/lib/Drupal/Core/Theme/Initialization.php
   @@ -0,0 +1,256 @@
   +  protected function buildActiveThemes() {
   +  }
   

   empty? why?

9. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -331,6 +346,7 @@ protected function build() {
   +    // @todo Do we want to allow themes to take part?
    
   

   I'd like to say no but really this is probably a yes.

Fixed the failures ... again :p

Seems this deserves the same @todo as add_css.

Agreed.

what

This is how I debug :p

Not seeing where this is used.

Good catch!

Wow this is weird...

Would you just use normal parameters in the constructor or pass along the extension object?

Had a comment but apparently this is a workaround. Should we make that clear for review/ongoing work on this patch?

Added ...

empty? why?

Removed it for now. Maybe we want to not lazy-build the active theme objects, but I guess it is fine to build them over time, as we do now.

Just realized that this could be quite some performance improvement for REST and other places where you don't have to initialize the theme system.

#28: Indeed! This can really help make non-HTML responses quite a bit faster.

• Rebased
• Introduced a ThemeInitializationInterface and used it
• Reworked a bit on better todo tests (note, all of them are basically todos which existed before but weren't really written down)
• Improved documentation etc.
• Replaced the @theme service with a @theme.manager service.

I read through the patch and its a superb cleanup.

1. Could we add a test that ensures that we don't regress on 'don't init the theme for non-html responses'. We didn't init a theme() in this case in a few versions of Drupal but apparently that regressed. A test would help us never regress again. Feel free to use a REST or XML feed or system/ajax route for that. We have an analogous test that ensures that we don't have any javascript on the homepage.
2. I am expecting theme_get_setting() to be part of the OO API. Is that a planned follow-up?
3. The patch has several @todo items. Those need issue links, or they need to be removed.

I am expecting theme_get_setting() to be part of the OO API. Is that a planned follow-up?

There is a related issue for this already: #2235901: Remove custom theme settings from *.info.yml

The patch has several @todo items. Those need issue links, or they need to be removed.

While I do agree that filling follow ups is a good idea here is a fundamental flaw in our process. By requiring followups or removing todos
we reduce the honesty of people. They will not write down each todo they could think of, but rather just go with some big problems.
Worse than technical dept is hidden technical dept.

Here are a list of followups:

• #2322617: Figure out where to add CSS/JS from themes
• #2322619: Remove workarounds in the theme system for the maintenance page and the installer
• #2322623: Try to remove the coupling of a view to the theme system

Thanks for opening those issues. IMO we are RTBC here.

Needs a reroll

Rerolled

Can someone explain why we have so many "random" test failures recently? I saw similar behaviour on the stack issue.

1.     $this->moduleHandler->alter($hooks, $form, $form_state, $form_id);
       $this->themeManager->alter($hooks, $form, $form_state, $form_id);
   

   This seems inconsistent - modules have a handler, themes have a handler and manager. For modules the handler fires hooks and for themes its the manager. I get why this is since the alter only get's fired for the active theme - which is something the ThemeManager knows about but not the ThemeHandler. As @dawehner said in IRC - perhaps we need to refactor ModuleHandler. :(

2. We have mentions of drupal_theme_initialize() and path_to_theme() left in Drupal\Core\Theme\Registry
3. Drupal\Core\Theme\Registry no longer needs to use Drupal\Core\Extension\Extension
4. theme.inc no longer needs to use Drupal\Core\Routing\RouteMatch

5. +++ b/core/includes/common.inc
   @@ -1164,8 +1164,21 @@ function _drupal_add_css($data = NULL, $options = NULL) {
   +  // @todo There is probably a better place to add the CSS from themes.
   +  if ($theme_add_css) {
   +    foreach ($theme_info->getStyleSheets() as $media => $stylesheets) {
   +      foreach ($stylesheets as $stylesheet) {
   +        _drupal_add_css($stylesheet, array(
   +          'group' => CSS_AGGREGATE_THEME,
   +          'every_page' => TRUE,
   +          'media' => $media
   +        ));
   +      }
   +    }
   +  }
   
   @@ -1812,7 +1826,16 @@ function drupal_js_defaults($data = NULL) {
   +  // @todo There is probably a better place to add the JS from themes.
   +  $active_theme = \Drupal::theme()->getActiveTheme();
   +  if ($theme_add_js) {
   +    // Add libraries used by this theme.
   +    foreach ($active_theme->getLibraries() as $library) {
   +      _drupal_add_library($library);
   +    }
   +  }
   

   Can we get an issue opened for this and referenced in the @todo

6. +++ b/core/includes/theme.maintenance.inc
   @@ -98,9 +96,10 @@ function _drupal_maintenance_theme() {
   +  // @todo This is just a workaround. Find a better way how to handle themes
   +  // on maintenance pages.
   +  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], array_reverse($base_theme)));
   

   Can we get an issue for this?

7. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -347,6 +347,13 @@ public function listInfo() {
   +      // @TODO Ensure that systemThemeList() does not contain an empty list
   +      //   during the batch installer.
   

   Followup issue? Or is this not a problem anymore?

8. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -121,7 +129,7 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
   -  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
   +  public function __construct(FormValidatorInterface $form_validator, FormSubmitterInterface $form_submitter, ModuleHandlerInterface $module_handler, KeyValueExpirableFactoryInterface $key_value_expirable_factory, EventDispatcherInterface $event_dispatcher, RequestStack $request_stack, ClassResolverInterface $class_resolver, ThemeManagerInterface $theme_manager, CsrfTokenGenerator $csrf_token = NULL, HttpKernel $http_kernel = NULL) {
   

   Missing new @param doc

9. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -0,0 +1,189 @@
   +/**
   + * Defines a theme and its information needed at runtime.
   + */
   +class ActiveTheme {
   

   Perhaps we could have a @see to the ThemeManager interface or something about the theme manager service so people know where such an object will be created and can be obtained from

10. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -0,0 +1,189 @@
    +  protected $owner;
    ...
    +  /**
    +   * Returns the path theme engine file.
    +   *
    +   * @return mixed
    +   */
    +  public function getOwner() {
    +    return $this->owner;
    +  }
    

    Needs docblock - and what is it? $owner and path theme engine file? Confusing.

11. +++ b/core/lib/Drupal/Core/Theme/DefaultNegotiator.php
    @@ -43,7 +43,10 @@ public function applies(RouteMatchInterface $route_match) {
    +    // @todo Find a proper way to work at the beginning of the installer when
    +    //   there is no configuration available yet. One proper way could be to
    +    //   provider a custom negotiator during the installer.
    +    return $this->config->get('default') ?: 'stark';
    

    Needs followup issue.

12. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -331,6 +343,7 @@ protected function build() {
    +    // @todo Do we want to allow themes to take part?
    

    Needs a followup issue.

13. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
    @@ -0,0 +1,220 @@
    +   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
    +   * @param \Drupal\Core\State\StateInterface $state
    +   */
    +  public function __construct(ThemeHandlerInterface $theme_handler, StateInterface $state) {
    

    @param's need description

14. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,161 @@
    +namespace Drupal\Core\Theme;
    +use Drupal\Core\Extension\ThemeHandlerInterface;
    

    Missing new line between namespace and use.

15. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,161 @@
    +use Drupal\Core\KeyValueStore\StateInterface;
    

    Not used and does not exist :)

16. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,161 @@
    +use Drupal\Core\Utility\ThemeRegistry;
    

    Not used.

17. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,161 @@
    +  public function __construct(ThemeHandlerInterface $theme_handler, Registry $theme_registry, ThemeNegotiatorInterface $theme_negotiator, ThemeInitialization $theme_initialization, RequestStack $request_stack) {
    

    Missing docblock

18. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -0,0 +1,161 @@
    +  protected function initTheme(RouteMatchInterface $route_match = NULL) {
    

    Missing docblock

19. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
    @@ -0,0 +1,78 @@
    +  /**
    +   * Execute the alter hook on the current theme.
    +   *
    +   * @param string $type
    +   *
    +   * @see \Drupal\Core\Extension\ModuleHandlerInterface
    +   */
    +  public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL);
    

    Missing param docs - I don;t think we should rely on the @see.

20. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -139,14 +149,6 @@ function testFrontPageThemeSuggestion() {
       /**
    -   * Ensures theme hook_*_alter() implementations can run before anything is rendered.
    -   */
    -  function testAlter() {
    -    $this->drupalGet('theme-test/alter');
    -    $this->assertText('The altered data is test_theme_theme_test_alter_alter was invoked.', 'The theme was able to implement an alter hook during page building before anything was rendered.');
    -  }
    

    Why are we removing this test? If we need to then we should also remove the route and the method from ThemeTestController

EDIT: fixed html

This seems inconsistent - modules have a handler, themes have a handler and manager. For modules the handler fires hooks and for themes its the manager. I get why this is since the alter only get's fired for the active theme - which is something the ThemeManager knows about but not the ThemeHandler. As @dawehner said in IRC - perhaps we need to refactor ModuleHandler. :(

#2324055: Split up the module manager into runtime information and extension information.

Drupal\Core\Theme\Registry no longer needs to use Drupal\Core\Extension\Extension

theme.inc no longer needs to use Drupal\Core\Routing\RouteMatch

Good catches!

Can we get an issue opened for this and referenced in the @todo

See comment 32:

• #2322617: Figure out where to add CSS/JS from themes
• #2322619: Remove workarounds in the theme system for the maintenance page and the installer
• #2322623: Try to remove the coupling of a view to the theme system

Perhaps we could have a @see to the ThemeManager interface or something about the theme manager service so people know where such an object will be created and can be obtained from

Added a reference to both relevant places.

Needs docblock - and what is it? $owner and path theme engine file? Confusing.

Well sure, but nothing new for this patch. The owner is the theme engine, in case the theme is a root base path.

Missing param docs - I don;t think we should rely on the @see.

Well, sure.

Why are we removing this test? If we need to then we should also remove the route and the method from ThemeTestController

Well yeah, this test checks generic alter functionality.

There are other places which still tests alterting though.

I think those changes address @alexpott's concerns.

Yep looks great but we need a CR for this.

Good point!
Splitted it up into the two change records, as they deal with two different problem spaces:

• htts://drupal.org/node/2324935
• htts://drupal.org/node/2324939

Reroll.

Back to RTBC.

Committed a4125e6 and pushed to 8.0.x. Thanks!

diff --git a/core/includes/theme.maintenance.inc b/core/includes/theme.maintenance.inc
index 8b5c6b3..2ceec4a 100644
--- a/core/includes/theme.maintenance.inc
+++ b/core/includes/theme.maintenance.inc
@@ -85,7 +85,7 @@ function _drupal_maintenance_theme() {
 
   // list_themes() triggers a \Drupal\Core\Extension\ModuleHandler::alter() in
   // maintenance mode, but we can't let themes alter the .info.yml data until
-  // we know a theme's base themes. So don't set global $theme until after
+  // we know a theme's base themes. So don't set active theme until after
   // list_themes() builds its cache.
   $theme = $custom_theme;
 
diff --git a/core/lib/Drupal/Core/Theme/Registry.php b/core/lib/Drupal/Core/Theme/Registry.php
index 947a443..0ad005d 100644
--- a/core/lib/Drupal/Core/Theme/Registry.php
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -168,7 +168,7 @@ protected function init($theme_name = NULL) {
       $this->baseThemes = $active_theme->getBaseThemes();
       $this->engine = $active_theme->getEngine();
     }
-    // Instead of the global theme, a specific theme was requested.
+    // Instead of the active theme, a specific theme was requested.
     else {
       $themes = $this->listThemes();
       $this->theme = $themes[$theme_name];

Fixed on commit.

Thank you so much alex!!!

This most probably caused the critical at #2331991: Theme missing when installing in non-English language in which Drupal is *very* confused if there is even an active theme. The active theme is only initialised partially (I have a half-baked patch there which makes it 2/3rds baked :P), but then no twig templates are used from the theme either.