[regression] Themes unable to implement hook_theme_registry_alter()

#2228093: Modernize theme initialization said that hook_theme_registry_alter() was allowed, is that a bug in the code?

Number two.

Hm. Can you explain how this meets the definition of critical? https://www.drupal.org/node/45111

Critical bugs include those that:

Render a system unusable and have no workaround.
Cause loss of data.
Expose security vulnerabilities.
Cause tests to fail in HEAD on the automated testing platform, since this blocks all other work.

Note especially this part:

Regressions in functionality are not automatically considered critical. Even if the bug existed before, it should be prioritized according to its impact.

I find it a bit difficult to believe that it's impossible to theme at all without this being fixed, but it sounds solidly major since it falls under "Issues that have significant repercussions but do not render the whole system unusable (or have a known workaround) are marked major."

Let's try that.

Another one.

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -328,7 +342,12 @@ protected function build() {
   +    $this->themeManager->alterForTheme($this->theme, 'theme_registry', $cache);
   +    $this->processExtension($cache, $this->theme->getName(), 'theme', $this->theme->getName(), $this->theme->getPath());
   ...
   +    // Let modules alter the registry.
   +    $this->moduleHandler->alter('theme_registry', $cache);
   +    $this->themeManager->alterForTheme($this->theme, 'theme_registry', $cache);
   

   This looks duplicated.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -397,12 +407,7 @@ protected function initTheme(RouteMatchInterface $route_match = NULL) {
   -  public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +  public function alterForTheme(ActiveTheme $theme, $type, &$data, &$context1 = NULL, &$context2 = NULL) {
        // Most of the time, $type is passed as a string, so for performance,
   

   Missing doxy

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -446,4 +450,14 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +   *
   +   * @todo Should we cache some of these information?
   

   This todo should be at alterForTheme.

And here is a test.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -328,7 +345,12 @@ protected function build() {
+    $this->processExtension($cache, $this->theme->getName(), 'theme', $this->theme->getName(), $this->theme->getPath());
+
+    // Let modules alter the registry.
+    $this->moduleHandler->alter('theme_registry', $cache);

These lines are still duplicated - unless I am missing something.

You are absolute right.

Maybe this helps a lot.

Adapted the issue summary.

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
   @@ -122,4 +122,22 @@ public function setActiveTheme(ActiveTheme $active_theme);
   +   * @param string|array $type
   

   Docs missing

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
   @@ -122,4 +122,22 @@ public function setActiveTheme(ActiveTheme $active_theme);
   +   * @return
   

   Docs missing

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
   @@ -122,4 +122,22 @@ public function setActiveTheme(ActiveTheme $active_theme);
   +   * @see ::alter() for more information.
   

   Should not contain trailing text.

4. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
   @@ -122,4 +122,22 @@ public function setActiveTheme(ActiveTheme $active_theme);
   +   *   (optional) An additional variable that is passed by reference.   to be altered.    A string describing the type of the alterable $data.
   +   * @return
   

   Formatting was mixed up here.

5. +++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
   @@ -84,3 +84,10 @@ function test_theme_theme_test_function_suggestions__theme_override($variables)
   + * Implements hook_theme_registry_alter()
   

   Needs full stop.

Also, needs an update to the change notice because we are changing the API.

(Reviewed with @jibran & @dashaforbes) during critical office hours)

@dawehner thanks for the patch. During our review we discussed why this is breaking a circular dependency.
theme manager is only using $themeRegistry in ThemeManager::theme() function. So for all normal workflow we can safely assume that theme registry will be available for theme function.
I can only come up with one scenario in which theme registry will not be available for theme function.
When we are rebuilding the container and before building theme registry we hit some actual circular dependency in that case we try to render the theme for displaying error/exception when there is no theme registry. It'd be good if we can add test for this and handle this boundary case.

@dawehner thanks for the patch. During our review we discussed why this is breaking a circular dependency.

Oh, its not that hard as you think:

The @theme.manager service needs the @theme.registry service, and the other way round.

Also, needs an update to the change notice because we are changing the API.

Do we? We have an alter hook, which is still documented in theme.api.php, yes, the ThemeManagerInterface
changed, but IMHO this is not something we need a dedicated change record for

.

When we are rebuilding the container and before building theme registry we hit some actual circular dependency in that case we try to render the theme for displaying error/exception when there is no theme registry

All bets are off when we get an error that early, an invalid constraint plugin will manifest as a twig error, but actually, its unrelated.
Not sure we should babysit this.
Boldy setting to rtbc

RTBC + 1

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -326,9 +343,9 @@ protected function build() {
+    $this->themeManager->alterForTheme($this->theme, 'theme_registry', $cache);

+++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
@@ -122,4 +122,22 @@ public function setActiveTheme(ActiveTheme $active_theme);
+   * Provides an alter hook for a specific theme.

I think it is worth documenting that the theme's base themes hooks will be invoked as well. Because that is the first thing I wondered when thinking about the patch.

That is a good idea!

Back to RTBC.

Simple grammar edit to #27:

- * Similar to ::alter it invokes also the alter hooks for the base themes.
+ * Similar to ::alter it also invokes the alter hooks for the base themes.

Thanks, that reads better.

I just can't leave well enough alone. Added a comma to my edit for even better, proper grammar:

- * Similar to ::alter it also invokes the alter hooks for the base themes.
+ * Similar to ::alter, it also invokes the alter hooks for the base themes.

Leaving at RTBC.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 08ce5a7 and pushed to 8.0.x. Thanks!