Updated: Comment #177

Problem/Motivation

The preprocess phase is responsible for too many aspects of the theme system, including providing alternate theme hook suggestions. This makes things tangled and makes critical issues like #939462: Specific preprocess functions for theme hook suggestions are not invoked more difficult to solve than they should be. Ideal solution discussed at #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() has been postponed to D9.

Proposed resolution

Move the creating/altering of theme hook suggestions to separate hooks, and handle the altering of theme hook suggestions before any preprocess functions are called.

Remaining tasks

User interface changes

n/a

API changes

New hooks introduced:

  • hook_theme_suggestions_HOOK(array $variables)
  • hook_theme_suggestions_HOOK_alter(array &$suggestions, array $variables)

Removed the ability to change template suggestions in preprocess functions by altering $variables['theme_hook_suggestion'] and $variables['theme_hook_suggestions'].

Code that altered $variables['theme_hook_suggestions'] or $variables['theme_hook_suggestion'] in preprocess needs to be moved to suggestion hooks:

From the module that is providing the theme hook (implementing hook_theme()):

Suggestions code is moved from template_preprocess_HOOK() to returning an array in hook_theme_suggestions_HOOK().

Before:

/**
 * Prepares variables for search results templates.
 */
function template_preprocess_search_results(&$variables) {
  $variables['theme_hook_suggestions'][] = 'search_results__' . $variables['plugin_id'];
}

After:

/**
 * Implements hook_theme_suggestions_HOOK().
 */
function search_theme_suggestions_search_results(array $variables) {
  return array('search_results__' . $variables['plugin_id']);
}

Any other module or theme:

Suggestions code is moved from HOOK_preprocess_HOOK() to manipulating the suggestions array in hook_theme_suggestions_HOOK_alter().

Before:

/**
 * Implements hook_preprocess_HOOK() for node templates.
 */
function MYTHEME_preprocess_node($variables) {
  $variables['theme_hook_suggestions'][] = 'node__' . 'my_first_suggestion';
  $variables['theme_hook_suggestions'][] = 'node__' . 'my_second_suggestion';
}

After:

/**
 * Implements hook_theme_suggestions_HOOK_alter().
 */
function MYTHEME_theme_suggestions_node_alter(array &$suggestions, array $variables) {
  $suggestions[] = 'node__' . 'my_first_suggestion';
  $suggestions[] = 'node__' . 'my_second_suggestion';
}

Altering $variables['theme_hook_suggestion'] (singular) is now just a matter of adding the suggestion to the end of the suggestions array in hook_theme_suggestions_HOOK_alter(). If you need to ensure that your suggestion is at the end, implement hook_module_implements_alter().

Before:

/**
 * Implements hook_preprocess_HOOK() for node templates.
 */
function MYTHEME_preprocess_node($variables) {
  $variables['theme_hook_suggestion'] = 'node__' . 'my_suggestion';
}

After:

/**
 * Implements hook_theme_suggestions_HOOK_alter().
 */
function MYTHEME_theme_suggestions_node_alter(array &$suggestions, array $variables) {
  $suggestions[] = 'node__' . 'my_suggestion';
}

#939462: Specific preprocess functions for theme hook suggestions are not invoked
#2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK()
#2004872: [meta] Theme system architecture changes

Original report by @mikl

Currently, we have the chicken-and-egg situation, where what template preprocessors to run are determined by the theme hook suggestions, which are set in template preprocessors.

That means that it's currently not possible to:

  1. Suggest a new template for nodes – say node--thursday
  2. Have the theme layer run template_preprocess_node__thursday

This problem has also been discussed in #939462: Specific preprocess functions for theme hook suggestions are not invoked.

In this issue, we propose to solve this by making suggestions returned in a separate (and earlier) phase than preprocess.

Effulgentsia gave me this task on the DrupalCon München sprint, he will probably be able to elaborate further.

CommentFileSizeAuthor
#146 1751194-146.patch51.04 KBstar-szr
#146 interdiff.txt1.32 KBstar-szr
#143 1751194-143.patch50.49 KBstar-szr
#143 interdiff.txt3.17 KBstar-szr
#141 1751194-141.patch50.67 KBstar-szr
#141 interdiff.txt3.63 KBstar-szr
#139 1751194-139.patch49.79 KBstar-szr
#133 1751194-133.patch49.47 KBstar-szr
#133 interdiff.txt5.92 KBstar-szr
#130 1751194-130.patch49.9 KBstar-szr
#130 interdiff.txt25.93 KBstar-szr
#129 1751194-129.patch46.25 KBstar-szr
#82 1751194-82.patch45.76 KBstar-szr
#78 1751194-78.patch45.8 KBstar-szr
#78 interdiff.txt1.08 KBstar-szr
#76 1751194-76.patch45.78 KBstar-szr
#68 1751194-68.patch45.82 KBstar-szr
#68 interdiff.txt814 bytesstar-szr
#67 1751194-67.patch45.75 KBstar-szr
#61 1751194-61.patch46.2 KBstar-szr
#61 interdiff.txt1.28 KBstar-szr
#60 1751194-60-tests.patch14.46 KBstar-szr
#60 1751194-60.patch46.49 KBstar-szr
#60 interdiff.txt8.87 KBstar-szr
#59 1751194-59.patch43.58 KBstar-szr
#59 interdiff.txt2.39 KBstar-szr
#54 1751194-54.patch45.47 KBstar-szr
#54 interdiff.txt3.43 KBstar-szr
#49 1751194-49.patch44.79 KBstar-szr
#49 interdiff.txt3.32 KBstar-szr
#46 1751194-46.patch42.68 KBstar-szr
#43 1751194-43.patch42.56 KBbenjifisher
#39 1751194-39.patch42.39 KBstar-szr
#39 interdiff.txt3.72 KBstar-szr
#38 1751194-38.patch41.33 KBstar-szr
#35 1751194-35.patch41.13 KBstar-szr
#35 interdiff.txt3.36 KBstar-szr
#33 interdiff.txt10.49 KBstar-szr
#33 1751194-33.patch39.59 KBstar-szr
#33 1751194-33-test-only.patch10.49 KBstar-szr
#32 1751194-32.patch29.09 KBstar-szr
#29 interdiff.txt10.88 KBstar-szr
#29 1751194-29.patch29.07 KBstar-szr
#27 1751194-27.patch27.54 KBstar-szr
#27 interdiff.txt4.04 KBstar-szr
#26 1751194-26-fix-views-preprocess.patch25.64 KBstar-szr
#26 interdiff.txt743 bytesstar-szr
#26 1751194-26.patch27.71 KBstar-szr
#26 interdiff2.txt2.85 KBstar-szr
#24 1751194-24.patch25.64 KBstar-szr
#24 interdiff.txt9.83 KBstar-szr
#22 1751194-22-do-not-test.patch18.79 KBstar-szr
#9 1751194-introduce-hook_template_suggestions_hook-9.patch22.64 KBmikl
#7 1751194-introduce-hook_template_suggestions_hook-7.patch19.52 KBeffulgentsia
#2 1751194-introduce-hook_template_suggestions_hook-2.patch19.68 KBmikl
#1 1751194-introduce-hook_template_suggestions_hook.patch16.04 KBmikl
introduce-hook_template_suggestions_hook.patch4.53 KBmikl
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikl’s picture

Here's an updated version of the patch. We're now (ab)using variable_process_phases to invoke hook_template_suggestions_HOOK instead of a manual process.

Additionally, I have gone through all the core modules and moved their suggestions to implementation of the new hook.

The template suggestions introduced in theme.inc still need to be moved to system.module.

mikl’s picture

Removed the final `theme_hook_suggestion` stuff from theme.inc.

mikl’s picture

Status: Needs work » Needs review

This should ostensibly work without breaking things :)

Status: Needs review » Needs work

The last submitted patch, 1751194-introduce-hook_template_suggestions_hook-2.patch, failed testing.

mikl’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1751194-introduce-hook_template_suggestions_hook-2.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
19.52 KB

reroll and minor fix.

Status: Needs review » Needs work

The last submitted patch, 1751194-introduce-hook_template_suggestions_hook-7.patch, failed testing.

mikl’s picture

Status: Needs work » Needs review
FileSize
22.64 KB

Another reroll, trying to fix some of the failing tests.

Status: Needs review » Needs work

The last submitted patch, 1751194-introduce-hook_template_suggestions_hook-9.patch, failed testing.

mikl’s picture

Status: Needs work » Needs review

@effulgentsia what did you change? the hook_template_suggestions_HOOK implementations no longer get the correct variables (ie. what would go into the templates) but rather a render array like this:

