Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK()

Steal issue and add more explanation for new API

Initial proof-of-concept for simply adding the hooks into Drupal. It doesn't yet replace/deprecate preprocess. I didn't add or modify any core functions, but instead created a simpletest. The test should eventually be integrated properly with the normal Theme API tests, just didn't want to wait 5 mins each time.

#2: drupal-introduce-hook_theme_prepare-2035055-2.patch queued for re-testing.

Nice start!

UpdateModuleHandler needs to be modified to allow for system theme_prepare hooks. Bah! Should fully pass now :)

+++ b/core/includes/theme.incundefined
@@ -1036,6 +1036,18 @@ function theme($hook, $variables = array()) {
+  // Invoke hook_theme_prepare().
+  $variables = NestedArray::mergeDeep($variables, Drupal::moduleHandler()->invokeAll('theme_prepare', array($hook, $variables)));
...
+  // Alter variables by invoking hook_theme_prepare_alter($variables, $hook).
+  Drupal::moduleHandler()->alter('theme_prepare', $variables, $hook_clone);

I think we should be consistent in our order:

Either $hook, $variables OR
$variables, $hook.

#9: Even for FAPI?

Personally, I'd rather stick with $variables, $hook (like FAPI does). That way it's consistent through out Drupal. Besides, $hook is supplemental. A module that implements hook_theme_prepare($variables) technically doesn't have to use $hook. If they want to just add to something to every $variables array, they could just ignore $hook.

typo

Updated issue summary (edit: per IRC conversations).

Updated issue summary.

Updated issue summary.

Bumping to a major task since this would fix a critical bug in the existing 8.x code: #939462: Specific preprocess functions for theme hook suggestions are not invoked

Forgot to set to NW since there's still a lot to do.

Reroll

Updated issue summary.

Updating title to reflect issue summary.

Ok, completely refactored code to coincide with issue summary and IRC chats:

• Added temporary Drupal::moduleHandler()->themeInvoke() method.
• Added theme tests for hook_theme_prepare() and hook_theme_prepare_THEME_ID().
• Added @todo comments for replacing things like #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() (tentatively and in reality a blocker for this issue I think) and #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler).

Still need to figure out how to deprecate hook_preprocess() properly though.

Updated issue summary.

Updated issue summary.

I know I changed it earlier, but was really starting to get on my nerves and thinking this is way too long of a title.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Sigh... passed locally, wtf. I'm just going to include the theme ids and suggestions in the default $context. Who knows how people will use it and it may be useful (even for suggestion specific hooks).

Updated issue summary.

Looks good on an initial read and applies cleanly, but I need some good use cases/examples to test against and give it a thorough thrashing.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Issue summary update.

This is introducing a new API to help with:

• Deprecating the use of hook_preprocess()
• And migrating all core hook_preprocess() functions to utilize this new API.

(edit: removed "example" code based on old talks, don't want to confuse this issue).

correct issue

This looks good to me. Don't see any brokenness anywhere and will definitely help themers.

Sorry, should have marked this as NW. I have some remaining tasks I still need to do.

Updated issue summary.

Some initial thoughts after a first review. This is looking good overall, I'm liking all the test coverage.

(edited to listify)

1. +++ b/core/includes/theme.inc
   @@ -995,6 +995,54 @@ function theme($hook, $variables = array()) {
   +    '#base_theme_id' => $hook,
   

   This won't actually be #base_theme_id in a lot of cases, especially for manually called theme suggestions like '#theme' => 'node__foo__bar'. $hook at this time is the most specific theme suggestion that an implementation was found for.

2. +++ b/core/includes/theme.inc
   @@ -995,6 +995,54 @@ function theme($hook, $variables = array()) {
   +  $invoke_hooks = array(
   +    'theme_prepare',
   +    'theme_prepare_' . $hook,
   +  );
   +  // If the original theme hook is not the same as the base theme hook, then
   +  // invoke the original theme hook suggestion after the base theme hook.
   +  if ($hook !== $original_hook) {
   +    $invoke_hooks[] = 'theme_prepare_' . $original_hook;
   +  }
   +
   +  // Invoke hooks for enabled modules.
   +  foreach ($invoke_hooks as $invoke_hook) {
   +    $variables = NestedArray::mergeDeep($variables, (array) Drupal::moduleHandler()->invokeAll($invoke_hook, array($context)));
   +  }
   +
   +  // Invoke hooks for the active theme. The active theme takes precedence over
   +  // modules.
   +  // @todo Replace with Drupal::themeHandler()->invoke() once implemented.
   +  // @see https://drupal.org/node/2029819
   +  foreach ($invoke_hooks as $invoke_hook) {
   +    $variables = NestedArray::mergeDeep($variables, (array) Drupal::moduleHandler()->themeInvoke($invoke_hook, array($context)));
   +  }
   +
   +  // Clone the context so it cannot be altered by modules or themes.
   +  $context_clone = $context;
   +
   +  // Invoke alter hooks for enabled modules.
   +  // @todo Add Drupal::themeHandler()->alter() invocations once implemented.
   +  // @see https://drupal.org/node/2029819
   +  foreach ($invoke_hooks as $invoke_hook) {
   +    Drupal::moduleHandler()->alter($invoke_hook, $variables, $context_clone);
   +  }
   

   The inline comments in this section don't tell me that the code is for prepare hooks and prepare alter hooks.

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemePrepareTest.php
   @@ -0,0 +1,54 @@
   +/**
   + * @file
   + * Definition of Drupal\system\Tests\Theme\ThemeTest.
   + */
   

   Contains \Drupal\system\Tests\Theme\ThemePrepareTest.

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemePrepareTest.php
   @@ -0,0 +1,54 @@
   +use Drupal\test_theme\ThemeClass;
   

   Is this actually used?

5. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemePrepareTest.php
   @@ -0,0 +1,54 @@
   + * Tests low-level theme functions.
   + */
   +class ThemePrepareTest extends WebTestBase {
   

   Docblock needs updating.

6. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemePrepareTest.php
   @@ -0,0 +1,54 @@
   +    $this->drupalGet('theme-test/prepare', array('query' => array('XDEBUG_SESSION_START' => 'PHPSTORM')));
   ...
   +    $this->drupalGet('theme-test/prepare-alter', array('query' => array('XDEBUG_SESSION_START' => 'PHPSTORM')));
   

   Remove debug query code please :)

7. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemePrepareTest.php
   @@ -0,0 +1,54 @@
   +    $this->assertText('Inserted variable1 via hook_theme_prepare().', 'Inserted variable1 via hook_theme_prepare().');
   +    $this->assertText('Inserted variable2 via hook_theme_prepare_BASE_THEME_ID().', 'Inserted variable2 via hook_theme_prepare_BASE_THEME_ID().');
   +    $this->assertText('Inserted themeVariable1 via theme hook_theme_prepare().', 'Inserted themeVariable1 via theme hook_theme_prepare().');
   +    $this->assertText('Inserted themeVariable2 via theme hook_theme_prepare_BASE_THEME_ID().', 'Inserted themeVariable2 via theme hook_theme_prepare_BASE_THEME_ID().');
   ...
   +    $this->assertText('Inserted variable1 via hook_theme_prepare() was altered by hook_theme_prepare_alter().', 'Inserted variable1 via hook_theme_prepare() was altered by hook_theme_prepare_alter().');
   +    $this->assertText('Inserted variable2 via hook_theme_prepare_BASE_THEME_ID() was altered by hook_theme_prepare_BASE_THEME_ID_alter().', 'Inserted variable2 via hook_theme_prepare_BASE_THEME_ID() was altered by hook_theme_prepare_BASE_THEME_ID_alter().');
   +    $this->assertText('Inserted themeVariable1 via theme hook_theme_prepare() was altered by theme hook_theme_prepare_alter().', 'Inserted themeVariable1 via theme hook_theme_prepare() was altered by theme hook_theme_prepare_alter().');
   +    $this->assertText('Inserted themeVariable2 via theme hook_theme_prepare_BASE_THEME_ID() was altered by theme hook_theme_prepare_BASE_THEME_ID_alter().', 'Inserted themeVariable2 via theme hook_theme_prepare_BASE_THEME_ID() was altered by theme hook_theme_prepare_BASE_THEME_ID_alter().');
   

   For all of these, leave the assert message off. They will then get a default assertion message of '"@text" found'. No need to duplicate, see WebTestBase::assertTextHelper().

8. +++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/ThemeTestController.php
   @@ -31,4 +31,16 @@ function functionTemplateOverridden() {
   +  function prepare() {
   +    return array(
   +      '#theme' => 'theme_test_prepare_variable',
   +    );
   +  }
   +
   +  function prepareAlter() {
   +    return array(
   +      '#theme' => 'theme_test_prepare_alter_variable',
   +    );
   +  }
   

   Looks like there should probably be a blank line before the prepare() method, and both of these need docblocks.

And the references I forgot in my review:

Reference for point #3 - https://drupal.org/node/1354#file.

Reference for point #8 - https://drupal.org/node/608152 and https://drupal.org/node/1354#classes.

Marking critical per @catch's comment in #1751194-86: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() (which this issue is dependent on). This issue also helps fix #939462: Specific preprocess functions for theme hook suggestions are not invoked.

Marking as an approved API change.

Updated issue summary.

Brushing the dust off this one. :)

This is simply a re-roll of #20 for the sake of having an updated patch that works against HEAD. Leaving as `needs work` and will incorporate suggestions from #25 with respective interdiff.

Here's suggestions from #25 implemented.

Per the Remaining Tasks, I also believe I addressed "Move the hook_preprocess() phase in between the hook_theme_prepare() and hook_theme_prepare_alter() phases (per discussion about this via Twig hangout meeting/IRC)"

Will take a look at this in the AM.

I guess bad things happen when you change variable names without grepping. :)

@Mark Carver, I'll try to resolve these in order to get this into an acceptable state before review.

Okay, let's try this one. Fixed the context variable names, also updated the routing file.

Added the concept of $context back.

Thanks @c4rl! Just adding an additional remain task: "Deprecate hook_preprocess_HOOK() in API doc."

Still need to finish out the additional tests too.

Going to be working on this today, hopefully to knock it out completely :)

Added documentation and deprecated hook_preprocess()

Updated issue summary.

Updated issue summary.

Here's an update to some of the theme() doc and also fixes the failing tests.

Updated issue summary.

Fixed $context and docs

#42: drupal-introduce-hook_theme_prepare_alter-2035055-42.patch queued for re-testing.

This patch is a great clean-up, is well documented and comes with tests. Awesome!

Regarding \Drupal::moduleHandler->themeInvoke(): Since it doesn't use any ModuleHandler object state, I would prefer if it were actually just a theme_invoke() procedural function for now. I know procedural functions suck, but sticking methods on stuff where it doesn't conceptually make sense sucks also. I didn't read the entire issue, so I'm not sure if this was discussed above already.

+++ b/core/modules/system/theme.api.php
@@ -150,14 +156,138 @@ function hook_preprocess(&$variables, $hook) {
+ * @see hook_theme_prepare_SUGGESTION()
...
+  // hook_theme_prepare_SUGGESTION().

SUGGESTION should be THEME_HOOK if I'm not mistaken.

Summarizing some discussion in IRC here. :)

There is some debate about a change introduced in #41 and #42 to how $context works. To me, the changes here seemed a digression from its intent as presented in the issue summary.

In #39, $context was simply an immutable array of a few internal parameters `'theme_id' => $hook, 'original_theme_id' => $original_hook, 'suggestions' => $suggestions`. It changed in #41 and #42 to be actually an argument for theme() and thus proposed to be user-defined/pluggable (even further via proposed ideas in https://gist.github.com/markcarver/6125045#file-theme-php-L20-L45).

I see this expansion of $context as potentially problematic for the following reasons:

• We have turned one container of information (i.e. $variables) into two (i.e. $variables and $context). So, themers/developers will have to grok two things instead of simply one. The $variables array may contain nested renderable arrays, which potentially have defined nested $context arrays. So, $variables isn't as theoretically clean as we might think.
• Though not yet implemented in the existing patches here, the changes proposed in https://gist.github.com/markcarver/6125045#file-theme-php-L20-L45 mean that RenderAPI implementations will have to change to accommodate the syntax. This likely requires rewriting every render array and hook_theme implementation in core, which seems like a lot of work, and I am hesitant to introduce further API changes to RenderAPI at this point that aren't explicitly fixing known bugs. Also, render arrays are generally alterable, and so if a #context parameter exists in a nested situation, it is not necessarily immutable.
• The motivation behind separating $variables and $context was acknowledged as being mostly motivated by separate issues for variable inspection, i.e. #1783134: [META] Make it possible to inspect twig variables and get information about objects and render arrays and #1804998: Add LadyBug from Symfony to Core to allow debugging variables within templates. I'm hesitant to convolve those motivations in this present issue.

Because of these reasons, we've decided in IRC that we should create a separate issue that would allow $context to be an argument (as proposed in #41 and #42, and not simply the few internal parameters it was in #39 i.e. `'theme_id' => $hook, 'original_theme_id' => $original_hook, 'suggestions' => $suggestions` (or some set of similar parameters).

Addressed conserns in #45 and created a temporary procedural function. Addressed concerns with #46 and removed all concepts of "$context" in theme() for a later issue. Updated documentation and this is now essentially a rename of preprocess (but allows the standardized "hook_" implementation through core, instead of the confusing template/theme/hook_ trifecta.

Issue summary will need to be updated, I'll have to do it later when/if I get a chance.

Also created the follow-up: #2121317: Introduce the "context" concept for theme system

Updated issue summary.

Updated issue summary.

Updated issue summary. Forgot to change test data when taking out $context. Will upload a new patch here shortly.

And this also still fixes #939462: Specific preprocess functions for theme hook suggestions are not invoked for 8.x. Do we have tests for that? If so they're not jumping out at me.

Fixed theme_invoke to pass by reference and fixed/added tests

Updated issue summary.

Updated issue summary.

+++ b/core/includes/theme.inc
@@ -997,10 +982,6 @@ function theme($hook, $variables = array()) {
-  // Supply original caller info.
-  $variables += array(
-    'theme_hook_original' => $original_hook,
-  );
   // Set base hook for later use. For example if '#theme' => 'node__article'
   // is called, we run hook_theme_suggestions_node_alter() rather than
@@ -1013,6 +994,12 @@ function theme($hook, $variables = array()) {

@@ -1013,6 +994,12 @@ function theme($hook, $variables = array()) {
     $base_theme_hook = $hook;
   }
+  // Supply original caller info.
+  $variables += array(
+    '#theme_hook' => $base_theme_hook,
+    '#theme_hook_original' => $original_hook,
+  );
+
   // Invoke hook_theme_suggestions_HOOK().

Seems like the change to using the `#` prefix on these params are making tests fail. Was the prefix intentional?

Nope, I think I accidently spaced what these were and added them out of habit. Here is the fix.

Updated issue summary.

Going to give this a review today (finally). :)

@c4rl, any chance you can get to this?

The changes look good to me, but I haven't had a chance to dig into the test coverage as much as I would like to. I can hopefully do this in the next week, but I'll unassign for now in case there is someone else who is available to give it a better review.

Needed a quick reroll.

Not sure about the test failures but I applied this patch regardless.

I tested this running the same hooks in a base and sub theme, e.g.:

basetheme_theme_prepare_html(&$variables)
basetheme_theme_prepare_html_alter(&$variables)
subtheme_theme_prepare_html(&$variables)
subtheme_theme_prepare_html_alter(&$variables)

Running all the above subtheme_theme_prepare_html() is not fired. If I remove basetheme_theme_prepare_html() then the sub theme starts firing. Is this something to do with the test failures (I haven't really looked hard at this yet)?

I suspect that's because we're not really using a true ThemeHandler here. This patch is adding theme_invoke().

+++ b/core/includes/theme.inc
@@ -707,6 +694,43 @@ function theme($hook, $variables = array()) {
+  // Invokes hook_theme_prepare(), hook_theme_prepare_THEME_HOOK() for the active
+  // theme, which takes precedence over modules.
+  // @todo Replace with Drupal::themeHandler()->invoke() once implemented.
+  // @see https://drupal.org/node/2029819
+  foreach ($prepare_hooks as $prepare_hook) {
+    theme_invoke($prepare_hook, $prepare_args);
+  }

Thanks @Jeff Burnz, you found a bug (and we found the fix for it on the Twig call)! So we're missing test coverage there, for now I will try to get this green.

This should fix the reported bug:

diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 0938b57..aa76705 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -1263,7 +1263,7 @@ function theme_invoke($hook, &$args = array()) {
     foreach ($theme_keys as $theme_key) {
       $function = $theme_key . '_' . $hook;
       if (function_exists($function)) {
-        return call_user_func_array($function, $args);
+        call_user_func_array($function, $args);
       }
     }
   }

The rest of the patch here is just tweaking the way the test assertions work, I found this method easier to debug and work with because you can see the list of hooks fired in order not just pipe separated.

I'm still expecting at least 2 fails because the specific suggestion hooks are no longer being picked up for theme_invoke(). Next patch will have some tests around the base theme/subtheme to make sure that bug has in fact been squashed.

(Migrating related issues from summary to related issues field as well.)

I'm not sure how that last patch is green, but moving on…

Main changes here:

• Added test coverage for the base theme + subtheme scenario described in #61. The -fail patch should demonstrate this bug.
• Added missing "&" to prepare hook implementations so they modify the variables by reference ;)
• Changed THEME_HOOK__SUGGESTION__SUGGESTION to THEME_HOOK__SUGGESTION because the extra bit is still a suggestion, just a longer one.
• Removed unused items from routing.

Patch ordering seems to be touchy on D7 d.o, I put the fail patch first.

we can replace @todo of #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler) with #2109287: Replace list_themes() with a service. which is RTBC.

Thanks @jibran. Going to rework this a bit now that #2109287: Replace list_themes() with a service. (and thus ThemeHandler) is in.

Because the patch is only adding code, and not converting any, it is unclear how this is a DX improvement. Can that be expanded upon in the issue summary, with a full code before/after example?

Here here on @tim.plunkett's comment in #70. I had a bit of a hard time grasping and trying to explain this in the twig talks I did in Van. Before/after would be amazing if the can explain the benefits we gain.

Added #70 as a task.

Agreed - and I can work on that, just a busy time of year.

65: 2035055-65.patch queued for re-testing.

Fixing up the test.

Passing this one back to @Mark Carver.

Going to take another run at this. Currently needs reroll.

Rerolled and took out the extra changes to theme_test.routing.yml, leaving them for the more modest #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter().

I read through #2004872: [meta] Theme system architecture changes again a few nights ago and the way forward is clear to me now. The patch was trying to introduce non-alter hooks to themes which is not related IMO and was actually making me not want to work on this issue. Also importantly, for consistency hook_theme_prepare() should return $variables instead of altering by reference.

This still needs more work but I wanted to put something up tonight. This just strips out the theme hook_theme_prepare implementations and updates/cleans up the tests. I will also be working on updating the issue summary and drafting a change notice in the near future. I'm still considering what makes the most sense to convert to the new API here but I will definitely incorporate that.

Got a bit excited with the tags.

This needed a small reroll, I'm hoping to have some time this weekend to get back to this in a more meaningful way.

I was able to make some progress but not ready to post quite yet.

After exploring hook_theme_prepare() as a hook_menu() style hook (and finding the downfalls of that approach), myself and some of the other theme system maintainers had some discussions about this and I can’t speak for everyone but in general we are now favouring a more minimal approach which would maintain the current preprocess system while still providing the same capabilities as theme hook suggestion-specific preprocess/prepare hooks.

Following our discussions and after re-reviewing #939462: Specific preprocess functions for theme hook suggestions are not invoked again (@effulgentsia's comment in #939462-6: Specific preprocess functions for theme hook suggestions are not invoked stood out in particular), we would favour the following approach for allowing suggestion-specific variable preparation/preprocessing:

#2201781: Pass all theme hook suggestions to theme preprocess functions to allow for suggestion-specific overrides

I think this is a better and more flexible API than trying to build a stack of separate suggestion-specific prepare/preprocess functions and deciding in which order and circumstances they should run, something that gets even more complicated when you have theme_hook_suggestions__that__can__cascade__down. After going through some architecture and use cases, we have determined the method proposed in the above issue gives both module developers and advanced themers the power they need to perform suggestion-specific variable preparation as necessary. This approach still paves the way for template consolidation as well.

Having said all that, I'm bumping this to 9.x in favour of the above issue.

I'm moving focus back to this issue because of my comment in #2201781-9: Pass all theme hook suggestions to theme preprocess functions to allow for suggestion-specific overrides. Also the related issue I'm attaching now explains in more detail why using this approach is not an acceptable solution.

@todos:
We need to pass suggestions in the $info array.

Also if, in the spirit of time, we need to keep the preprocess name for this issue to progress quickly (no API change), then I think that is an acceptable (albeit very unfortunate) compromise. The underlying "fix" in this patch is still valuable, regardless of which hook implements it.

Given the short deadline, it is highly unlikely that this will get done (including all the necessary conversions in core). Moving this to be addressed in 9.x (if still necessary) and moving focus back to #939462: Specific preprocess functions for theme hook suggestions are not invoked.

We should see whether any of this is viable for a minor version with a bc layer.

I don't see the benefit of renaming this for the 8.x lifecycle and would suggest keeping this in 9.x. It's a hook rename, we shouldn't be creating variables in preprocess nor prepare. It's an alter hook to change or alter them before they hit the template. It should also be said that variables changing via preprocess should in theory be very rare and ideally not at all in core (save for tests to make sure it works).

Sigh... despite all the work I did on this issue and as sad as I am to see this issue get sideswiped, it may have been for the best.

Closing in favor of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers.