Specific preprocess functions for theme hook suggestions are not invoked

Try this:

/**
 * Implementation of hook_theme().
 */
function mytheme_theme() {
  return array(
    'views_view__page_1' => array (
      'arguments' => array(),
    ),
  );
}

/**
 * 
 */
function mytheme_preprocess_views_view__page_1(&$variables) {
  dpm('This should be working');
}

This seems to be a core theme() behavior: if the 'base hook' property is set for a theme registry item, theme() will use the 'preprocess functions' and 'process functions' from the 'base hook' theme registry item. This means that currently, preprocess functions for the 'views_view__test_preprocess' theme registry item will always get ignored in favor of the preprocess functions on the 'views_view' theme registry item, since $theme_registry['views_view__test_preprocess']['base hook'] is set to 'views_view'.

gleroux02, I believe this is a change in behavior from using Views on Drupal 6? The 'base hook' theme registry property seems to be new in Drupal 7, and its presence is not documented in the hook_theme() docs (http://api.drupal.org/api/function/hook_theme/7). Inline documentation in theme() indicates that the original theme registry item name is retained in $variables['theme_hook_suggestion'], so if the core behavior is correct, then maybe template_preprocess_views_view() could check there and execute any additional preprocess functions itself...

This issue is related to commit 344200 and #241570: Theme preprocess functions do not get retained when using patterns.

Tracking. See my comment here: http://drupal.org/node/241570#comment-3679436

I'm moving this issue to the core queue, in order to close out #241570: Theme preprocess functions do not get retained when using patterns. That other issue initially dealt with a related, but different problem, which was that hook_preprocess_views_view() wasn't called when views-view--page-1.tpl.php was being used. That issue was fixed for D6 in #241570-6: Theme preprocess functions do not get retained when using patterns and for D7 in #241570-50: Theme preprocess functions do not get retained when using patterns. The D6 solution made it so that both hook_preprocess_views_view() and hook_preprocess_views_view__page_1() would be called. The D7 solution intentionally made it so that only hook_preprocess_views_view() would be called. There has been further discussion in that issue requesting that hook_preprocess_views_view__page_1() be added to the D7 theme system, so that D7 doesn't represent a regression to D6 in this regard. I'd like that discussion to continue in this issue, so that new people joining the discussion don't need to read that issue's history, and can just focus on this particular question.

Here are some things I don't like about the way in which D6 implements suggestion-specific preprocess functions:

1. They are auto-discovered for themes, but not for modules. If you have a theme with a views-view--page-1.tpl.php file, then your theme can have a mytheme_preprocess_views_view__page_1() function, but if you have a module with a mymodule_preprocess_views_view__page_1() function, it does not get called. If you want a module to have a suggestion-specific preprocess function, you need to add a 'views-view--page-1.tpl.php' to your module folder, and this to your module code:
   

   function mymodule_theme() {
     return array(
       'views_view__page_1' => array(
         'template' => 'views-view--page-1',
         'original hook' => 'views_view',
       ),
     );
   }
   

   And, you need mymodule_theme() to run AFTER views_theme(), which probably means you need to set the system weight of mymodule to 1 or higher.

2. Related to above, a theme can only have a suggestion-specific preprocess function if it also implements the template. So, mytheme_preprocess_views_view__page_1() only runs if the theme also has a 'views-view--page-1.tpl.php' file.
3. Refining the above point, if you implement a suggestion-specific preprocess function in a theme that has a base theme, but don't implement the corresponding template in the specific theme, then whether that preprocess function runs or not depends on if your base theme has the template. For example, suppose mytheme is a subtheme of mybasetheme, and you have a mytheme_preprocess_views_view__page_1() function, but no 'views-view--page-1.tpl.php' file within mytheme. Then, if you have a 'views-view--page-1.tpl.php' file in mybasetheme, then mytheme_preprocess_views_view__page_1() will run. But if you don't, then it won't.
4. They don't chain in the way you'd expect. For example, suppose your theme has a mytheme_preprocess_views_view__taxonomy_term_leaf() function and a mytheme_preprocess_views_view__taxonomy_term_leaf__page() function, along with
   'views-view--taxonomy-term-leaf.tpl.php' and 'views-view--taxonomy-term-leaf--page.tpl.php' files. When the 'views-view--taxonomy-term-leaf--page.tpl.php' file is used, I would expect mytheme_preprocess_views_view__taxonomy_term_leaf() and mytheme_preprocess_views_view__taxonomy_term_leaf__page() to both be called, but they're not. Only mytheme_preprocess_views_view__taxonomy_term_leaf__page() is.
5. You do not get suggestion-specific preprocess functions for things that use 'template_file_suggestions' as opposed to theme hook patterns. In other words, while you can have a mytheme_preprocess_views_view__page_1(), you cannot have a mytheme_preprocess_node_story().

IMO, the above WTFs make suggestion-specific preprocess functions more trouble than they're worth. To me, the following construct is preferable:

function mytheme_preprocess_views_view(&$variables) {
  if (in_array('views_view__page_1', $variables['theme_hook_suggestions'])) {
    ...
  }
}

Although I realize that the above isn't as syntactically attractive as a targeted function name, in D7, this construct works without the prior WTFs:

1. The above can be done in a module or theme.
2. The above is decoupled from whether 'views-view--page-1.tpl.php' is implemented. In other words, it can be used to target special preprocessing for the "Page 1" view, regardless of whether the Page 1 view requires significantly different markup than other views.
3. The above chains correctly. If the 1st param to in_array() above was 'views_view__taxonomy_term_leaf', it would run even if the actual template used was 'views-view--taxonomy-term-leaf--page.tpl.php'.
4. The above works identically for nodes, fields, and other places where the suggestions are specified within template_preprocess_HOOK() rather than by the caller of theme().

Note, what I say above is not fully true yet, but requires #956520-16: Available theme hook suggestions are only exposed when there are preprocess functions defined to be made true. But that's a totally straightforward patch that needs to be committed to core anyway, and I'm confident will be pretty soon (maybe not before RC, but soon).

So, my recommendation is to mark this issue "won't fix" or kick it to D8, and to require the less attractive, but more robust, pattern to be used for suggestion-specific preprocessing in D7. Alternatively, a contrib module like ctools can implement hook_theme_registry_alter() to make suggestion-specific preprocess functions work in D7 (with all the limitations they have in D6, or maybe even fixing some of them).

But, that's my recommendation only. Please share your thoughts. If my arguments for why suggestion-specific preprocess functions are undesirable in D7 core aren't persuasive, and if the community really wants this D6->D7 regression fixed, I'll support a D7 core patch to fix it.

I'm marking this issue major, rather than critical, because this is something that can be done after RC, and even in a 7.1 release.

Quite a summary :-D

My issue (elsewhere) actually was that this doesn't work:

$output = theme('links__node__comment', $links);

function exampleMODULE_preprocess_links__node__comment(&$variables) {
  $variables['what-you-d-expect'] = 'yeah';
}

function exampleMODULE_preprocess_links__node(&$variables) {
  $variables['what-you-d-perhaps-also-mayhaps-expect'] = 'um, yeah';
}

According to Jacine, this seems to work for themes, and possibly also for templates, but not for preprocess theme functions of modules.

Yep, as per #6 (limitations 1-5), the equivalent of #7 doesn't work in D6 either. I'd love to see it fixed properly in D8. Now that we have a module_implements() cache, I think in D8 we can merge the way we do drupal_alter() with the way we do preprocess functions, simplifying the API and the theme registry building process, and reducing the size of the theme registry, shaving a few ms from each page request. But I'm not sure it makes sense as D7 material.

+1

Subscribing, asking if this can be fixed in Drupal 7 (because it would be lovely), but looking to do it right in D8 in any case. Frankly i think the examplemodule_preprocess_links__node() function should not only be called but override the template_preprocess_links() function, as you could easily call the general function from within your specific preprocess function if you want it.

Subscribing

Until this is "fixed" just do the following:

function mytheme_preprocess_views_view_list(&$variables) {
	$view = $variables['view'];
	$function = implode('__', array(__FUNCTION__, $view->name, $view->current_display));
	
	if (function_exists($function)) {
		$function(&$variables);
	}
}

function mytheme_preprocess_views_view_list__VIEWNAME__DISPLAY(&$variables) {
	print_r($variables);
}

Subscribing and hoping someone can get to benchmarks on #956520: Available theme hook suggestions are only exposed when there are preprocess functions defined soon and then we can revisit this issue.

Subscribing

subscribing also

Before in D6 when you had the tpl file like

views-view-fields--VIEWNAME--block-1.tpl.php

function MYTHEME_preprocess_views_view_fields__VIEWNAME__block_1(&$vars) 

the preprocess function was properly called.

the same thing for D7 is NOT working anymore...

if you do something like:

function MYTHEME_preprocess_views_view_fields (&$vars)

it works fine like the above example,
but it DOES NOT work for specific view + display, like the example bellow:

function MYTHEME_preprocess_views_view_fields__VIEWNAME__block_1 (&$vars)

any thoughts out there?

My temporary solution for D7 at this point:

/**
  * Generic preprocess that is still working on D7
  */
function MYTHEME_preprocess_views_view_fields(&$vars) {
  if (isset($vars['view']->name)) {
    $function = 'MYTHEME_preprocess_views_view_fields__' . $vars['view']->name . '__' . $vars['view']->current_display;
    
    if (function_exists($function)) {
     $function($vars);
    }
  }
}

/**
  * Then the specific preprocess that worked without the above code for D6
  */
function MYTHEME_preprocess_views_view_fields__VIEWNAME__block_1 (&$vars) {
  // my specific preprocess code
}

subscribing

EDIT: I followed the instructions in comments #12 and #16 however it seems that I'm not able to get them to fire either from my base theme or my sub theme? Did you guys have to do anything special in the first place to get theme hooks to be called?

I've opened a separate issue for this problem as I realise it's probably off topic, but I was hoping you might have already trodden this path: #1160268: subtheme_preprocess_page not getting called

EDIT 2: Ok seems I've gotten around this problem without knowing how, thanks to #12 / #16 your advice is now working :)

To carn1x...

This was my temporary solution I still don't know why on D6 the preprocess functions worked without the above code... and that's why I posted it here... I still want a proper explanation for that?

Thanks...

Ah! I was looking for this issue. I posted in the wrong place but this patch will fix it.

http://drupal.org/node/956520#comment-4478448

There's a bit more info starting from that comment.

from effulgentsia overview #6:

Here are some things I don't like about the way in which D6 implements suggestion-specific preprocess functions:

They are auto-discovered for themes, but not for modules. If you have a theme with a views-view--page-1.tpl.php file, then your theme can have a mytheme_preprocess_views_view__page_1() function, but if you have a module with a mymodule_preprocess_views_view__page_1() function, it does not get called...

The patch fixes this.

Related to above, a theme can only have a suggestion-specific preprocess function if it also implements the template. So, mytheme_preprocess_views_view__page_1() only runs if the theme also has a 'views-view--page-1.tpl.php' file.

Also fixed. You don't even need the 'views-view--page-1.tpl.php file to exist at all. If the variable process function implements it, it will auto register the hook and inherit all the base elements.

Refining the above point, if you implement a suggestion-specific preprocess function in a theme that has a base theme, but don't implement the corresponding template in the specific theme, then whether that preprocess function runs or not depends on if your base theme has the template. For example, suppose mytheme is a subtheme of mybasetheme, and you have a mytheme_preprocess_views_view__page_1() function, but no 'views-view--page-1.tpl.php' file within mytheme. Then, if you have a 'views-view--page-1.tpl.php' file in mybasetheme, then mytheme_preprocess_views_view__page_1() will run. But if you don't, then it won't.

This should also be fixed but I haven't tested this.

They don't chain in the way you'd expect. For example, suppose your theme has a mytheme_preprocess_views_view__taxonomy_term_leaf() function and a mytheme_preprocess_views_view__taxonomy_term_leaf__page() function, along with
'views-view--taxonomy-term-leaf.tpl.php' and 'views-view--taxonomy-term-leaf--page.tpl.php' files. When the 'views-view--taxonomy-term-leaf--page.tpl.php' file is used, I would expect mytheme_preprocess_views_view__taxonomy_term_leaf() and mytheme_preprocess_views_view__taxonomy_term_leaf__page() to both be called, but they're not. Only mytheme_preprocess_views_view__taxonomy_term_leaf__page() is.

We have to draw the line somewhere and as I was writing the patch, there was an note in drupal_find_theme_functions() to "keep things simple". I could have written the patch to do what your expecting and I was considering it but it is most likely overkill.

You do not get suggestion-specific preprocess functions for things that use 'template_file_suggestions' as opposed to theme hook patterns. In other words, while you can have a mytheme_preprocess_views_view__page_1(), you cannot have a mytheme_preprocess_node_story().

You mean mytheme_preprocess_node__story? That would depend on how theme() called and it's outside the scope of the theme function.

EDIT: Actually, the same problems exists even with the change from 'template_file_suggestions' to 'theme_hook_suggestions'. Although the patch doesn't address this, I did find a workaround in a patch to a theme I've been working on: #1179656: Consistent availability of variable processor, file includes and auto registration for hook__suggestions. see point #3. I can update the patch to fix it in core but I think an entirely different approach in core should be made for 8. 'theme_hook_suggestions' being set from preprocess works against it.

Lets try that again. That was an old patch.

Why is the test server getting a 404 on the patch?

Eh, hidden space in the file name.

Not sure what the deal is. It was tested two days ago and it passed. Passes locally also.

Fails on comment block but theme_comment_block isn't all that special.

#25: specific_preprocess_hook_suggestions_939462_25.patch queued for re-testing.

Anyone other than dvessel get a chance to test out the latest D7 patch yet?

I'll try again by creating a patch for 8 then back port it to 7. Might get more attention.

I've just applied the patch in #25 on my D7.2 installation and I can confirm that now my

function mytheme_preprocess_views_view__taxonomy_term__page(&$vars) {
   ....
}

is now correctly picked up: YEAHHHH!!!

I was waiting for that for a long time:

Thank you very much!

Same here, fixed the prob. Hope it gets into the code soon :)

