Updated: Comment #70

Issue History

#939462: Specific preprocess functions for theme hook suggestions are not invoked was started as a D7 issue, and is now a D8 issue (and assuming an eventual D7 backport). D8 fixes were partially addressed in #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter(). Most of the following was discussed with D8 in mind, but the decision was made to push the changes into D9.

Problem/Motivation

All changes to $variables that will be printed in templates currently happen in the same place, preprocess. Things that happen in preprocess include both creating initial variables and then altering those variables later. There is also inconsistency and confusion (for new themers) on when to use the module/theme name for hook_ prefixing or the very ambiguous template_ prefix.

Proposed resolution

Use a standard hook naming convention, exactly like the FAPI does. These standardized hooks would allow the theme system's phases to be split into a more systemic and easier to understand process. The two "phases" to add would be:

  1. A phase where the variables that are needed are prepared (created).
  2. A phase where anyone (modules or themes) can alter existing variables.

Remaining tasks

  • Add new prepare hooks.
  • Add new prepare alter hooks.
  • Create temporary tests for all the new hooks.
  • Add new temporary method (Drupal::moduleHandler()->themeInvoke()) to invoke hooks on active theme, similar to how Drupal::moduleHandler()->alter() currently works.
  • Keep backwards compatibility support for hook_preprocess() by allowing it to run after the new API. Once everything is converted over, we don't have to worry about hook_preprocess() being last.
  • 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
  • Deprecate hook_preprocess_HOOK() in API doc.
  • Document hook_theme_prepare() in theme.api.php
  • Add/change/remove subsections in theme() docblock.
  • Add relevant @todo comments for cleanup later.
  • Create test for hook_theme_prepare_THEME_HOOK__SUGGESTION().
  • Add before/after example code in issue summary to demonstrate the benefits of this refactoring. (per #70)

User interface changes

None.

API changes

None.

API additions

Technically this is just one new "hook" with variations for allowing specific suggestion targeting. These specific hooks are invoked based on which template was used. For example array('#theme' => 'node__page', ...), has two different THEME_HOOKs. Two different prepare functions are called: hook_theme_prepare_node() and hook_theme_prepare_node__page().

  • hook_theme_prepare(&$variables, $hook)
  • (and in a loop) hook_theme_prepare_THEME_HOOK(&$variables)

All hooks can be appended with _alter for the altering "phase". This phase alters existing $variables (added by the first phase above) by reference.

  • hook_theme_prepare_alter(&$variables, $hook)
  • (and in a loop) hook_theme_prepare_THEME_HOOK_alter(&$variables)

Mapping of Old API to New API

hook[template]_preprocess = hook_theme_prepare[_alter]
hook[template]_preprocess_THEME_HOOK = hook_theme_prepare_THEME_HOOK[_alter]
hook[template]_preprocess_THEME_HOOK__SUGGESTION = hook_theme_prepare_THEME_HOOK[_alter]

The last mapping is not actually not supported in old API. This issue would fix an existing critical bug for D8: #939462: Specific preprocess functions for theme hook suggestions are not invoked.

Files: 
CommentFileSizeAuthor
#85 2035055-85.patch31.76 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 63,787 pass(es). View

Comments

markcarver’s picture

Assigned: Unassigned » markcarver
Fabianx’s picture

Issue summary: View changes

Steal issue and add more explanation for new API

markcarver’s picture

Status: Active » Needs review
FileSize
5.72 KB
8.39 KB
FAILED: [[SimpleTest]]: [MySQL] 55,906 pass(es), 78 fail(s), and 85 exception(s). View

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.

Status: Needs review » Needs work
Issue tags: -API change, -theme system cleanup

The last submitted patch, drupal-introduce-hook_theme_prepare-2035055-2.patch, failed testing.

markcarver’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +API change, +theme system cleanup

The last submitted patch, drupal-introduce-hook_theme_prepare-2035055-2.patch, failed testing.

Fabianx’s picture

Nice start!

markcarver’s picture

Status: Needs work » Needs review
FileSize
670 bytes
9.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,151 pass(es). View

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

Fabianx’s picture

+++ 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.

jenlampton’s picture

I vote for $hook, $variables everywhere :)

Fabianx’s picture

#9: Even for FAPI?

markcarver’s picture

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.

markcarver’s picture

Issue summary: View changes

typo

markcarver’s picture

Priority: Major » Normal
Status: Needs work » Needs review