array (
  'elements' => array (
    '#pre_render' => array (
      0 => '_field_extra_fields_pre_render',
    ),
    '#entity_type' => 'node',
    '#bundle' => 'page',
    '#theme' => 'node',
    '#node' => Drupal\node\Node::__set_state(array( ['THE ACTUAL node object.']

That obviously causes a lot of errors :/

effulgentsia’s picture

barraponto’s picture

Status: Needs review » Needs work

This needs work, right? Patch is not "review" quality ;)

kscheirer’s picture

Yeah, this is colliding with some of the TWIG template code that's going in. Can someone figure out how to reroll this? There's a test in #939462: Specific preprocess functions for theme hook suggestions are not invoked we can use for this case too.

Fabianx’s picture

Issue tags: +Twig

Tagging with Twig, so I can find this again

Fabianx’s picture

Issue tags: +Performance

First I was opposed to this, but now it makes much sense.

We need to have this information during the building of the registry to be performance efficient.

Maybe we need rather an _info hook, because I also need to add some information into the mix for render arrays and we can probably find more ...

catch’s picture

Fabianx’s picture

Status: Needs work » Closed (duplicate)

Yeah, lets please move the patch over to #939462: Specific preprocess functions for theme hook suggestions are not invoked. It is a bug fix and not a feature request.

star-szr’s picture

Status: Closed (duplicate) » Needs work

I'd like to revive this issue as part of #2004872: [meta] Theme system architecture changes, so re-opening.

star-szr’s picture

Title: Introduce hook_template_suggestions_HOOK to separate suggestions from variable preprocessing » Introduce hook_theme_suggestions_alter() and hook_theme_suggestions_THEME_ID_alter()

And re-titling to reflect the hook names proposed in #2004872: [meta] Theme system architecture changes.

star-szr’s picture

Assigned: Unassigned » star-szr

Working on this, should have a new patch up tomorrow.

star-szr’s picture

FileSize
18.79 KB

Not quite ready for testbot yet but here is a rough work-in-progress patch.

This is based on #9 and reworked to match what has been proposed in #2004872: [meta] Theme system architecture changes. There are some theme_hook_suggestions in Views and Views UI that need to be converted still. I'm hoping to have a patch for testbot by the end of the weekend.

star-szr’s picture

Category: feature » task
star-szr’s picture

Title: Introduce hook_theme_suggestions_alter() and hook_theme_suggestions_THEME_ID_alter() » Introduce hook_theme_suggestions_alter() and hook_theme_suggestions_HOOK_alter()
Status: Needs work » Needs review
FileSize
9.83 KB
25.64 KB

Plenty of @todos left but let's see what happens.

Status: Needs review » Needs work

The last submitted patch, 1751194-24.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
2.85 KB
27.71 KB
743 bytes
25.64 KB

First patch fixes the failing test - removed one line too many when moving things around.

Second patch no longer adds 'theme_hook_suggestions' to the $variables array (this felt wrong), instead it passes $suggestions to the ENGINE_render_template() function. This way we can still display the theme suggestions in Twig debug output.

star-szr’s picture

FileSize
4.04 KB
27.54 KB

This fixes up the Views suggestions hook implementations and makes HOOK the hook implementation that is actually used. So if theme('comment__node_article', …) is called, hook_theme_suggestions_comment_alter() is invoked instead of hook_theme_suggestions_comment__node_article(). You can still check $variables['theme_hook_original'] to get e.g. 'comment__node_article'.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Tagging for tests, and more work.

star-szr’s picture

FileSize
29.07 KB
10.88 KB

Now passing the derived hook to hook_theme_suggestions_alter(), casting alter hook params to arrays, and adding initial version of API docs.

I also created a Twig debug bugfix issue as a result of working on this patch: #2029225: Twig debug output does not show the base or derived hooks when a theme suggestion is called directly

Still working on tests so leaving at CNW for now.

jenlampton’s picture

Category: task » bug
Priority: Normal » Critical

Wow, this is looking great. I wish I were better at writing tests, but let me know if there's anything else I can do help here :)

Also, changing to a critical bug since this will solve #939462: Specific preprocess functions for theme hook suggestions are not invoked

Marking that issue as a dupe of this one.

star-szr’s picture

I'm back onto this over the next few days.

star-szr’s picture

Status: Needs work » Needs review
FileSize
29.09 KB

Reroll first, small update needed in node_theme_suggestions_node_alter() to catch up with HEAD.

star-szr’s picture

FileSize
10.49 KB
39.59 KB
10.49 KB

Tests added to verify the ability of both modules and themes to alter suggestions for template files and theme functions.

Status: Needs review » Needs work

The last submitted patch, 1751194-33-test-only.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.36 KB
41.13 KB

I will do the routerification and controllerification of the test callbacks once #2030457: Warning: Missing argument 3 for template_preprocess() for theme functions overridden by templates lands.

Other than that, here are some needed documentation updates. This is ready for review.

I'm still trying to understand the implications of #939462: Specific preprocess functions for theme hook suggestions are not invoked, read through the whole issue this morning. As it is, this patch does not solve the problem(s) in that issue. I think it would make the problem much easier to solve, though.

star-szr’s picture

Status: Needs review » Needs work

#2030457: Warning: Missing argument 3 for template_preprocess() for theme functions overridden by templates is in so this needs a small update. That will happen tonight or tomorrow.

star-szr’s picture

Category: bug » task
Priority: Critical » Major

Ah, and @tim.plunkett re-opened #939462: Specific preprocess functions for theme hook suggestions are not invoked which reminded me that it should probably still be postponed since the test case in #939462-96: Specific preprocess functions for theme hook suggestions are not invoked still fails with this patch. More code would be needed (maybe part of or after #1886448: Rewrite the theme registry into a proper service?) to solve the preprocess issue.

So reverting status based on the fact that this is not (yet) fixing critical bugs.

star-szr’s picture

Status: Needs work » Needs review
FileSize
41.33 KB

First a reroll, no changes.

star-szr’s picture

FileSize
3.72 KB
42.39 KB

Converted test page callbacks to controller methods and routes.

Status: Needs review » Needs work
Issue tags: -Performance, -Twig, -Theme Component Library

The last submitted patch, 1751194-39.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Performance, +Twig, +Theme Component Library

#39: 1751194-39.patch queued for re-testing.

benjifisher’s picture

Assigned: star-szr » benjifisher
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch from #39 does not apply locally. I will reroll it.

benjifisher’s picture

Assigned: benjifisher » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
42.56 KB

OK, it was pretty simple to reroll the patch. The only conflict was c094757 from #1963942: Change all instances of $vars to $variables. Update attached.

It seems that HEAD is broken, so I expect some tests to fail ...

I am reviewing the patch manually.

benjifisher’s picture

Status: Needs review » Needs work

Generally, the changes look good. I like replacing the verbose usage
$variables['theme_hook_suggestions'][] = ...
with
$suggestions[] = ...
I do have some suggestions for improvement:


+++ b/core/includes/theme.inc
@@ -1050,14 +1065,15 @@ function theme($hook, $variables = array()) {
+    foreach (array('preprocess functions', 'process functions') as $processor) {
+      if (isset($base_hook_info[$processor])) {
+        $info = array_merge($info, array($processor => $base_hook_info[$processor]));

Why use array_merge()? AFAICT, this has the same effect as

$info[$processor] = $base_hook_info[$processor]);

The lines
  // Dead databases will show error messages so supplying this template will
  // allow themers to override the page and the content completely.
  if (isset($variables['db_is_active']) && !$variables['db_is_active']) {
    $variables['theme_hook_suggestion'] = 'maintenance_page__offline';
  }

in template_preprocess_maintenance_page() have been replaced by

  // Dead databases will show error messages so supplying this template will
  // allow themers to override the page and the content completely.
  if (defined('MAINTENANCE_MODE')) {
    $suggestions[] = 'maintenance_page__offline';
  }

in system_theme_suggestions_maintenance_page_alter(). This seems to miss the case described in the comment:

  // drupal_is_front_page() might throw an exception.
  try {
    $variables['is_front'] = drupal_is_front_page();
  }
  catch (Exception $e) {
    // If the database is not yet available, set default values for these
    // variables.
    $variables['is_front'] = FALSE;
    $variables['db_is_active'] = FALSE;
  }

(from _template_preprocess_default_variables()). I think you could add the try/catch statements to template_preprocess_maintenance_page().


The lines
  if ($id = $variables['elements']['#block']->id()) {
    $config_id = explode('.', $id);
    $machine_name = array_pop($config_id);
    $suggestions[] = 'block__' . $machine_name;
  }

in block_theme_suggestions_block_alter() were copied from template_preprocess_block(), leading to a few lines of code duplication. I am afraid that, down the line, one of these will be modified and then the CSS id attribute will be out of sync with the template suggestion. Maybe refactor those few lines into a separate _block_*() function?


The lines
    // Test adding a class to the block content.
    $variables['content_attributes']['class'][] = 'test-class';
    template_preprocess_block($variables);
    $this->assertEqual($variables['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes');

were removed from testBlockThemeHookSuggestions(). I agree that they do not belong there, but perhaps they should be added to another test class, or maybe we need a new test class.

star-szr’s picture

Assigned: Unassigned » star-szr

Thanks very much @benjifisher!

I'm looking into all your points but also noticing that this needs another reroll so I'm going to start with that.

star-szr’s picture

Status: Needs work » Needs review
FileSize
42.68 KB
star-szr’s picture

That's all I can manage for today but I will come back to this in a day or two unless someone else wants to grab it.

star-szr’s picture

Status: Needs review » Needs work
star-szr’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
44.79 KB

Points #1 and #2 from comment #44 have been fixed. For #3 I asked in #drupal-contribute and then opened #2043527: Theme name is included in block machine name but should be stored as a key instead so I consider that taken care of :)

For #4 (testing the block classes), I just did a copy and paste into a new test class, couldn't find anything similar.

Fabianx’s picture

Status: Needs review » Needs work
Issue tags: -Performance +needs profiling

Looks very great to me already! (I spent hours on that already way back after DrupalCon and this solution is really nice. I probably wanted to do too much (tm).)

But I found some things that unfortunately need a little work:

+++ b/core/includes/theme.incundefined
@@ -1036,11 +1035,27 @@ function theme($hook, $variables = array()) {
+  $suggestion_hooks = array(
+    'theme_suggestions',
+    'theme_suggestions_' . $hook,
+  );
+  Drupal::moduleHandler()->alter($suggestion_hooks, $suggestions, $variables, $hook);

This looks right, but it is not :-).

Consider calling

#theme => 'node__article'

which would call

hook_theme_suggestions_node__article_alter instead of hook_theme_suggestions_node_alter as expected

if node--article.html.twig exists.

I think however using the $info['base hook'] should work here.

+++ b/core/includes/theme.incundefined
@@ -1051,14 +1066,15 @@ function theme($hook, $variables = array()) {
+    foreach (array('preprocess functions', 'process functions') as $processor) {
+      if (isset($base_hook_info[$processor])) {
+        $info[$processor] = $base_hook_info[$processor];
+      }

Is it correct to _replace_ the $processor functions here instead of add to it?

+++ b/core/includes/theme.incundefined
@@ -1155,7 +1146,7 @@ function theme($hook, $variables = array()) {
-    $output = $render_function($template_file, $variables);
+    $output = $render_function($template_file, $variables, $suggestions);

This is nice.

However: Is this an API change?

FWIW, I would be fine with adding

$variables['theme_hook_suggestions'] = $suggestions;

at this point to keep some BC here.

+++ b/core/modules/field/field.moduleundefined
@@ -924,6 +924,20 @@ function field_page_build(&$page) {
+  $suggestions = array_merge($suggestions, array(
+    'field__' . $element['#field_type'],
+    'field__' . $element['#field_name'],
+    'field__' . $element['#bundle'],
+    'field__' . $element['#field_name'] . '__' . $element['#bundle'],

array_merge seems slower here (1 additional function call) than adding the suggestions to the end.

As they are used within reverse priority order, just adding to it, should be fine.

+++ b/core/modules/node/node.moduleundefined
@@ -667,6 +667,18 @@ function node_preprocess_block(&$variables) {
+  $suggestions = array_merge($suggestions, array(
+    'node__' . $node->bundle(),

Again, an unneeded function call.

Please keep this up, this is a great start!

markhalliwell’s picture

+++ b/core/includes/theme.incundefined
@@ -1155,7 +1146,7 @@ function theme($hook, $variables = array()) {
-    $output = $render_function($template_file, $variables);
+    $output = $render_function($template_file, $variables, $suggestions);

This is nice.

However: Is this an API change?

Even though this technically is an API change, it's a necessary one if we're going to introduce these hooks. We really only want one place to alter theme suggestions, we don't need two. If we don't take out $variables['theme_hook_suggestions'] then (in theory) this would introduce a lot of cruft in contrib modules: some would add via these hooks and others would still add them via $variables['theme_hook_suggestions'].... let's not do that please :)

markhalliwell’s picture

Scratch my above comment, misunderstood.

Fabianx’s picture

I absolutely agree that there should be only one way to define suggestions, that is a necessary change.

I asked if the change to the engine definition is necessary and that templates don't have $vars['theme_hook_suggestions'] available anymore.

Both are rather big and unnecessary changes as that line could be replaced with:

from:

-    $output = $render_function($template_file, $variables);
+    $output = $render_function($template_file, $variables, $suggestions);

to:

+    $variables['theme_hook_suggestions'] = $suggestions;
     $output = $render_function($template_file, $variables);

So while it is no longer possible to change the suggestions via $variables, it is still possible to print them in templates and the engine definition remains the same.

star-szr’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
45.47 KB

This patch should address most of #50 but I still want to add some more tests around '#theme' => 'node__article' scenarios.

Regarding the discussion from #51 to #53 about including theme_hook_suggestions in $variables, I didn't want to include those and not have them be alterable. I could be convinced to go either way on that but I thought including them would be more confusing to people who don't know about these new hooks who would then try to change that array and nothing would happen.

As for this note from #50:

+++ b/core/includes/theme.incundefined
@@ -1051,14 +1066,15 @@ function theme($hook, $variables = array()) {
+    foreach (array('preprocess functions', 'process functions') as $processor) {
+      if (isset($base_hook_info[$processor])) {
+        $info[$processor] = $base_hook_info[$processor];
+      }

Is it correct to _replace_ the $processor functions here instead of add to it?

To fix #939462: Specific preprocess functions for theme hook suggestions are not invoked I think we would need to change this behaviour and add new tests, but AFAIK currently the suggestion-specific implementation never has its own preprocess functions so $info[$processor] is always empty. The existing functionality is covered by Drupal\system\Tests\Theme\ThemeTest::testPreprocessForSuggestions() and those tests fail if you delete these lines. This patch adds a clearer comment that explains the change and the code but the functionality is covered by existing tests.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Needs more tests.

star-szr’s picture

Okay, I now understand after talking to @Mark Carver on IRC what @Fabianx was proposing in #53 and I agree. We can just add the theme hook suggestions into $variables right before the render engine function is called. My concern is was that preprocess functions would see this and try to alter it, and this would make twig_debug easy to break.

So NW for that as well :)

Fabianx’s picture

+++ b/core/includes/theme.incundefined
@@ -1036,11 +1035,39 @@ function theme($hook, $variables = array()) {
+  // Invoke theme hook suggestion alter hooks.
+  $suggestions = array();
+  $suggestion_hooks = array(

The $suggestions being an empty array feels a little strange, because there already is a suggestion coming in as either

a) array or
b) direct theme callback

via the first argument to theme().

I am fine with that being follow-up though, because this is really tricky to unwrangle and hook implementations can look at $theme_hook_original to get that information.

Besides the BC needed for theme_hook_suggestions, I am fine with the changes here :).

Thanks!

star-szr’s picture

Should have time to work on this tonight or tomorrow.

star-szr’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
43.58 KB

Here are the changes in line with #53 so that we pass in the template suggestions right before calling the template render function. Much simpler (and the patch is smaller), thanks @Fabianx :)

Still needs work after this to add a few more test cases.

star-szr’s picture

Issue tags: -Needs tests
FileSize
8.87 KB
46.49 KB
14.46 KB

Here are the tests for calling specific theme suggestions like node__article to verify the correct suggestions alter hook is fired.

Also cleaned up some of the existing tests by getting rid of unnecessary assertion messages, the default '"@text" found' will do just fine :)

star-szr’s picture

FileSize
1.28 KB
46.2 KB

Moving these unrelated changes out to #2050883: Incorrect function references in views_preprocess_comment() and views_preprocess_node() to make this patch easier to review and more focused.

+++ b/core/modules/views/views.moduleundefined
@@ -238,18 +238,14 @@ function views_plugin_list() {
-  // The 'view' attribute of the node is added in views_preprocess_node()
+  // The 'view' attribute of the node is added in
+  // \Drupal\views\Plugin\views\row\EntityRow::preRender().
+++ b/core/modules/views/views.moduleundefined
@@ -260,16 +256,39 @@ function views_preprocess_node(&$variables) {
-  // The 'view' attribute of the node is added in template_preprocess_views_view_row_comment()
+  // The view data is added to the comment in
+  // \Drupal\views\Plugin\views\row\EntityRow::preRender().

Edit: I also cancelled a couple of the tests above since the queue is a bit backed up. So this should be the corresponding green to the red test-only patch above :)

Status: Needs review » Needs work
Issue tags: -needs profiling, -Twig, -Theme Component Library

The last submitted patch, 1751194-61.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: +needs profiling, +Twig, +Theme Component Library

#61: 1751194-61.patch queued for re-testing.

Fabianx’s picture

Two things need to happen here:

* The issue summary needs to be updated with the template.

* This is an API change for changing beahvior in preprocess (albeit a necessary one), so we need to go through the approval process.

Approval process

Theme system maintainers need to agree that this is

a) major
b) necessary
c) this patch is they way to solve this.

Then we need to get approval from a core committer and add the "API change approved" tag.

There is a flow-chart for that, see:

https://drupal.org/node/2045345

Thanks!

Fabianx’s picture

+++ b/core/includes/theme.incundefined
@@ -1155,6 +1160,7 @@ function theme($hook, $variables = array()) {
+    $variables['theme_hook_suggestions'] = $suggestions;
     $output = $render_function($template_file, $variables);

This could use a comment that this is needed to keep backwards-compatibility for templates and render engines.

Besides that looks fine to me, is almost RTBC (from my POV) besides the tasks I outlined above.

Fabianx’s picture

Re-adding tags ...

star-szr’s picture

FileSize
45.75 KB

Rerolled to fix conflicts after #1843650: Remove the process layer (hook_process and hook_process_HOOK) was committed, no other changes.

star-szr’s picture

FileSize
814 bytes
45.82 KB

@Fabianx pointed out in IRC that the logic in views_preprocess_node() got changed, this should at least be consistent with what was there previously.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Issue summary updated, going to try and profile this.

star-szr’s picture

First round of profiling, standard homepage with 10 nodes.

=== 8.x..8.x compared (51f5bb078e5a7..51f5bb45dd257):

ct : 78,863|78,863|0|0.0%
wt : 387,046|387,800|754|0.2%
cpu : 349,183|349,300|117|0.0%
mu : 20,908,008|20,908,008|0|0.0%
pmu : 20,951,424|20,951,424|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f5bb078e5a7&...

=== 8.x..suggestions-1751194-68 compared (51f5bb078e5a7..51f5bbaa8fc1d):

ct : 78,863|80,196|1,333|1.7%
wt : 387,046|391,635|4,589|1.2%
cpu : 349,183|353,518|4,335|1.2%
mu : 20,908,008|20,963,576|55,568|0.3%
pmu : 20,951,424|21,008,488|57,064|0.3%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f5bb078e5a7&...

Fabianx’s picture

More profiling results (50 nodes, up to 4 comments per node):

=== baseline-8.x..8.x compared (51f628573214c..51f62a3af1f52):

ct  : 370,447|370,447|0|0.0%
wt  : 1,385,621|1,385,240|-381|-0.0%
cpu : 1,376,086|1,380,086|4,000|0.3%
mu  : 36,913,608|36,913,608|0|0.0%
pmu : 37,034,496|37,034,496|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f628573214c&...

=== baseline-8.x..core-1751194 compared (51f628573214c..51f62b0e7bc11):

ct  : 370,447|375,651|5,204|1.4%
wt  : 1,385,621|1,403,322|17,701|1.3%
cpu : 1,376,086|1,396,088|20,002|1.5%
mu  : 36,913,608|37,367,520|453,912|1.2%
pmu : 37,034,496|37,480,096|445,600|1.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f628573214c&...

Fabianx’s picture

And the same data from XHProfCli:

+--------------------------+------------+------------+------------+------------+------------+
| namespace                |        min |        max |       mean |     median |       95th |
+--------------------------+------------+------------+------------+------------+------------+
| Calls                    |            |            |            |            |            |
|                          |            |            |            |            |            |
| drupal-perf-core-1751194 |    375,651 |  1,234,775 |    384,306 |    375,651 |    375,651 |
| drupal-perf-8x           |    370,447 |  1,227,008 |    379,077 |    370,447 |    370,447 |
|                          |            |            |            |            |            |
| Wall time                |            |            |            |            |            |
|                          |            |            |            |            |            |
| drupal-perf-core-1751194 |  1,403,244 |  7,831,856 |  1,484,083 |  1,413,843 |  1,448,570 |
| drupal-perf-8x           |  1,385,551 |  7,539,964 |  1,468,799 |  1,398,843 |  1,442,383 |
|                          |            |            |            |            |            |
| CPU time                 |            |            |            |            |            |
|                          |            |            |            |            |            |
| drupal-perf-core-1751194 |  1,396,087 |  4,884,305 |  1,445,010 |  1,408,088 |  1,436,089 |
| drupal-perf-8x           |  1,376,086 |  4,840,302 |  1,429,129 |  1,392,087 |  1,424,088 |
|                          |            |            |            |            |            |
| Memory usage             |            |            |            |            |            |
|                          |            |            |            |            |            |
| drupal-perf-core-1751194 | 37,358,864 | 54,476,496 | 37,566,409 | 37,358,864 | 37,358,864 |
| drupal-perf-8x           | 36,913,608 | 54,366,168 | 37,128,117 | 36,913,608 | 36,913,608 |
|                          |            |            |            |            |            |
| Peak memory usage        |            |            |            |            |            |
|                          |            |            |            |            |            |
| drupal-perf-core-1751194 | 37,471,944 | 55,214,472 | 37,687,367 | 37,471,944 | 37,471,944 |
| drupal-perf-8x           | 37,034,496 | 55,130,816 | 37,256,876 | 37,034,496 | 37,034,496 |
|                          |            |            |            |            |            |
+--------------------------+------------+------------+------------+------------+------------+

As we can see the data matches well.

Fabianx’s picture

Again it is the EntityBCDecorator that makes a seemingly fast operation slower:

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=views_theme_...

As expected the majority of the time is spent in ModuleHandler::alter:

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f628573214c&...

Fabianx’s picture

Issue summary: View changes

Update issue summary to use template and explain changes

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -needs profiling +theme system cleanup

I am setting RTBC status, because we deem it correct and "funtionality finished" by now.

We have tests, profiling and a nice issue summary.

The API change is needed to be able to properly support calling preprocess or prepare_alter functions that include suggestions and is part of the general theme system cleanup.

The only BC breaking change is that suggestions can no longer be set within preprocess, but can be set within the new hook_theme_suggestions_alter or hook_theme_suggestions_BASE_THEME_ID_alter(). (BASE_THEME_ID == theme HOOK used, like "node" or "comment" or "container").

Fabianx’s picture

Issue summary: View changes

Profiling completed

webchick’s picture

Mark Carver brought this to me tonight for approval for the API change here. Here's a summary of the conversation.

This issue, in combination with #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK(), will fix #939462: Specific preprocess functions for theme hook suggestions are not invoked, which is currently classified as a critical issue. So that's a point in its favour. It also makes 2035055 much easier to solve, so there's another one. And, it's a much nicer and intuitive API to work with, so there's a third.

However, there are tons of .module hunks in here where code is being removed and re-added as hooks. This means that we're breaking module developer APIs, which is exactly what http://buytaert.net/drupal-8-api-freeze tells us we shouldn't be doing at this point. I also can't quite suss out what about 939462 makes it a literally release-blocking issue; most likely it's just a major one, which subtracts a point from above. And finally, the people working on the theme system knew the same deadlines as all the people working on every other system, so it's unclear to me why theme system changes would be granted special exceptions to the code freeze date. At best, this confuses people, at worst it can create feelings of resentment among contributors.

That said, the risk to committing this and actually breaking someone's ported D8 module out there are incredibly low, especially if it happens soon, the benefits are obvious and clear, and even if 939462 is only major, closing a major issue is still a win.

So I think I find myself on the side of the fence that would add the "approved API change" tag to this issue, but I'd like to sleep on it first, and/or a second opinion from one of the other core maintainers.

star-szr’s picture

FileSize
45.78 KB

Needed a quick reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1751194-76.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
1.08 KB
45.8 KB

Fixed up the one line in the test after #2033383: Provide a default plugin bag landed and also got rid of a deprecated config() in favour of \Drupal::config().

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

Patch was rerolled, test before last was fixed and this test is now green. Back to RTBC. Also FWIW: we haven't released anything since July 1st, so that's another plus to get this in asap.

mikl’s picture

Yeah, would be great if we can get this in before it's one year anniversary – good job on keeping the patch alive for so long, with Drupal changing so much in the meantime…

webchick’s picture

Assigned: star-szr » catch

Since catch voiced concerns about theme-related changes that affect module developers, I'd like him to give final sign-off on this. Assigning.

star-szr’s picture

FileSize
45.76 KB

Straight reroll, some context in the patch was removed in #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off so the patch no longer applied.

Status: Reviewed & tested by the community » Needs work
Issue tags: -API change, -Twig, -theme system cleanup, -Theme Component Library

The last submitted patch, 1751194-82.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Twig, +theme system cleanup, +Theme Component Library

#82: 1751194-82.patch queued for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Passing so back to RTBC

catch’s picture

Assigned: catch » Unassigned

Fine with the API change - this is necessary to fix #939462: Specific preprocess functions for theme hook suggestions are not invoked so should really have been critical.

I had a look through and nothing jumped out, but not enough to commit tonight so unassigning in case someone beats me to it.

effulgentsia’s picture

Priority: Major » Critical
Issue tags: +Approved API change

Raising priority and tagging per #86.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Thanks, catch!

+++ b/core/modules/block/block.module
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function block_theme_suggestions_block_alter(array &$suggestions, array $variables) {

+++ b/core/modules/field/field.module
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function field_theme_suggestions_field_alter(array &$suggestions, array $variables) {

+++ b/core/modules/forum/forum.module
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function forum_theme_suggestions_forums_alter(array &$suggestions, array $variables) {

+++ b/core/modules/search/search.pages.inc
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function search_theme_suggestions_search_results_alter(array &$suggestions, array $variables) {

+++ b/core/modules/taxonomy/taxonomy.module
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function taxonomy_theme_suggestions_taxonomy_term_alter(array &$suggestions, array $variables) {

+++ b/core/modules/views_ui/views_ui.theme.inc
@@ -473,5 +473,11 @@ function template_preprocess_views_ui_view_preview_section(&$variables) {
+/**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function views_ui_theme_suggestions_views_ui_view_preview_section_alter(array &$suggestions, array $variables) {

It is a bit bizarre for a module to implement an alter hook on behalf of itself. In hook_perm(), for example, you just return a list of permissions you want to add to the list; you don't take the list of permissions by reference and alter it.

It seems to me we might want to either rename hook_theme_suggestions_alter() to just hook_theme_suggestions(), or possibly add it in addition. Not sure what the performance impact of that would be, though.

+++ b/core/modules/system/system.module
@@ -968,6 +968,48 @@ function system_menu() {
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function system_theme_suggestions_html_alter(array &$suggestions, array $variables) {
+  $suggestions = array_merge($suggestions, theme_get_suggestions(arg(), 'html'));
+}
+
+/**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function system_theme_suggestions_page_alter(array &$suggestions, array $variables) {
+  $suggestions = array_merge($suggestions, theme_get_suggestions(arg(), 'page'));

+++ b/core/modules/views/views.module
@@ -261,6 +259,19 @@ function views_preprocess_node(&$variables) {
 /**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function views_theme_suggestions_node_alter(array &$suggestions, array $variables) {
+  $node = $variables['elements']['#node'];
+  if (!empty($node->view) && $node->view->storage->id()) {
+    $suggestions[] = 'node__view__' . $node->view->storage->id();
+    if (!empty($node->view->current_display)) {
+      $suggestions[] = 'node__view__' . $node->view->storage->id() . '__' . $node->view->current_display;
+    }
+  }

@@ -269,9 +280,18 @@ function views_preprocess_comment(&$variables) {
+/**
+ * Implements hook_theme_suggestions_HOOK_alter().
+ */
+function views_theme_suggestions_comment_alter(array &$suggestions, array $variables) {
+  $comment = $variables['elements']['#comment'];
+  if (!empty($comment->view) && $comment->view->storage->id()) {
+    $suggestions[] = 'comment__view__' . $comment->view->storage->id();
+    if (!empty($comment->view->current_display)) {
+      $suggestions[] = 'comment__view__' . $comment->view->storage->id() . '__' . $comment->view->current_display;
     }

Like, these actually make sense as alter hooks, because system.module is altering things defined by the theme system and views.module is altering things defined by node.module/comment.module.

+++ b/core/modules/system/theme.api.php
@@ -158,6 +158,44 @@ function hook_preprocess_HOOK(&$variables) {
 /**
+ * Alters theme suggestions.
+ *
+ * Provide alternative theme function or template suggestions for theme hooks.
...
+function hook_theme_suggestions_alter(array &$suggestions, array $variables, $hook) {

There's nothing in theme.api.php that explains what "theme suggestions" are, so we should talk about it here.

This could do with a plain English description, and a @code example to explain the concept.

star-szr’s picture

Assigned: Unassigned » star-szr

The more I think about this, hook_theme_suggestions() AND hook_theme_suggestions_alter() seems like the way to go. That needs to be profiled to see what the cost is. I don't think it's adding another phase, just refining the suggestions phase.

The biggest advantage I see is that the module implementing the theme hook would always have its suggestions come first (be less specific) which makes sense to me. If everyone is implementing alter hooks then it's just based on alphabetical order and hook_module_implements_alter().

webchick’s picture

Yep, and that's a pretty standard pattern used across lots of modules: "build the thing," "alter the thing," "alter specific things of the thing" (uh. ;))

tstoeckler’s picture

Let's name it hook_theme_suggestions_info() and hook_theme_suggestions_info_alter(), then please. That's the standard used throughout core.

webchick’s picture

Title: Introduce hook_theme_suggestions_alter() and hook_theme_suggestions_HOOK_alter() » Introduce hook_theme_suggestions_info() and hook_theme_suggestions_info[_HOOK]_alter()

Agreed.

star-szr’s picture

And we still want hook_theme_suggestions_info_alter() (the generic alter for all hooks), right?

mikl’s picture

Title: Introduce hook_theme_suggestions_info() and hook_theme_suggestions_info[_HOOK]_alter() » Introduce hook_theme_suggestions() and hook_theme_suggestions[_HOOK]_alter()

#91, #92, that seems pretty backwards.

The reason for the _info suffix on, say, hook_block_info or hook_entity_info, is to differentiate it from hook_block_view and the other hook_block_*

We don't need any differentiator here, since there is no hook_theme_suggestions_view (nor would it make sense to have one).

Also, all the other _info hooks provide metadata for something. This case doesn't. This hook does not provide info(rmation) about suggestions. It provides suggestions.

So I'd urge you to reconsider. _info makes no sense here, and only serves to make this hook name even longer.

a.ross’s picture

While anyone in their right mind would generally support keeping function names short, I think applying naming conventions is more appropriate here. That convention, I assume, doesn't hinge merely on differentiating between various similarly named functions.

mikl’s picture

doesn't hinge merely on differentiating various functions from similarly named functions.

No, the _info suffix also serves to denote those hooks who provide metadata:

  • hook_block_info provides metadata for blocks.
  • hook_entity_info is used to define new entity types.
  • hook_cron_queue_info, in the words of the documentation, “Declare queues holding items that need to be run periodically.”

I don't think `hook_theme_suggestions` (and its alter variation) fits the pattern here. It does not provide Drupal with new information. It does not declare new templates. It does not provide metadata. It only serves to indicate which templates Drupal should use for what purpose.

markhalliwell’s picture

No, the _info suffix also serves to denote those hooks who provide metadata:

@mikl is correct. _info means we're returning an associative array of information, which in this case we are not. We are only providing a simple array of template suggestions.

Fabianx’s picture

Agree on #96:

We have theme suggestions, we return them and we alter them. No need for info, which returns structured data.

In general the new thing would be:

* hook_theme_suggestions() - like hook_theme_prepare() should be defined by the owning module or a module extending a base type.
* hook_theme_suggestions_alter() - can be defined by "foreign" modules and themes.

So hook_theme_suggestions() would return:

return array(
'node__1',
'node__foo'
);

and moduleHandler->invoke is used.

While the alter hook would alter that array.

Works for me.

jenlampton’s picture

Title: Introduce hook_theme_suggestions[_HOOK]_alter() » Introduce hook_theme_suggestions() and hook_theme_suggestions[_HOOK]_alter()
Assigned: Unassigned » star-szr
Status: Reviewed & tested by the community » Needs work

I think if we're going to add *another* new phase to the theme system - hook_template_suggestions() - then we need to remove something. Right now we have 4 ways to do the same thing.

If you want to add a theme_hook_suggestion in Drupal 8, you can do the following

  1. provide a pattern in hook_theme
  2. implement hook_template_suggestions()
  3. implement hook_template_suggestions_alter()
  4. pass an array to theme()

In Drupal 7, the equivilents would have been:

  1. provide a pattern in hook_theme
  2. implement template_preprocess_foo()
  3. implement hook_template_preprocess_foo()
  4. pass an array to theme()
jenlampton’s picture

Title: Introduce hook_theme_suggestions() and hook_theme_suggestions[_HOOK]_alter() » Introduce hook_template_suggestions() and hook_template_suggestions[_HOOK]_alter()
Issue tags: -Twig, -Theme Component Library, -Approved API change

After much discussion in IRC, I decided I could be convinced to do this if I was allowed to kill something.

So: #2063793: Remove the ability to provide a 'pattern' in hook_theme()

jenlampton’s picture

grr dreditor. retagging.

tstoeckler’s picture

Title: Introduce hook_template_suggestions() and hook_template_suggestions[_HOOK]_alter() » Introduce hook_template_suggestions_info() and hook_template_suggestions[_HOOK_]info_alter()
Issue tags: -Twig, -Theme Component Library, -Approved API change

No, #96/#97 are not correct. The _info suffix is (AFAIK) consistently applied throughout core currently, wherever we have a "build information" step and then a separate "alter information" step.

_info means we're returning an associative array of information, which in this case we are not. We are only providing a simple array of template suggestions.

This is not correct. Whether template suggestions are or are not "information" is in the eye of the beholder. I would certainly answer that question with yes. Whether info hooks return associative arrays or not is an implementation detail.

Furthermore, I would actually suggest to use associative arrays to allow for easier altering. Otherwise one will have to use array_search().

Please let's just use the existing standard here, please. If you disagree please open a separate issue. I am totally open to a discussion about improving our current standards, but while we have them let's use them.

Fabianx’s picture

Re-adding tags

Fabianx’s picture

Title: Introduce hook_template_suggestions_info() and hook_template_suggestions[_HOOK_]info_alter() » Introduce hook_theme_suggestions_info() and hook_theme_suggestions_info[_THEMEID]_alter()

I am not convinced on the _info, but I don't care enough about naming either.

I just care about:

* The alter is after the THEMEID
* We stick with theme_suggestions instead of template_suggestions for D8 as we still have theme functions and suggestions work for those as well (even if discouraged).

Furthermore, I would actually suggest to use associative arrays to allow for easier altering. Otherwise one will have to use array_search().

This is not true as the last implemented hook/THEMEID in the array wins.

So you can just _add_ to the suggestions in the alter without having to remove anything.

But having an associative array on the other hand (on what) would not help either, so array_search it would need to be.

mikl’s picture

Title: Introduce hook_theme_suggestions_info() and hook_theme_suggestions_info[_THEMEID]_alter() » Introduce hook_template_suggestions() and hook_template_suggestions[_HOOK]_alter()

#102: You can't just say we are incorrect without providing actual examples. Please point out where an info hook is not used to provide metadata in nested arrays.

There's no standard, or even a convention defining that that all the stuff that can be _alter()'ed must be named info, and there's plenty of cases where it's not the case: hook_js_alter(), hook_css_alter(), hook_admin_paths_alter(), hook_batch_alter(), hook_file_url_alter(), hook_form_alter(), need I go on?

Please, in the future, try to provide a cohesive argument for your position (you know, with facts and examples), rather than simply stating that the well-reasoned position of others is “incorrect”. If not, you're just inflaming the issue and wasting everyone's time.

tstoeckler’s picture

So you can just _add_ to the suggestions in the alter without having to remove anything.

Certainly you *can* but the whole point of the _info/_info_alter() separation is that you add in _info() and *never* in _alter()!

Please point out where an info hook is not used to provide metadata in nested arrays.

As I tried to explain the type of data structure returned by an info hook is irrelevant. You might be right that they all return associative arrays, I'm not sure. But that's not the point. The point is the separation of "build" and "alter".

That said, I don't feel as strongly about this as others, so I won't fight it in any way. I still think it would be inconsistent to lose the _info suffix, but I won't lose sleep over it.

mikl’s picture

#106:

The point is the separation of "build" and "alter".

Did you actually read my post where I named a whole handful of examples of _alter() hooks? Most of these do have a build step as well.

I don't feel as strongly about this as others, so I won't fight it in any way.

So you just posted to point out that you're right, and we're deluded by our emotions? That's nice. You might want to read “How to disagree” at some point.

tstoeckler’s picture

@mikl: Sorry, if I come off as condescending, that was not my intention. I don't think you're deluded by your emotions. I realize that you have made a technical case for your opinion. I tried to make mine. And we disagree. That happens. And it seems a resolution of this conflict is not immediate. So instead of continuing the discussion I'll just defer to the people that have actually put effort into this issue in favor of actually making progress.

- did you actually read my post where I named a whole handful of examples of _alter() hooks? Most of these do have a build step as well.

Yes, I did read your post multiple times. Apparently I did not do a very good job of explaining my position. I did not rebutt your reasoning in detail because, as I tried to explain, I personally think it's coming from the wrong angle. But again, I defer.

In case you still feel wronged by me, maybe we can resolve this in IRC?

webchick’s picture

Guys, please remember we're all on the same side here and want to make Drupal better. :) Here, have a ponycorn:

Unicorn

FWIW though, I agree with mikl that this seems to fall more in-line with the examples he cited in #105 than hook_entity_info(), etc. Now let's please put harsh feelings aside and get 'er done!

mikl’s picture

In case you still feel wronged by me

For the record, I don't feel wronged. We're just arguing about computer code here :)

Now let's please put harsh feelings aside and get 'er done!

Duly noted.

tstoeckler’s picture

OMG, that is the Web 3.0 version of the <blink> tag... :-)

mikl’s picture

mikl’s picture

Following up on #88, should we simply drop the _alter suffix, since in many cases we're not altering, but simply adding?

Fabianx’s picture

There is another case for dropping _info:

The function signature of _info is:

hook_element_info() {
}

Our function signature is / would be:

hook_theme_suggestions($variables, $hook) {
}

As the suggestions almost always depend on the $variables contextual information. (node__article is only possible if you know $variables['#node'])

That was also the case for having only the _alter case, because altering is usually done within some kind of context _and_ we already come in with suggestions to theme - even if one hook had been pre-selected already at this point.

There is two ways to supply theme suggestions to theme() directly (which can be combined):

a__b__c__d__...__z

or

array('a__c', 'a__b', 'a')

which is very confusing in itself as only the first suggestion is stripped from start to end, but the rest must exist as is in the theme registry.

So technically if I call:

#theme => array('node__mysuggestion'),

Then before the hook_suggestions_alter there is already a matching hook selected, which has the highest priority and which we currently don't allow to alter ... (are we missing test coverage here? Do we need to allow to alter the default suggestion if it is found? Has this been possible before?)

But if I call:

#theme => 'node',

Then the suggestions is empty at the beginning and modules "alter" the suggestions with their own.

I wanted to avoid digging into how suggestions that are passed to theme() are altered, etc. and liked the beauty and scope of the original patch but it seems the discussion is necessary to have something that makes sense overall especially in the situation of _alter vs. non-alter.

Thanks,

Fabian

Fabianx’s picture

Issue tags: +Needs tests

We definitely miss test coverage for the different arguments to theme():

* #theme => 'hook__suggestion', => hook--suggestion.html.twig should be used.
* #theme => 'hook__suggestion', module adds hook--suggestion2 => hook--suggestion.html.twig should be used.
* #theme => 'hook__suggestion', module alters it specifically => altered version should be used.

And it gets even more complicated if we pass an array of suggestions to #theme ...

thedavidmeister’s picture

Not sure if anyone noticed this yet, but other hooks sometimes have a _info() prefix, maybe we should think about doing that here ;)

catch’s picture

Yes no _info() suffix please, if there needs to be a suffix it could be _build() possibly. _info() hooks are mostly for statically defining stuff which is no what's happening here - if I saw _info() I'd expect it to be more like hook_theme().

I'm not really sure what the value of two stages is here anyway - given adding a suggestion will win.

thedavidmeister’s picture

The reality is that each "phase" we add will incur some overhead so if we can remove one without causing problems, maybe we should do that.

webchick’s picture

Can we do this with just one phase (hook_template_suggestions())? Is there a use case for "I want to remove module X's theme suggestions" as opposed to "I as 'custom' module want to add some template suggestions that just so happen to be named 'node__' something"?

mikl’s picture

I would be all for doing away with the _alter() step, but I think we should still do hook_template_suggestions[_HOOK]. Perhaps, we should do that exclusively. Is there a use case for using hook_template_suggestions to add suggestions to all theming hooks? I would expect in almost all cases, you want to add suggestions for a specific hook.

a.ross’s picture

If there is no other use case, it'd still be useful for debugging I'd say.

star-szr’s picture

The issue I see with doing away with the _alter() step is this (IMO quite common) scenario:

  • Module called mymodule.module wants to add a template suggestion for nodes.
  • mymodule_template_suggestions_node() adds this template suggestion.
  • node_template_suggestions_node() actually runs AFTER this because of module weight.

Yes, you can change the module or hook weight but this seems like a pretty common use case that would be painful to accomplish.

Previously, template_preprocess_node() was serving as the "build" step (always runs first), and mymodule_preprocess_node() allowed manipulating/adding to the suggestions array.

So roughly the mapping of old API to new (not worrying too much about the naming of the new hooks):
template_preprocess_node() -> node_theme_suggestions_node()
mymodule_preprocess_node() -> mymodule_theme_suggestions_node_alter()

Fabianx’s picture

#122: And that is very consistent with the rest of the theme system re-architecture like theme prepare phase.

+1 on

template_* -> hook_theme_suggestions[_HOOK]()
mythingy_* -> hook_theme_suggestions[_HOOK]_alter()

webchick’s picture

Title: Introduce hook_template_suggestions() and hook_template_suggestions[_HOOK]_alter() » Introduce hook_template_suggestions[_HOOK]() and hook_template_suggestions[_HOOK]_alter()

Make it so!

Fabianx’s picture

Title: Introduce hook_template_suggestions[_HOOK]() and hook_template_suggestions[_HOOK]_alter() » Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()

Yes, that works perfectly, just changing title from "template" to "theme" as in D8 we still support theme functions.

Unless someone strongly thinks otherwise ...

There was a x-post before on this ...

webchick’s picture

Yep, sorry, I was distracted when I made that title change. Let's make it happen. :)

jenlampton’s picture

<3

star-szr’s picture

I'm back onto this over the weekend :)

star-szr’s picture

Status: Needs work » Needs review
FileSize
46.25 KB

First a reroll since the patch no longer applied.

star-szr’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Issue tags: +needs profiling
FileSize
25.93 KB
49.9 KB

Many changes here. Main ones:

  • Addition of hook_theme_suggestions_HOOK() and updating implementations to use this hook instead of the alter.
  • Test coverage for hook_theme_suggestions_HOOK().
  • Add 'original module' information in _theme_process_registry() so that within theme() we know what module to invoke for hook_theme_suggestions_HOOK().
  • Entries in theme.api.php expanded (still more work to be done).
  • Removed unnecessary entries in theme_test_menu().

Regarding #115:

* #theme => 'template__suggestion', => template--suggestion.html.twig should be used.
* #theme => 'template__suggestion', module adds template--suggestion2 => template--suggestion.html.twig should be used.
* #theme => 'template__suggestion', module alters it specifically => altered version should be used.

[Changed hook__suggestion to template__suggestion to make the following easier:]

The HOOK in hook_theme_suggestions_HOOK is always 'template', never 'template__suggestion'. 'template__suggestion' would always be available in $variables['theme_hook_original'] (as mentioned in #27, #57). Is that the "specific alter" case (third case)? Maybe I'm misunderstanding but I don't think the second case rings true, if you add a new suggestion to the end of the array (always present, not based on anything in $variables) and the template exists in the theme, it should be used. This is equivalent to the old $variables['theme_hook_suggestion'] or simply adding to the end of $variables['theme_hook_suggestions'].

I can add some more tests for when theme() is given an array of suggestions, I need to think through the use cases a bit more.

Updated the issue summary with a bit more information about the before/after API change and remaining tasks.

Fabianx’s picture

+++ b/core/includes/theme.inc
@@ -1003,11 +1009,41 @@ function theme($hook, $variables = array()) {
+  if (isset($info['original module'])) {
+    $suggestions = (array) Drupal::moduleHandler()->invoke($info['original module'], 'theme_suggestions_' . $base_theme_hook, array($variables));
+  }

Uhm, nope.

We should remove dependency on theme registry, not add new to it.

This should be just:

$suggestions = Drupal::moduleHandler()->invokeAll()

and merge in the arrays.

with a follow-up to change it to:

Drupal::themeHandler()->invokeAll()

The reason being:

If I have a map, and the map module defines a suggestion, then if I now create a more specific map (bundle).

I want to add my suggestions just for this bundle I created, I just implement the suggestions hook as these suggestions are independent of any other suggestions.

So it is okay if several modules / themes implement theme_suggestions_HOOK.

Fabianx’s picture

Status: Needs review » Needs work

#130:

One more thing:

The currently selected suggestion needs to be added to the the end before the altering.

Use-Case (and I have node--1--mynode.html.twig):

#theme => 'node__1__mynode'

* Suggestion is traversed
* node__1__mynode found in theme registry

node__1__mynode => used suggestion

Now suggestions are added:

node__article, node__1

In the old system the preprocess would get the node__1__mynode as theme_hook_suggestion.

In the new system it instead needs to be added at the end before the altering.

Obviously that is not true for a base hook.

And such cases need test coverage. I expect with current drupal they succeed, with the patch they fail as I cannot see how we preserve an original suggestion. (Might be missing something)

Fabianx’s picture

Issue summary: View changes

Updat remaining, add more information about API change

star-szr’s picture

Status: Needs work » Needs review
FileSize
5.92 KB
49.47 KB

Thanks @Fabianx, that all makes sense. Implemented the feedback from #131 and #132 and added test coverage to make sure node__1__mynode is added to the $suggestions array before the alter.

Edit: also some docs tweaks.

star-szr’s picture

Issue summary: View changes

Update when the issue summary was updated

star-szr’s picture

Issue summary: View changes

Update remaining

Status: Needs review » Needs work

The last submitted patch, 1751194-133.patch, failed testing.

star-szr’s picture

So the test failures are because hook_theme_suggestions_HOOK is now being invoked during updates which is not allowed: https://drupal.org/node/2026515

Fabianx’s picture

#135: Does that mean we need to create an upgrade safe theme() function now? Or would we check the instance of the ModuleHandler?

I think we need an upgrade safe theme function or disallow usage of theme() during updates ...

star-szr’s picture

Assigned: star-szr » Unassigned

I'm not sure the best way forward, it seems we may need to disallow theme() during updates. I'm only seeing these theme hooks being called in the test failures:

task_list
maintenance_page

update_task_list() calls drupal_render() on '#theme' => 'task_list' which seems like it could be refactored to late render after updates have completed. The maintenance_page calls may be much more difficult to refactor.

@Fabianx - Would the upgrade safe theme function be something similar to the old st() in Drupal 7?

star-szr’s picture

Status: Needs work » Needs review
FileSize
49.79 KB

Patch no longer applied, here is a reroll. No changes.

Status: Needs review » Needs work

The last submitted patch, 1751194-139.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
3.63 KB
50.67 KB

Fixes for:

Status: Needs review » Needs work

The last submitted patch, 1751194-141.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
50.49 KB

Fix for module_enable() being gone and a better check for theme API hooks as discussed in IRC - whitelisting any theme_ hooks from the system module.

markhalliwell’s picture

Issue tags: -Needs tests

Yay! Finally back to green! I would love to RTBC this (again), but it still needs profiling.

Fabianx’s picture

+++ b/core/includes/theme.inc
@@ -1117,6 +1119,9 @@ function theme($hook, $variables = array()) {
+    // Add the theme suggestions to the variables array just before rendering
+    // the template for backwards compatibility with template engines.
+    $variables['theme_hook_suggestions'] = $suggestions;
     $output = $render_function($template_file, $variables);

To be more backwards compatible we should probably also provide the theme_hook_suggestion used.
for preprocess / prepare we will pass that as context instead though.

---

Besides this one nitpick, this is good to go from my side as well.

Now onto performance testing :).

star-szr’s picture

FileSize
1.32 KB
51.04 KB

Here is the BC fix, suggestions on improving the inline comments and/or code welcome of course :)

I stepped through an example before and after the patch and it works exactly the same.

mikl’s picture

Title: [Change Notice] Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() » Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Status: Active » Needs review
Issue tags: +needs profiling, +Twig, +theme system cleanup, +Theme Component Library, +Approved API change

Here's a code example for the change note:

Suppose we wanted to use a different template for nodes that a have the
awesome field checked, here's how it could be done.

Before:

/**
 * template_preprocess for the node template().
 */
function awesome_preprocess_node(&$variables) {
  $node = $variables['elements']['#node'];

  if ($node->field_awesome()) {
    $variables['theme_hook_suggestions'][] = 'node__' . $node->bundle() . '__awesome';
    $variables['theme_hook_suggestions'][] = 'node__awesome';
  }
}

After:

/**
 * Implements hook_theme_suggestions_HOOK().
 */
function awesome_theme_suggestions_node(array $variables) {
  $suggestions = array();
  $node = $variables['elements']['#node'];

  if ($node->field_awesome()) {
    $suggestions[] = 'node__' . $node->bundle() . '__awesome';
    $suggestions[] = 'node__awesome';
  }

  return $suggestions;
}
joelpittet’s picture

Issue tags: -needs profiling

Scenario:

  • Anonymous all permissions
  • 50 articles displayed on homepage view
  • 50 articles nodes with 4 comments per node generated.
=== 8.x..8.x compared (523f26a22745b..523f27198f838):

ct  : 234,611|234,611|0|0.0%
wt  : 1,106,868|1,108,009|1,141|0.1%
cpu : 1,039,925|1,040,008|83|0.0%
mu  : 44,263,728|44,263,728|0|0.0%
pmu : 44,619,080|44,619,080|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f26a22745b&...

=== 8.x..1751194-hook_theme_suggestions compared (523f26a22745b..523f27f8820ba):

ct  : 234,611|242,371|7,760|3.3%
wt  : 1,106,868|1,131,627|24,759|2.2%
cpu : 1,039,925|1,065,925|26,000|2.5%
mu  : 44,263,728|44,344,488|80,760|0.2%
pmu : 44,619,080|44,700,416|81,336|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f26a22745b&...

joelpittet’s picture

Issue summary: View changes

Update when the issue summary was updated.

star-szr’s picture

Issue summary: View changes

Add before/after example code

star-szr’s picture

Issue summary: View changes

(Update when the issue summary was updated)

star-szr’s picture

Issue summary: View changes

Small update to remaining tasks

mikl’s picture

The increase in function calls is a bit larger than I expected. Does this test really render more than thousands of templates, or is there something bad going on?

star-szr’s picture

As far as I can see it's almost all ModuleHandler.

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f26a22745b&...

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=theme%403&ru...

This is the function to check.

60 calls to ModuleHandler more.

We knew we would need the new function calls and that that costs some performance, but there are no obvious bottlenecks, but it makes our rendering pipeline much straighter. (and any OO system would have even more overhead ...)

This has an overhead of 26 ms for this calls.

This should be fine and there is no performance regression.

Ready to be committed :-).

markhalliwell’s picture

woo hoo!!

webchick’s picture

Assigned: Unassigned » catch

Cool! Just assigning to catch to check over, since there's a performance impact here.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Are we saying the performance impact would be 13ms instead of 26ms without the alter hook?

I don't really understand why we have the two hooks (it was split into two quite late in the patch) and 13 ms is worth saving if we don't need both.

mikl’s picture

I'm not a great fan of the _alter variant, but without it, we would remove the ability to mess with/react to suggestions added by other modules/themes, which is possible with the old _preprocess hook.

That could be said to be an improvement. So bottom line, +1 for removing it. I talked to mortendk, he agrees.

c4rl’s picture

Assigned: catch » c4rl
Status: Needs review » Needs work

Catching up with this issue at the Prague sprint.

It seems to me if we are going to simplify this patch to only one hook we should actually *keep* the hook_theme_suggestions[_HOOK]_alter() and remove hook_theme_suggestions[_HOOK]().

Reasons:

1. The main purpose of this patch is to decouple suggestions from the preprocess phase. The _alter() hook alone does just that.
2. In D7, we *only* had altering (via preprocess) and that seemed to be fine (in terms of use-cases, that is). Though I know in #90 we adhere to the "define/alter/alter specific" pattern, we are not really solving an existing problem and are introducing a new hook (which could actually be *more* confusing rather than less confusing).
3. The base template definition is always in hook_theme() and part of the registry, so this seems like a fine definition phase for most use-cases.

Reducing the performance impact by half seems worthwhile. Rationale for two hooks seems more philosophical rather than practical. Unless there are objections, I'll plan to work to refactor this to simply contain the _alter() variant.

jenlampton’s picture

Title: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() » Introduce hook_theme_suggestions[_HOOK]_alter()

I actually prefer only having the _alter hook (as originally proposed), especially if there are performance implications of adding more than one new phase. (updating issue title)

mikl’s picture

Maybe a philosophical issue, but do we need the the _alter suffix when there's just the one hook? While it can be used for altering, it will likely be used for simply adding suggestions most of the time.

c4rl’s picture

I would advise the _alter stay since we are indeed passing suggestions via reference (and this is the classic alter pattern in Drupal) and I think it is good DX to have function/method names describe what they are physically doing as best as possible rather than what they are intended for or when they occur.

Fabianx’s picture

Assigned: c4rl » Unassigned
Status: Needs work » Reviewed & tested by the community

Uhm, the call to the alter is actually just 3 ms, which is probably not worth saving for.

(http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=Drupal%5CCor... and those are 25 legit calls)

Also there is a problem when removing _alter.

----- How do you decide if you want to override the default suggestion node__article or not?

The _alter and non-alter came in quite late, but for test coverage reasons.

Lets keep this as is. Back to RTBC for catch to re-review.

Edit:

The _alter phase supports the use case of what before had been overwriting the single $variables['hook_theme_suggestion'].

c4rl’s picture

@Fabianx #160 Hm, I'm recommending we *keep* hook_theme_suggestions[_HOOK]_alter() and remove hook_theme_suggestions[_HOOK]() instead. Your comment seems to be addressing the opposite. :)

What are your thoughts given my remarks in #156?

Fabianx’s picture

#161: We need to keep _both_.

If you keep only one, you can either not alter or you can not not-alter (i.e. you are always altering).

Case 1 - only alter:

* article__node comes in
* You alter it automatically because you add to the end.
* => your node--whatever.html.twig (the node suggestions) is _always_ overriding the initial suggestion.

Case 2 - no altering, only adding:

* You cannot override an incoming suggestion.

Therefore you need both. Lets please keep this standard drupal pattern.

c4rl’s picture

I thought about this some and realized that two hooks are indeed necessary.

In D7, imagine foo.module and theme_foohook() being defined in foo_theme():

If we only implemented the _alter hook, this is the equivalent of only supporting "foo_preprocess_foohook()" and not "template_preprocess_foohook()."

* template_preprocess_foohook() serves as a definition phase because this always runs before any other preprocessor.
* foo_preprocess_foohook() serves as an alter phase (though an unusual implementation).

The naming is what threw me off since "template_preprocess_x" vs "foo_preprocess_x" have different execution order, i.e. "template_preprocess_x" is not technically a hook since it runs first and is irrespective of module weight. :P

So, if we only had an _alter hook, hooks run in weight order, so definition as a pre-cursor to other implementations isn't possible.

Indeed we need both. Apologies for the confusion here.

jenlampton’s picture

Title: Introduce hook_theme_suggestions() and hook_theme_suggestions[_HOOK]_alter() » Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Assigned: star-szr » Unassigned
Status: Needs work » Reviewed & tested by the community

Retitling :)

markhalliwell’s picture

Assigned: Unassigned » catch

Yes, _both_ are needed. Mind you the goal here is implement (and alter) suggestions _before_ the prepare (preprocess) phase: #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK(). That issue will effectively be deprecating (eventually replacing) the preprocess layer entirely though. We cannot invoke these new hooks without having proper suggestions (and altered ones) first.

Yes :) RTBC.... please let's get this in, I have much work to do now!

Assigning back to catch per #160.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK that's fair enough, just wanted to check.

Committed/pushed to 8x, thanks!

markhalliwell’s picture

Title: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() » [Change Notice] Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Assigned: catch » Unassigned
Status: Fixed » Active
Issue tags: +Needs change record

I'm soooo happy right now!!! :D

mikl’s picture

See #147 for a code example for the change notice.

mikl’s picture

Title: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() » [Change Notice] Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Status: Needs review » Active
Issue tags: -needs profiling +Needs change record

*sigh* retagging.

steveoliver’s picture

Assigned: Unassigned » steveoliver

Creating change notice.

star-szr’s picture

Assigned: steveoliver » Unassigned
Priority: Critical » Major

Thanks @mikl, I also updated the issue summary with code examples based on yours :)

And I think the change notice itself is "only" major: #2083415: [META] Write up all outstanding change notices before release

star-szr’s picture

Assigned: Unassigned » steveoliver

Crosspost. Go @steveoliver go!

steveoliver’s picture

Priority: Major » Critical
Issue tags: -Needs tests

Created change record: New hooks for theme suggestions.

Leaving active because there are still remaining tasks.

c4rl’s picture

Title: [Change Notice] Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() » Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Assigned: steveoliver » Unassigned
Priority: Major » Critical
Issue tags: -Needs change record +Needs tests
+++ b/core/modules/system/theme.api.php
@@ -158,6 +158,67 @@ function hook_preprocess_HOOK(&$variables) {
+ * @todo Add @code sample.
+ *
+ * @param array $suggestions
+ *   An array of theme suggestions.
+ * @param array $variables
+ *   An array of variables passed to the theme hook. Note that this hook is
+ *   invoked before any preprocessing.
+ *
+ * @see hook_theme_suggestions_HOOK()
+ */
+function hook_theme_suggestions_HOOK_alter(array &$suggestions, array $variables) {
+  if (empty($variables['header'])) {
+    $suggestions[] = 'hookname__' . 'no_header';
+  }
+}

RE: This @todo. Already includes a code sample, yes? So can we remove the @todo's and cross-off that remaining task on the issue?

Fabianx’s picture

Priority: Critical » Major

If I see this correctly all @todos are addressed, but leaving for Cottser to decide this.

star-szr’s picture

Priority: Critical » Major
Issue tags: +Needs tests

See @webchick's review in #88 for the @code bit - some other hook docblocks use this, hook_page_alter() is an example. I won't be able to get to the remaining tasks anytime soon so anyone feel free to jump in.

c4rl’s picture

Status: Active » Fixed
Issue tags: -Needs tests

To preserve the intent of this issue, I've created #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() as a follow-up and marking this one as fixed.

c4rl’s picture

Issue summary: View changes

Minor tweak to proposed resolution

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Moving remaining tasks to separate issue to mark this one as fixed.

c4rl’s picture

Issue summary: View changes
Issue tags: +DX (Developer Experience), +Needs followup

I think this has possibly introduced the need for a follow-up, based on a DX problem.

Formerly, available suggestions could be grokked by inspecting the respective $variables parameter via a preprocessor function. Understanding what suggestions are available is a necessary task for themers to decide what templates and hooks they wish to use.

As the suggestions have moved, the original method of inspection no longer exists. Presently, you have to dump $suggestions conditionally based on $base_theme_hook right after hook_theme_suggestions_HOOK_alter() runs in theme.inc. I believe 'hacking-core-as-a-method-of-inspection' is bad DX.

Therefore, (as a stopgap until the theme system is redeveloped to be OO, i.e. D9+) it seems plausible that we should add something like $variables['theme_suggestions'] that would be read-only, i.e. have no adverse effects on theme() or child function calls. In this way, the method of inspection is preserved.

Thoughts?

c4rl’s picture

Another potential problem. Preprocessors are available to both modules and themes. However, ModuleHandler::invokeAll() *only* calls out to modules and *not* themes. Since this has moved from a preprocessor to a hook called via invokeAll() that means a theme cannot provide suggestions, which breaks functionality we once had.

In that regard, since this issue was once Critical, should we address this problem in #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter(), also bumping it to Critical?

Related #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)

c4rl’s picture

After some discussion in IRC, Cottser has provided some insights.

Regarding #179, a theme can implement hook_theme_suggestions_alter() to discover what templates are available.

Regarding #180, Because themes generally invoke hooks after all modules have been called, the use-case for implementing hook_template_suggestions() doesn't particularly apply in most cases since it is physically impossible for anything to alter what a theme has provided. Nevertheless, since themes can indeed call hook_theme(), and hook_theme_suggestions() is bound fairly explicitly to hook_theme(), I would prefer to have consistency across modules and themes here: Someone new to Drupal leans about hook_theme() and hook_theme_suggestions() in the realm of modules. So yay. Then they are writing a theme, and invoke hook_theme() in this theme, but can't do hook_theme_suggestions(), they have to do hook_theme_suggestions_alter(). So this seems like something else they have to understand, rather than just providing a consistent API.

Minimally, we could provide a note in the documentation about hook_theme_suggestions() as a stopgap until #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler) is done

star-szr’s picture

akalata’s picture

Issue summary: View changes