applied in D7.4 installation; confirmed that its working.

Here's are some numbers:

// first.module
/**
 * Implements hook_block_view().
 */
function first_block_view($block_name = '') {
  if ($block_name == 'list_modules') {
    $list = module_list();
    
    $theme_args = array('items' => $list, 'type' => 'ol');
    $content = theme('item_list__first', $theme_args);
    
    $block = array(
      'subject' => t('Enabled Modules'),
      'content' => $content,
    );
    
    return $block;
  }
}

// example_chris.module
function example_chris_preprocess_item_list__first (&$variables, $hook) {
  dpm(__FUNCTION__);
  dpm(microtime());
  dpm($hook);
}

function example_chris_preprocess_item_list (&$variables, $hook) {
  dpm(__FUNCTION__);
  dpm(microtime());
  dpm($hook);
}

Here are the results:

example_chris_preprocess_item_list
0.31776300 1309472124
item_list__first

example_chris_preprocess_item_list__first
0.31799800 1309472124
item_list__first

I think 3 people is enough for RTBC; I realize that this is theme.inc we are talking about, let me know if brief glances by 3 people is not enough for an RTBC, and what could we do to make this an RTBC. xhprof profiler benchmarking needed? I'll provide if asked.

Yeah this one really needs so profiling, leaving at CNR rather than CNW since it may still apply to D8.