Updated issue summary (edit: per IRC conversations).

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Title: Introduce hook_theme_prepare(), and hook_theme_HOOK_prepare() » Introduce hook_theme_prepare() and hook_theme_THEME_HOOK_prepare() with _alter
Priority: Normal » Major

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

markcarver’s picture

Status: Needs review » Needs work

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

markcarver’s picture

Priority: Normal » Major
Status: Needs review » Needs work
FileSize
9.13 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Reroll

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Title: Introduce hook_theme_prepare() and hook_theme_THEME_HOOK_prepare() with _alter » Introduce hook_theme_prepare[_alter](), hook_theme_prepare_BASE_THEME_ID[_alter]() and hook_theme_prepare_THEME_ID[_alter]()

Updating title to reflect issue summary.

markcarver’s picture

Status: Needs work » Needs review
FileSize
12.45 KB
15.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,616 pass(es), 0 fail(s), and 966 exception(s). View

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

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

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Title: Introduce hook_theme_prepare[_alter](), hook_theme_prepare_BASE_THEME_ID[_alter]() and hook_theme_prepare_THEME_ID[_alter]() » Introduce hook_theme_prepare() phase

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.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, drupal-introduce-hook_theme_prepare_alter-2035055-17.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Status: Needs work » Needs review
FileSize
910 bytes
15.2 KB
PASSED: [[SimpleTest]]: [MySQL] 57,730 pass(es). View

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).

markcarver’s picture

Issue summary: View changes

Updated issue summary.

seanr’s picture

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.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Issue summary update.

markcarver’s picture

Priority: Critical » Major
Status: Needs work » Needs review
Issue tags: -Approved API change

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).

markcarver’s picture

Issue summary: View changes

correct issue

jenlampton’s picture

Issue summary: View changes

links to issues

jenlampton’s picture

Issue summary: View changes

who

seanr’s picture

Status: Needs review » Reviewed & tested by the community

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

markcarver’s picture

Status: Reviewed & tested by the community » Needs work

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

markcarver’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

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.

Cottser’s picture

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.

markcarver’s picture

Priority: Major » Critical
webchick’s picture

Issue tags: +Approved API change

Marking as an approved API change.

webchick’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

Issue summary: View changes

updated terminology

jenlampton’s picture

Issue summary: View changes

loop!

jenlampton’s picture

Issue summary: View changes

update

c4rl’s picture

Title: Introduce hook_theme_prepare() phase » Introduce hook_theme_prepare[_alter]()
Assigned: markcarver » c4rl
Priority: Major » Critical
Status: Needs review » Needs work
Issue tags: +Approved API change

Brushing the dust off this one. :)

c4rl’s picture

FileSize
14.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,959 pass(es), 8 fail(s), and 140 exception(s). View

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.

c4rl’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
8.82 KB
13.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,855 pass(es), 8 fail(s), and 11,474 exception(s). View

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)"

markcarver’s picture

Assigned: c4rl » markcarver

Will take a look at this in the AM.

Status: Needs review » Needs work

The last submitted patch, drupal-2035055-31.patch, failed testing.

c4rl’s picture

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

c4rl’s picture

Assigned: markcarver » c4rl

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

c4rl’s picture

Status: Needs work » Needs review
FileSize
5.49 KB
14.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,135 pass(es). View

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

c4rl’s picture

Issue summary: View changes

Added the concept of $context back.

markcarver’s picture

Title: Introduce hook_theme_prepare[_alter]() » Introduce hook_theme_prepare[_alter]() and deprecate hook_preprocess_HOOK()
Status: Needs review » Needs work

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

Still need to finish out the additional tests too.

markcarver’s picture

Assigned: c4rl » markcarver

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

markcarver’s picture

Status: Needs work » Needs review
FileSize
7.88 KB
21.69 KB
FAILED: [[SimpleTest]]: [MySQL] 59,515 pass(es), 4 fail(s), and 11,249 exception(s). View

Added documentation and deprecated hook_preprocess()

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, drupal-introduce-hook_theme_prepare_alter-2035055-39.patch, failed testing.

markcarver’s picture

Status: Needs work » Needs review
FileSize
9.18 KB
26.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,065 pass(es). View

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

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

FileSize
3.9 KB
26.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,512 pass(es). View

Fixed $context and docs

Status: Needs review » Needs work
Issue tags: -Needs tests, -API change, -theme system cleanup, -Approved API change

The last submitted patch, drupal-introduce-hook_theme_prepare_alter-2035055-42.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +API change, +theme system cleanup, +Approved API change
tstoeckler’s picture

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.

c4rl’s picture