The main change here is theme rebuilds, so you would need to profile a page that is building the theme registry cache. xhprof should show the difference both in CPU and memory for that if it's done before/after. Main thing to watch out for the APC or other opcode cache if you're running that - should either be consistently warm or consistently cold (or just disabled).

As well as this, I can see a call to get_defined_functions() then array operations on that, can't see that doing anything other than increasing the memory requirements here but will need testing. I've been thinking about trying to move away from including files altogether and instead scan them with regexp at #1188084: Try to avoid loading every specified include file during theme registry rebuilds. although this has no code yet so shouldn't hold the patch up in itself.

Apart from that there's also an issue at #519940: Performance: optimize building theme registry by using get_defined_functions() instead of function_exists() using get_defined_functions() elsewhere in the theme registry rebuild process, not sure if that is applicable at all here (maybe the function_exists() calls that aren't removed?).

And there are no tests added in this patch but this ought to be testable I think.

Subscribing.

Subscribing

#25 breaks drupal for me...

It seems that doing a git apply -v specific_preprocess_hook_suggestions_939462_25.patch on the root of drupal doesn't patch correctly the theme.inc file... (Using Git 1.7.6-preview20110708)

The warning I got after reverting to the original, seems to indicate an include path problem...

Warning: include(C:\www\apache\htdocs\mysite.com/page.tpl.php) [function.include]: failed to open stream: No such file or directory in theme_render_template() (line 1276 of C:\www\apache\htdocs\mysite.com\includes\theme.inc).

Warning: include() [function.include]: Failed opening 'C:\www\apache\htdocs\mysite.com/page.tpl.php' for inclusion (include_path='.;C:\php5\pear') in theme_render_template() (line 1276 of C:\www\apache\htdocs\mysite.com\includes\theme.inc).

I got lots of the same kind of warning for the block.tpl.php, toolbar.tpl.php, html.tpl.php file (same invalid file structure).

I presume the patch has a problem ?

Tagging issues not yet using summary template.

Sub

Tagging for backport.

Subscribing.

With the new Drupal version update (7.9), is needed a new patch to correct this problem.

This will need to be rerolled now.

tagging for learnability.

(rerolled #25 for D7)
Assumption n°3 of comment 21 confirmed is also confirmed:
SUBtheme_preprocess_views_view__VIEWNAME_DISPLAYID(&$vars) is fired
(without the template file)

This patch (almost) made my day, but ... I'm getting a bunch of notices:
Notice : Indirect modification of overloaded element of ThemeRegistry has no effect dans theme() (line 1008 in /var/www/drupal/includes/theme.inc).
(the line is unset($hooks[$hook]['includes']))

#25: specific_preprocess_hook_suggestions_939462_25.patch queued for re-testing.

The patch does not apply. It needs to be created with git. :)

Git version of the patch at #25

We need to create a patch for D8 first, and then profile it as catch says above. (When an issue is fixed, the fix is usually backported to D7 around the same time.)

The patch above might apply to D8 from within the core/ directory so I'd suggest trying that in order to create a D8 patch.

I don't have a D8 to port this patch to,
but can someone elaborate on these ThemeRegistry warnings ?

You can get D8 from git using these instructions:
http://drupal.org/project/drupal/git-instructions

It looks like _theme_post_process_registry() is acting directly on the cached ThemeRegistry object, whereas it should be acting on the raw theme registry array which is created during the rebuild. I didn't look beyond this but hopefully that'll help you track it down.

Here's a D8 Git version of the patch. I've commented out the line
unset($hooks[$hook]['includes']);
as it did nothing except cause errors. The functionality it represents may still be required though. I'm simply creating the patch for D8 to help the effort along. Sadly I believe it will fail testing.

Changing status for testbot.

The patch in #57 appears to be either old or against 7.x; it is missing the core/ directory.

I've been using this technique with great success. However, I would like to point out that the pass-by-reference forcing in...

if (function_exists($function)) {
$function(&$variables);
}

...can cause "Call-time pass-by-reference has been deprecated..." errors in newer versions of PHP. Since your "mytheme_preprocess_views_view_list__VIEWNAME__DISPLAY" function makes a pass-by-reference declaration in the function parameter definition, you're still fine and get the behavior you want with out the PHP warning.

So it should be...

if (function_exists($function)) {
$function($variables);
}

Reroll.
This fixes my problem for D7, didn't try it on D8.

To clarify, my problem was what sun described in #7.

Thanks for the reroll.

Code style review:

1. +++ b/core/includes/theme.incundefined
   @@ -499,29 +499,47 @@ class ThemeRegistry Extends DrupalCacheArray {
   +      // Add all modules so they can intervene with their own variable
   +      // processors. This allows them to provide variable processors even
   +      // if they are not the owner of the current hook.
   ...
   +      // Theme engines get an extra set that come before the normally
   +      // named variable processors.
   ...
   +      // The theme engine registers on behalf of the theme using the
   +      // theme's name.
   

   Looks like these comments are wrapping a bit early.

2. +++ b/core/includes/theme.incundefined
   @@ -499,29 +499,47 @@ class ThemeRegistry Extends DrupalCacheArray {
   -      // overridden.  Pull the rest of the information from what was defined by
   +      // overridden. Pull the rest of the information from what was defined by
   

   This change is superfluous to the patch.

3. +++ b/core/includes/theme.incundefined
   @@ -620,31 +631,90 @@ function _theme_process_registry(&$cache, $name, $type, $theme, $path) {
   + * This completes the theme registry adding missing functions and hooks.
   

   The word "This" is not needed here; it should just begin "Completes..." Reference: http://drupal.org/node/1354#functions

   Also, it would be better to have a comma after "registry."

4. +++ b/core/includes/theme.incundefined
   @@ -620,31 +631,90 @@ function _theme_process_registry(&$cache, $name, $type, $theme, $path) {
   +  // Collect all known hooks. Discovered functions must be based on a known hook.
   

   This line is one character too long; we need to wrap it a word earlier.

5. +++ b/core/includes/theme.incundefined
   @@ -945,14 +1010,13 @@ function drupal_find_base_themes($themes, $key, $used_keys = array()) {
   +    // If called before all modules are loaded, we do not necessarily have a full
   

   Also one character too long.

6. +++ b/core/includes/theme.incundefined
   @@ -1000,6 +1064,7 @@ function theme($hook, $variables = array()) {
   +    //unset($hooks[$hook]['includes']);
   

   We shouldn't have commented-out code.

Also, we still need a test here as well, and that profiling. Thanks!

Oh, tagging novice for the task of cleaning up the code style issues in #64.

working on in the Long Beach Contribute meetup tonight: http://groups.drupal.org/node/211453

(we're in IRC in #drupal-la and #drupal)

Just encountered this issue. Seems to take too long to get this fixed imho. Subscribing.

@bvanmeurs, you can help speed along the process by any of the following:

• Testing the patch @tim.plunkett submitted in #64 and posting here whether it resolves the issue and doesn't cause any other problems.
• Updating the patch with the cleanups described in #64.
• Contributing an automated test that fails without the fix and passes with it.

Thanks!

Edit @mike stewart -- I saw the complaint but not your comment. That's awesome; thank you so much!

arg! new laptop ... spent too much of the meeting last night just getting setup. finally a working environment (late) to discover that the patch would no longer cleanly apply.

error: patch failed: core/includes/theme.inc:1008

need to do a diff in D8 commits on that file to see whats new.

Also #62 @tim.plunkett's comment wasn't clear to me. He said this fixes his problem in D7 but not D8 -- but the patch is clearly a D8 patch, so I wanted to be sure thats where the re-roll belongs? (I assume yes -- but clarification would be nice).

I am busy for the next few hours, but hope to get back to this later today... but didn't want to prevent anyone else from working on, so I've unassigned. If no one else picks up ... I'll be back :-]

@mike stewart, I was saying I'd only tested it on D7, not that it didn't work on D8.

Is this related to http://drupal.org/node/1430300? Can anyone see if this is still required? I have attached the patch from #62 with comments from xjm in #64.

Additionally I can try to make a test for it if someone can provide basic instructions on what is required. If not I'll hand this back over to mike for the testing.

This isn't related to #1430300: Preprocess functions in an include file fail to get called when the theme implements a suggestion override, but you can extend the test from that issue which is now in both 8 and 7.

If in test_theme/template.php, you add a test_theme_preprocess_theme_test__suggestion() function that adds a variable, and change test_theme_theme_test__suggestion() to include that variable in its output, and include that in the assetion in ThemeUnitTest::testPreprocessForSuggestions(), that would test what's being fixed in this issue.

Here is the patch for the test. It fails locally with or without the patch. Let me know if I made it wrong though. I've still new to testing.

The testbot probably kept failing because of the line unset($hooks[$hook]['includes']); which causes
Notice: Indirect modification of overloaded element of ThemeRegistry has no effect in theme() (line 1068 of /d8/core/includes/theme.inc).

Lets try again.

I'm not sure what happened with the last one but hopefully this one will finally apply but fail with the test. @effulgentsia Please let me know if I misunderstood what you were looking for in the test.

The testbot queue is quite backed up ATM:
http://qa.drupal.org/pifr/queued

So TBH I'd give it at least a day.

+++ b/core/includes/theme.incundefined
@@ -1008,46 +1073,25 @@ function theme($hook, $variables = array()) {
-    // Include files required by the base hook, since its variable processors
-    // might reside there.
-    if (!empty($base_hook_info['includes'])) {
-      foreach ($base_hook_info['includes'] as $include_file) {
-        include_once DRUPAL_ROOT . '/' . $include_file;

These were the lines added in #1430300: Preprocess functions in an include file fail to get called when the theme implements a suggestion override, are you sure they should be removed?

I'm not sure what the right answer is here. It probably shouldn't be taken out but since it is a nested if statement I don't know of a good way to leave it in while maintaining what the original patch in #62 did. If you let me know the proper way to handle this I can make a new patch that will apply and accomplish the goal.

#25: specific_preprocess_hook_suggestions_939462_25.patch queued for re-testing.

tim.plunkett,

This one still fails, but I added the code back in which fixed one of the errors.

issues:

• Things have changed. There's a part that conflicts with #1430300: Preprocess functions in an include file fail to get called when the theme implements a suggestion override, I tried to merge as best as ossible. See commit dc685b07.
• I'm also unsure of what the unset() call did. I removed as requested by xjm in #939462-64: Specific preprocess functions for theme hook suggestions are not invoked. To be honest, I tried not to read the code, just merge. I had to read, though, because of the conflict above.
• I'm removing the Novice tag out of honesty.

Yeah, it actually broke installation. I've patched the patch to revert some changes.

I'm going to try to revisit this soon.

Oh yeah I tried and failed hard. No idea.

Here's a test that proves this is broken.

The idea is that if you call a theme function with a suggestion, the preprocess for the suggestion should run even if there is only a default theme function.

Because I got confused a couple times with all the theme_test_test_theme_theme stuff, here is a simplified example of what is wrong here:

function foo_theme() {
  return array(
    'foo' => array(
      'variables' => array('foo' => 'foo'),
    ),
  );
}

function template_preprocess_foo__bar(&$variables) {
  $variables['foo'] = 'bar';
}

function theme_foo($variables) {
  return $variables['foo'];
}

function foo_page() {
  return theme('foo__bar', array());
}

The test in #92 asserts that the result of foo_page() is 'bar' and not 'foo'.

Is this correct?

Yes, that should be the expected behavior / result.

Glad to hear it. Rerolled for PSR-0.

I haven't followed all the latest work here, but I think #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() might be a good approach to solving this. The patch there still needs refinement, but linking to it here for anyone interested.

This will need to get solved for #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core, so tagging accordingly. I'm tempted to mark this a dupe of #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter(), but not doing so yet, cause it's not yet clear whether there's consensus on that being the best approach, and there's work that's been done on this issue that I haven't reviewed yet, but still want to.

My solution for this problem is merging the preprocess functions and deleting the 'base hook'.

This solution worked for me:

/**
 * Implements hook_theme_alter().
 */
function my_module_theme_registry_alter(&$theme_registry){
  $theme_registry['views_view__nodequeue_1']['process functions'] = array_merge($theme_registry['views_view']['process functions'], $theme_registry['views_view__nodequeue_1']['process functions']);

  $theme_registry['views_view__nodequeue_1']['preprocess functions'] = array_merge($theme_registry['views_view']['preprocess functions'], $theme_registry['views_view__nodequeue_1']['preprocess functions']);

  unset($theme_registry['views_view__nodequeue_1']['base hook']);
}

Tests in #96 succesfully fail. I think this test is RTBC, but I'll check in #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() and see is that resolves the test.

#96: drupal-939462-96.patch queued for re-testing.

The tests fail, yes. Because we have no fix yet :)

If the test in #96 fails, that's great, but we can't commit it to 8.x without an accompanying fix..

Tagging with Twig so I can find this again ...

Marked #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() as duplicate of this issue.

oh, wow, scrolling back to #8 was really worth it — @effulgentsia mentioned:

Yep, as per #6 (limitations 1-5), the equivalent of #7 doesn't work in D6 either. I'd love to see it fixed properly in D8.

Now that we have a module_implements() cache, I think in D8 we can merge the way we do drupal_alter() with the way we do preprocess functions, simplifying the API and the theme registry building process, and reducing the size of the theme registry, shaving a few ms from each page request.

This:

drupal_alter(array('template_preprocess_foo__bar__baz', 'template_preprocess_foo__bar', 'template_preprocess_foo'), $variables);

would apparently be in line with my findings and workaround/changes in #782672-16: Loosen the coupling between the user module and the installer/maintenance theme (though the problem space over there is slightly different)

The tricky part would be to get drupal_alter() working for the special hook naming pattern of theme [pre]processors, which isn't the regular hook_fixed_DYNAMIC(), but instead, template_preprocess_DYNAMIC() plus hook_preprocess_DYNAMIC(), and all without _alter suffix (although we could decide to change that).

And yet another potential duplicate (arrrgh), but that one has a patch:

#956520-30: Available theme hook suggestions are only exposed when there are preprocess functions defined

Edit:

Marked as duplicate as the patch was moved over here and the last working version is in #62 in this issue.

#62 works for D7, #96 is the accompanying D7 test (which also works), and both need to be ported to D8. Is that right?

Side note: This is a blocker for #1647978: Entity API needs a 1.0 release as part of the Drupal.org D7 Upgrade Initiative.

There's a patch on #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() that claims to work as well. I thought #96 was an 8.x test no?

#1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() seemed reasonable to me, so getting that patch working with the test from #96 would be good. If #62 is still in the running then the same thing with that for comparison.

Moving to CNW, 'to be ported' is generally for things that have been committed already.

Changing title to more closely match solution than original problem

Added some more problem into the title, this is still a bug and not a feature to be introduced.

#96 is indeed a D8 test.

So for now someone needs to combine the test at #96 with a port of the patch at #62.

Or re-start with the test in #96 and a drupal_alter approach ... (which I personally find cleaner)

Here is the port of #62 merged with the #96 test.
It's my first reroll for D8 so I quadruple checked everything and hope I haven't missed anything.
I used Drupal ladder lesson on re-roll patch and some git log to find the commit where #62 patch applied.

Regarding the drupal_alter(), I'd rather let this for more experienced developers.

#116: drupal-939462-116.patch queued for re-testing.

I'm not sure why my patch is failing the tests, the installation went smoothly on my server.
Any suggestion ?

#116: drupal-939462-116.patch queued for re-testing.

New patch that applies to HEAD.

Pushing the patch without the tests.

Seems like it's failing on

Detect a Drupal installation failure
Ensure that you can perform a fresh Drupal install.

Which sounds more like a testbot problem than a patch problem.

Ok I've been able to reproduce this, WSOD on the install page, some theme function may have change too much during this time, I'll have another look at it on Sunday.

New try, a problem with the patch during maintenance mode was causing the WSOD. I modified this in the theme() function:
if (isset($info['template'])) {
by
if (isset($info['template']) && !defined('MAINTENANCE_MODE')) {

Incorrect patch in #129, sent corrected version.

Last attempt for me.

OK have rerolled and revised some things to get some of the test failures out. I found the following:

1.
$template_candidates = array($info['template'], str_replace($info['theme path'] . '/templates/', '', $info['template']));

This made the path for Bartik templates wrong. Revised '/templates/' to '/' on line #1314 in patched file.

2.
'theme_hook_original' => $original_hook,

This returned render array errors all over the place. So changed to '#theme_hook_original' and they are gone.

Still lots of errors, but hoping less then before, and maybe I can help get those down.
Of course my changes may be in error themselves.

If anyone with more experience would like to jump in here, please feel free.

And sorry, no interdiff as I did not realize I needed to create it before the patch.

Actually I can create an interdiff, just difficult to know which patch to make it to.

I too patch in #84 and #133 and made a cross patch of them. Mostly followed #84, which is very similar to #133.

Anyway here is interdiff to #133.

OK that did not work, still something definitely wrong with paths being called.

For modules the path is repeated from 'core'.
For themes the path is repeated from 'core' and an extra 'templates' directory tacked on the end.
Also not picking up 'twig' templates when they are present.

Will investigate all path's in the code, see if I can track this down.

If anyone has more knowledge/idea of what is wrong here, please feel free to chime in.

OK found out why the file path was being repeated

e.g. include(C:\xampp\htdocs\drupal8/core/themes/seven/templates/core/themes/seven/templates/templates/page.tpl.php)

Still can not get rid of second /templates/ at the end. If I remove it from here

      // Do the same for template files.
      if (isset($info['template']) && !defined('MAINTENANCE_MODE')) {
        $result[$hook]['template'] = $themed_path . '/templates/' . $info['template'];
      }

Then it also gets removed from system.module calls in .include files templates.
Causing even more errors.

Any help or insight would be appreciated.

Hope I have not introduced a whole bunch more of errors.

hhmmm

OK lets look at that again.

OK was a little overzealous with my deletions, added back in part of previous change.

Should be able to install site for tests now.

I have looked at this over and over. Seem to be chasing my tail around the same problems. Unable to solve this myself (Not enough experience), however I hope my observations below will help someone solve this.

With patch #142 applied the following occurs.

1) On lines #484-#487 of theme.inc the following code "allows system.module to declare theme functions on behalf of core .include files".

      // Do the same for template files.
      if (isset($info['template']) && !defined('MAINTENANCE_MODE')) {
        $result[$hook]['template'] = $themed_path . '/templates/' . $info['template'];
      }

However it also adds "/templates/" a second time to the end of theme paths, so theme templates are not found.
If you remove it however, then system module templates are not found.
It seems that somewhere else in core, the theme path that gets passed to "$themed_path" has "/templates/" already declared.

This is one of the biggest show stoppers and may be hiding many other issues.

2) Around lines #512-#519 code was introduced to enable finding the best theme engine for the template (with the introduction of twig engine). However this included code already present to render the output using the template file around lines #1152-#1156.

I am not sure if that is required twice or not, but I did find that it introduced a problem with paths for template files.
If the following:

    if (isset($info['path'])) {
      $template_file = $info['path'] . '/' . $template_file;
    }

was removed altogether from both places, then the path for template files was not repeated.

include(C:\xampp\htdocs\drupal8/core/themes/seven/templates/core/themes/seven/templates/templates/page.tpl.php)

became

include(C:\xampp\htdocs\drupal8/core/themes/seven/templates/templates/page.tpl.php)

however, this only worked on already installed site. If you tried a new install with this in place, you got a WSOD for install page.

Re-introduce in only one place, and the double path was also re-introduced, so it is needed for install, but creates incorrect paths otherwise.

As you can see, a lot of circular path issues here.

I am sure there is probably an easy fix for all this, but it is beyond my experience level to find the solution.

Therefore, I will give this up so someone else can work on this.

Hope I have not wasted too much time.

Taking this on again. Could not leave it alone.
Think I may have solved all the path issues at least. Basically did less messing with the existing code. Of course this may mean the issue does not get solved, but at least have a firmer footing for moving on with this.
Lets see what test bot thinks.

Oopps here bot.

OK that's much better. Seems my change from 'theme_hook_original' to '#theme_hook_original' was either in error or I did not carry it through to later in the code when it is called. Testing locally to see what the fix is.

OK think I have fixed the last issue of 'theme_hook_original' lets see what else comes up.

2 fails! We're getting close! Lots of love!

Finally I think I have fixed the remaining 2 issues, at first I thought the problem was that we have not actually fixed the main issue and the tests were showing this. But having gone over and over the tests, I think there were some issues with our tests themselves. This patch fixes that as far as I can tell.

Of course, I may be completely wrong and have just massaged the tests to pass, without addressing the real issue.
So please, someone with more experience then me, look this over and make sure I am not working around the tests just to get things green.

I really am a novice, so please assume nothing :-)

Anyway here goes, lets see what the bot says anyway.

Ooppps there's a typo for you, lets try that again.

OK getting over excited here :P. Lets try that with the bot.

Right, that's it for me :-)

Next step is for someone with far more experience to review this and manually test if we have actually solved the issue and I have not just bent the patch to get green test results.

Unassigned from myself, as if this is not actually fixing the issue, then I have no clue how to make it work.

Good luck.

Wow, amazing work @mbrett5062!

I've rerolled the patch to reduce some of the superfluous changes, hopefully I didn't mess anything up.

I've yet to really understand the changes made, but I think the test case I wrote was pretty accurate, and this passing is a great sign!

I don't understand the title change in #112 and #113. It seems like someone tried to merge 2 or 3 issues together?

Retitling as the only bug we know this fixes is the one described in #7 and with a test case in #92.

That is definitely on my list of issues to review. (if someone else beats me to it, feel free to unassign me)

+++ b/core/includes/theme.incundefined
@@ -554,31 +569,90 @@ function _theme_process_registry(&$cache, $name, $type, $theme, $path) {
+  // Get all user defined functions.
+  list(, $user_func) = array_values(get_defined_functions());
+  $user_func = array_combine($user_func, $user_func);
+
+  // Gather prefixes. This will be used to limit the found functions to the
+  // expected naming conventions.
+  $prefixes = module_list();
+  if ($theme_engine) {
+    $prefixes[] = $theme_engine . '_engine';
+  }
+  foreach ($base_theme as $base) {
+    $prefixes[] = $base->name;
+  }
+  $prefixes[] = $theme->name;
+
+  // Collect all known hooks. Discovered functions must be based on a known
+  // hook.
+  $hooks = implode('|', array_keys($cache));
+
+  // Collect all variable processor functions in the correct order.
+  $processors = array();
+  foreach ($prefixes as $prefix) {
+    $processors += preg_grep("/^{$prefix}_(pre)?process_($hooks)(__)?/", $user_func);
+  }

Nice work on the patch.. However, filtering out (pre)process functions like that if they don't match a certain naming pattern is unacceptable. This prevents custom modules/themes to hook into the theme registry with hook_theme_registry_alter() and manually register custom functions.

I just took a quick glance at the patch so forgive me if I am interpreting this chunk of code incorrectly.

@tim.plunkett, the main change I made to tests was the addition to 'template.php' at the end of the patch. This seemed to be what was missing when you and I arrived at 2 fails each. With this inclusion, all tests pass. The only thing that worries me, is that I wonder if the tests should have passed without it, and am I just cheating the tests.

That's done in _theme_process_registry(), which is called by _theme_build_registry().
_theme_build_registry() is also what invokes hook_theme_registry_alter(), so it will still be able to add whatever it needs after that filtering.

As noted by @catch in #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core, while this issue is not actually a blocker for consolidating theme functions, this bug becomes more obvious the greater the use of hook suggestions we introduce.

If absolutely necessary we can make that issue critical though given a wider use of suggestions.

See the following, which will all show this bug. (From theme functions consolidation meta issue)

#1812724: Consolidate all form element templates and add theme_hook_suggestons
#1819284: [meta] Consolidate all form element container templates, and add theme_hook_suggestions
#1812684: [meta] Consolidate all table templates and add theme_hook_suggestions
#1813426: [meta] Consolidate all item list templates and add theme_hook_suggestions
#1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays

So I think we should move this to critical and get this in now.

Rerolled.

@@ function _theme_process_registry(&$cache, $name, $type, $theme, $path) {
+  // Get all user defined functions.
+  list(, $user_func) = array_values(get_defined_functions());
+  $user_func = array_combine($user_func, $user_func);
...
+  // Collect all known hooks. Discovered functions must be based on a known
+  // hook.
+  $hooks = implode('|', array_keys($cache));
...
+  foreach ($prefixes as $prefix) {
+    $processors += preg_grep("/^{$prefix}_(pre)?process_($hooks)(__)?/", $user_func);
+  }

Hm. This will be painfully slow.

get_defined_functions() is a huge list. $cache is a huge list. And $hooks is a huge list. We're grepping over all functions not only once, but once for every extension in the system.

The concatenated $hooks in the PCRE additionally have a good chance of exceeding the maximum limit for the PCRE subject.

We had critical bug reports in the past for some similar code in (IIRC) Schema API already — and that code grepped over all functions only once.

So, hm. While the rest of the code seems to be make sense (but could use some higher-level comments in various spots to explain a bit more what is done and why it is done [instead or in addition of how it is done]), I suspect that we'll run into major performance problems with the proposed implementation. :-/

Off-hand, I can't provide a suggestion for how to address this. The only possible solution I can think of would be a pretty radical change:

Instead of auto-discovering (pre)processor functions, require everyone to register them via hook_theme(). Leave the auto-discovery for actual theme_ function overrides and templates only.

TBH, I almost guess that this sounds uglier than it actually is. We're generally moving towards a much more declarative system with D8, so the auto-discovery behavior for theme processing functions is kind of a legacy alien in that architecture anyway. It would cut away and resolve the performance problems entirely, and most likely simplify the theme registry processing to a pretty big extent.

+1 for the declarations idea.

Auto discovery is nice but not really required and not worth a performance hit IMO.

From a themer's DX perspective, overriding theme functions is dead easy. Nothing new to learn, just copy-paste-rename-edit-clearcaches. Implementing new theme functions through hook_theme introduces a lot of (useful) concepts, but it's way too much for simple suggestions.

OTOH, declaring functions in hook_theme would make it easier for themer's to inherit someone else's work. So while I agree with the declarative approach, I'd ask to please make it easier. Right now, declaring a suggestion requires me to declare it's base hook, variables, render element, etc. Can't I just declare the base hook if I'm not overriding anything?

Declaration of the variables, render element, etc. is done in hook_theme and that would remain as is.

This is for declaring specific preprocessor functions for an item already declared in hook_theme so it should be much simpler.

Actually, this is for declaring preprocessor functions for theme suggestions implementations. Declaring a suggestion implementation in a theme's hook_theme should be enough to get its preprocess functions running, right?

I really like @Sun's idea in #165 about declaring, but I wonder what the experience of that would be for a theme dev...

Let's take views as an example. I want to override a specific display of a specific view: views-view-fields--city--default.html.twig. Do I need to register the template before I can override it? I assume no. If I wanted to add a preprocessor for that template file, would I need to register it? I assume yes. I think this is okay since the preprocessor needs to be in template.php anyway, so adding a hook_theme in there to let the system know about it wouldn't be too painful.

Now, let's take item-list--user as an example. I want to add a preprocessor for the user list, but I don't want to change any markup. Please, don't make me add a new template file that just includes item-list.html.twig, but if I have to I will gladly register a new preprocess function in template.php.

I'm also more okay with this approach than I would have been in D7, because I think that with all templates in Twig (fingers crossed) I dont expect there will be nearly as much preprocessing going on to start with. The more savvy theme devs who want to use preprocess should have no problems registering new functions.

However, I think I'd still be happier if we could find a performant way to auto-discover all the preprocess functions as well as the template for the theme hook called.

Isn't there a way we can improve the time it takes to find all the matching functions? In this case, we know what the names of the functions will be, we're not in the same boat as modules where it can start with anything. Can't we sort the functions in alphabetical order or something, and only search the ones that are close matches?

Speaking of how modules do things... what about lazily building up the theme registry instad of doing it all at once? Modules do this by hook, every time a hook is called only the functions that implement that hook are registered. Can we do the same thing for the theme registry by building it up one theme hook at a time? So, for example, when we're on a page with the user list and theme(array('item_list__user', 'item_list')) is called, we could register all preprocess functions for item-list--user and item-list, and when we hit a page with node links, we can add preprocess functions for item-list--links to the registry too.

Lazy building the theme registry could work I think, that was one of my intended follow-ups to the initial CacheArray patches (for schema registry too) but I never got to it. At the moment we cache a 'full' theme registry, then consult that at runtime on CacheArray misses, we'd need to get rid of that full cache and try to do the minimum in the CacheArray miss to populate it correctly. Only question is how much work that ends up being each time there's a cache miss vs. buidling the whole thing at once but we might be able to do a partial build then fill in the gaps too.

I can't see how lazy-building improves the situation with #165 — we'd still have to iterate over all functions that are currently defined in order to dynamically find applicable preprocessors and processors.

I'd actually think it would make the situation worse, because we're not able to plan and optimize that discovery ahead of time; i.e., the loop over all functions would potentially have to re-run for every single theme() call that specifies a different theme hook name. For example, just 'input__foo' and 'input__bar' are different and need to discover different (pre)processors.

Aside from that, @jenlampton's interpretation of my proposal in #170 appears to be correct — everyone would only have to declare (pre)process callbacks, and because you're in working in PHP/template.php already, having to additionally register the callback in hook_theme() doesn't really look very unusual/cumbersome to me.

Just to clarify, which of the following would you have to explicitly declare?

With only node.tpl.php present:
1. mytheme_preprocess_node
2. mytheme_preprocess_node__article

With node--article.tpl.php present
3. mytheme_preprocess_node__article

As I understand, we're only changing for case 2. If that's true, then this sounds good to me.

Currently, #1 and #3 work currently AFAIK, and adding an extra step for those to continue to work sounds unfortunate to me.

FWIW, we actually have some pretty heavy name clashes with the auto-discovered hook_process_THEMEHOOK() already:
http://api.drupal.org/api/search/8/_process_

Quite a couple of those are NOT theme process functions, but form element #process callbacks. It's a mere coincidence of extreme luck that those functions are not invoked as theme process functions. ;)

I've tried to whip up a proof of concept patch for explicit (pre)process function declarations over the past hours, but the current theme system code is beyond my code sanity threshold. So after debugging and backtrace-ing for hours, I'll now approach the reverse, rewrite and re-implement the whole thing more or less from scratch as a proper service, ensuring that the resulting registry is mostly identical to the current. Totally requires a separate issue, of course; #1886448: Rewrite the theme registry into a proper service

Selfishly reuploading a reroll of #62 for D7

@sun and @timplunkett discussed this issue in IRC, and it sounds like it's going to be really hard to make progress here without #1886448: Rewrite the theme registry into a proper service. I bumped that issue to critical; let's postpone this on that to make it clear what our current status is.

I think this may be the issue I am having, but I am not 100% sure.

I am currently implementing the Foundation theme in Drupal 8 as Twig, and noticed functions like this are not working:

• theme_menu_link
• theme_menu_tree
• theme_menu_tree__MENU_NAME
• theme_menu_local_tasks

etc.. if I edit the theme .info.yml file and remove the 'engine' attribute, those functions begin to work, but I would assume because D8 thinks up front that the theme is phptemplate? I noticed that the twig_engine function does not look for theme functions like phptemplate_engine does, which is what the patches above seem to attempt to address.

I have a BoF tomorrow for Foundation 4 in Drupal 8 and would like to crank through on the conversion but this might stop us in our tracks (for the time being).

Hi #177, Hello from the Coders Lounge at DoubleTree.

We have that fixed within the removal of double engine code from http://drupal.org/node/1806478.

For now you can just add to the twig engine that it should search for theme_ functions as well.

Please feel free to base your theme upon the Twig-Megapatch from http://drupal.org/node/1987510.

Thanks! Hit us up in the coders lounge tomorrow or today / yesterday in Coders Lounge at Double Tree.

Thanks a lot,

Your Twig-Team!

Thanks Fabianx, that's what I did short term (change twig_theme()) - I will take a look at that patch.

I think this is also going to end up being a duplicate of #2004872: [meta] Theme system architecture changes.

closing as dupe.

I'm not clear what this is a duplicate of.

@catch #180? #2004872: [meta] Theme system architecture changes

That's a meta issue though, there needs to be a specific issue with a patch to mark this as duplicate of, and that issue should be a critical bug too (or be prepared to not fix it at all for 8.x).

Okay, how about dupe of #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()

It's got a patch on it, and it should solve this problem :) I'll bump up the priority over there.

Thanks @tim.plunkett!

As noted in #1751194-35: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter(), that issue alone doesn't solve this problem. Or at least doesn't make the test case in #96 pass. I think it will make this issue easier to solve though, even if it ends up getting solved from the theme registry issue.

So… tentatively bumping to 8.x and re-postponing for now. Hopefully we can get this solved soon.

#2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() combined with #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() will fix this issue in 8.x. Because that issue is a major shift in how the theme system will work, bumping this back to 7.x so @tim.plunkett's patch in #175 can be worked on and committed against 7.x.

This is for 7.x. The 8.x equivalent #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() already has the parent.

I'm moving this back to 8.x. We're not going to be moving forward with the addition of the hook_theme_prepare() API in 8.x (which is now postponed to 9.x). I'm also going to try and take a stab at this sometime this week.

Sorry for creating a lot of noise (bad code day :)

The problem is no longer abstract/hypothetical; we have a real use-case in core now.

AFAICS, the remaining task is to extract the original/scope-creep changes from #1886448: Rewrite the theme registry into a proper service, so that they can be committed here. — At least, that appears to be the only solution of all that have been proposed thus far that actually has (had) a working implementation.

Another use case is #1962846: Use field instance name for header of comment list, drop comment-wrapper template

Bumping this to a beta blocker since there's a decent chance it'll rely on non-trivial API changes to fix.

a decent chance it'll rely on non-trivial API changes to fix

I've heard-tell that these changes might include registering preprocess functions for a theme implementation rather than a discovery process for preprocess implementations. So that means we:

1. augment all of the core theme implementations
2. Implement hook_theme_registry_alter (docs) for all the existing implementations of specific hooks
3. Write a thorough change notice so that module devs poriting during the beta can make the same changes.

This feels like a big impact for core, but not-so-huge for contrib. I'd lobby that it not block the beta cut. The patterns we'd move to already exist in other places meaning the registering of functions (similar to #pre_render) on what's essentially an info hook (hook_theme).

@jessebeach Take a look at #1962846: Use field instance name for header of comment list, drop comment-wrapper template to use additional preprocess function for comment field I need anyway register theme implementation now, the same applies to node title field now. If you think that there's other way - please explain

I cannot seem to get enough time to devote to this. I'll still follow, review and offer my feedback but I cannot work on code dev. There's still a lot of code that could probably be leveraged/moved from #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK().

Very good that this is a beta blocker... :-)))

Since #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() is now fixed for D8 (great job, everyone who worked on that), my concerns from #6 are no longer relevant, so I support this issue. I'm pretty tempted to try a drupal_alter()-like approach (see #8), even though #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK(), which would have made that easier, has been bumped to D9. But, that just mean instead of using drupal_alter() as-is, we need a copy of it that works for the naming pattern of *_preprocess_*().

@sun and others: do you foresee a problem with that working out? Looks like the only reason this issue was tagged as beta blocking is due to #195's suggestion to use a manually specified 'preprocess' array in each hook_theme() implementation, but that was rejected in #1886448-96: Rewrite the theme registry into a proper service as bad DX. I don't really see why a drupal_alter()-like approach couldn't work though, in which case, there's no need for this to be beta blocking.

I also found bug #2257667: Theme hook template_preprocess_item_list__search_results() not fired, but was pointed here by sun. There seems to me a lot of suggestions broken.

From November 2010 in #6:

I'm marking this issue major, rather than critical, because this is something that can be done after RC, and even in a 7.1 release.

From January 2013 in #163 (when we had a passing patch):

I think we should move this to critical and get this in now.

We knew of this bug before D7.0 came out, and it didn't block release. The critical status was in order to get some momentum going, and really isn't applicable now.

@catch, @alexpott, @Dries, and @webchick discussed this issue this morning and agreed that it is not beta-blocking.