Status: Needs review » Needs work

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).

markcarver’s picture

Status: Needs work » Needs review
FileSize
15.89 KB
27.19 KB
FAILED: [[SimpleTest]]: [MySQL] 59,534 pass(es), 2 fail(s), and 0 exception(s). View

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.

markcarver’s picture

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

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, drupal-introduce-hook_theme_prepare_alter-2035055-47.patch, failed testing.

markcarver’s picture

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

Cottser’s picture

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.

markcarver’s picture

Status: Needs work » Needs review
FileSize
23.83 KB
35.08 KB
FAILED: [[SimpleTest]]: [MySQL] 59,661 pass(es), 21 fail(s), and 69 exception(s). View

Fixed theme_invoke to pass by reference and fixed/added tests

markcarver’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, drupal-introduce-hook_theme_prepare_alter-2035055-52.patch, failed testing.

c4rl’s picture

+++ 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?

markcarver’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
34.99 KB
PASSED: [[SimpleTest]]: [MySQL] 59,673 pass(es). View

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

markcarver’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Assigned: markcarver » c4rl
Issue summary: View changes

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

markcarver’s picture

@c4rl, any chance you can get to this?

c4rl’s picture

Assigned: c4rl » Unassigned

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.

Cottser’s picture

FileSize
34.11 KB
FAILED: [[SimpleTest]]: [MySQL] 59,103 pass(es), 25 fail(s), and 0 exception(s). View

Needed a quick reroll.

Status: Needs review » Needs work

The last submitted patch, 59: 2035055-59.patch, failed testing.

Jeff Burnz’s picture

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)?

Cottser’s picture

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);
+  }
Cottser’s picture

Assigned: Unassigned » Cottser

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.

Cottser’s picture

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.)

Cottser’s picture

FileSize
18.36 KB
40.28 KB
FAILED: [[SimpleTest]]: [MySQL] 60,107 pass(es), 25 fail(s), and 0 exception(s). View
40.28 KB
FAILED: [[SimpleTest]]: [MySQL] 59,001 pass(es), 2 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 65: 2035055-65-fail.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review

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

jibran’s picture

Cottser’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

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?

joelpittet’s picture

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.

jwilson3’s picture

Issue summary: View changes

Added #70 as a task.

Cottser’s picture

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

Cottser’s picture

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

The last submitted patch, 65: 2035055-65.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
40.28 KB
PASSED: [[SimpleTest]]: [MySQL] 60,202 pass(es). View
740 bytes

Fixing up the test.

Cottser’s picture

Assigned: Cottser » markcarver

Passing this one back to @Mark Carver.

Cottser’s picture

Assigned: markcarver » Cottser
Status: Needs review » Needs work

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

Cottser’s picture

Status: Needs work » Needs review
FileSize
39.59 KB
PASSED: [[SimpleTest]]: [MySQL] 62,986 pass(es). View
1.38 KB

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().

Cottser’s picture

Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
FileSize
31.8 KB
PASSED: [[SimpleTest]]: [MySQL] 63,700 pass(es). View
20.07 KB

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.

Cottser’s picture

Status: Needs review » Needs work
Cottser’s picture

Issue tags: +Twig, +sprint, +theme system ar
Cottser’s picture

Issue tags: -theme system ar

Got a bit excited with the tags.

Cottser’s picture

Status: Needs work » Needs review
FileSize
31.76 KB
PASSED: [[SimpleTest]]: [MySQL] 63,787 pass(es). View

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

Cottser’s picture

Status: Needs review » Needs work

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

Cottser’s picture

Version: 8.x-dev » 9.x-dev
Assigned: Cottser » Unassigned
Priority: Critical » Normal
Status: Needs work » Postponed
Issue tags: -Approved API change, -sprint

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.

markcarver’s picture

Version: 9.x-dev » 8.x-dev
Priority: Normal » Critical
Status: Postponed » Needs work
Issue tags: +sprint
Related issues: +#2223885: Detect standalone hook_[pre]process_THEME_HOOK__SUGGESTION implementations

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.

markcarver’s picture

Title: Introduce hook_theme_prepare[_alter]() and deprecate hook_preprocess_HOOK() » Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK()
Version: 8.x-dev » 9.x-dev
Priority: Critical » Major
Issue tags: -sprint

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.

markcarver’s picture

Cottser’s picture

Issue tags: -Twig
akalata’s picture

Issue summary: View changes
catch’s picture

Version: 9.x-dev » 8.1.x-dev
Status: Needs work » Postponed

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

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).

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.