Proposed commit message:

Issue #939462 by lauriii, Antti J. Salminen, NROTC_Webmaster, mbrett5062, tim.plunkett, tostinni, rteijeiro, dvessel, barraponto, theapi, joelpittet, cilefen, tuutti, drzraf, Fabianx, markcarver, xjm, catch, jenlampton, sun, effulgentsia, Cottser, davidhernandez, kscheirer, andypost, akalata, rooby, hass, fubhy, jhodgdon, lemunet, gleroux02, mike stewart, kevinquillen, MXT, mlncn, becw, PavanL, chriscalip: Specific preprocess functions for theme hook suggestions are not invoked

General summary

  • This is not considered a beta blocker or critical issue for D8.
  • While patches exist in this thread for D7 (and may work for you), official fix will be to D8 first -- please do not switch issue metadata until we are ready to start the D7 backport.

Problem/Motivation

When you have a template suggestion available and are using that template, preprocess functions following naming conventions provided via hook_theme_suggestions_HOOK() are not being recognized, for example:

MYTHEME_preprocess_node__article()
MYTHEME_preprocess_block__search_form_block()
MYMODULE_preprocess_page__front()
MYMODULE_item_list__search_results()
etc.

Proposed resolution

Add a post-process step to the theme registry build to look for these types of preprocess functions and add them to the theme registry.

Remaining tasks

Patch needs review.

User interface changes

N/A

API changes

Removes the global drupal_group_functions_by_prefix() (moves it to a class to make things testable) which was added May 13, 2015: #2339447: Improve theme registry build performance by 85%

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, there was similar functionality to this in Drupal 6, or at least in Views
Issue priority Major because it represents a big improvement to themer/developer experience.
Disruption Little to no disruption, just an addition that core and contrib can choose to use or not use.

Files: 
CommentFileSizeAuthor
#359 interdiff.txt2.94 KBlauriii
#359 specific_preprocess-939462-359.patch23.53 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,671 pass(es). View
#355 drupal_939462_355.patch23.05 KBXano
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,019 pass(es). View
#355 interdiff.txt1.2 KBXano

Comments

sense-design’s picture

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');
}
gleroux02’s picture

You are right that the code that you provided will allow the specific views preprocess function to get picked up, but I should not have to declare each preprocess function in my theme by adding it to a hook_theme function. It should just get picked up if that tpl is in the theme.

becw’s picture

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

becw’s picture

MXT’s picture

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

effulgentsia’s picture

Title: Specific template preprocess functions are not working » Specific preprocess functions for theme hook suggestions are not invoked
Project: Views » Drupal core
Version: 7.x-3.x-dev » 7.x-dev
Component: Code » theme system
Priority: Normal » Major

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.

sun’s picture

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.

effulgentsia’s picture

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.

helior’s picture

+1

mlncn’s picture

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.

droplet’s picture

Subscribing

fubhy’s picture

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

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.

tinefin’s picture

Subscribing

temosayin’s picture

subscribing also

lemunet’s picture

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
}
carn1x’s picture

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

lemunet’s picture

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

dvessel’s picture

Status: Active » Needs review
FileSize
18.62 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [specific_preprocess_hook_suggestions_ 939462_19.patch] from [drupal.org]. View

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.

dvessel’s picture

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.

dvessel’s picture

Status: Needs work » Needs review
FileSize
18.65 KB
FAILED: [[SimpleTest]]: [MySQL] Fetch test patch: failed to retrieve [specific_preprocess_hook_suggestions_ 939462_21.patch] from [drupal.org]. View

Lets try that again. That was an old patch.

dvessel’s picture

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

dvessel’s picture

FileSize
18.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch specific_preprocess_hook_suggestions_939462_25.patch. Unable to apply patch. See the log in the details link for more information. View

Eh, hidden space in the file name.

dvessel’s picture

Status: Needs work » Needs review
dvessel’s picture

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.

dvessel’s picture

Status: Needs work » Needs review
marcp’s picture

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

dvessel’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

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

MXT’s picture

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!

JGO’s picture

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

chriscalip’s picture

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

chriscalip’s picture

Status: Needs work » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -DX (Developer Experience), -views, -preprocess functions, -drupal 7 +Needs tests, +Needs profiling

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.

jox’s picture

Subscribing.

rooby’s picture

Subscribing

rfabbri’s picture

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

xjm’s picture

Tagging issues not yet using summary template.

Agileware’s picture

Sub

jherencia’s picture

catch’s picture

Issue tags: +needs backport to D7

Tagging for backport.

aroq’s picture

Subscribing.

jpargana’s picture

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

xjm’s picture

Status: Needs review » Needs work

This will need to be rerolled now.

jenlampton’s picture

tagging for learnability.

drzraf’s picture

FileSize
18.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 939462-48.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

(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']))

theapi’s picture

xjm’s picture

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

theapi’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
18.67 KB
FAILED: [[SimpleTest]]: [MySQL] 37,116 pass(es), 0 fail(s), and 9,499 exception(es). View

Git version of the patch at #25

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

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.

drzraf’s picture

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

xjm’s picture

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

catch’s picture

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.

theapi’s picture

FileSize
18.67 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch specific_preprocess_hook_suggestions_939462_57.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

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.

jenlampton’s picture

Status: Needs work » Needs review

Changing status for testbot.

xjm’s picture

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

jschrab’s picture

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);
}
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.73 KB
PASSED: [[SimpleTest]]: [MySQL] 34,613 pass(es). View

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

tim.plunkett’s picture

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

xjm’s picture

Status: Needs review » Needs work

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!

xjm’s picture

Issue tags: +Novice

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

mike stewart’s picture

Assigned: Unassigned » mike stewart

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

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

bvanmeurs’s picture

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

xjm’s picture

@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!

mike stewart’s picture

Assigned: mike stewart » Unassigned

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 :-]

tim.plunkett’s picture

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

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
18.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-939462-70.patch. Unable to apply patch. See the log in the details link for more information. View

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.

effulgentsia’s picture

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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
20.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-939462-74.patch. Unable to apply patch. See the log in the details link for more information. View
1.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-test.patch. Unable to apply patch. See the log in the details link for more information. View

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

NROTC_Webmaster’s picture

FileSize
20.57 KB
FAILED: [[SimpleTest]]: [MySQL] 34,791 pass(es), 2 fail(s), and 4 exception(s). View
1.56 KB
FAILED: [[SimpleTest]]: [MySQL] 34,802 pass(es), 1 fail(s), and 0 exception(s). View

Lets try again.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
NROTC_Webmaster’s picture

FileSize
20.32 KB
FAILED: [[SimpleTest]]: [MySQL] 34,787 pass(es), 3 fail(s), and 4 exception(s). View
1.56 KB
FAILED: [[SimpleTest]]: [MySQL] 34,806 pass(es), 1 fail(s), and 0 exception(s). View

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.

xjm’s picture

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

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

tim.plunkett’s picture

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

NROTC_Webmaster’s picture

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.

gitesh.koli’s picture

Status: Needs work » Needs review
NROTC_Webmaster’s picture

FileSize
20.26 KB
FAILED: [[SimpleTest]]: [MySQL] 35,915 pass(es), 2 fail(s), and 0 exception(s). View

tim.plunkett,

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

barraponto’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
18.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

issues:

barraponto’s picture

Status: Needs work » Needs review
FileSize
997 bytes
18.06 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

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

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I'm going to try to revisit this soon.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Oh yeah I tried and failed hard. No idea.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.73 KB
FAILED: [[SimpleTest]]: [MySQL] 36,696 pass(es), 2 fail(s), and 0 exception(s). View

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.

tim.plunkett’s picture

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?

fubhy’s picture

Yes, that should be the expected behavior / result.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
FAILED: [[SimpleTest]]: [MySQL] 47,588 pass(es), 2 fail(s), and 0 exception(s). View

Glad to hear it. Rerolled for PSR-0.

effulgentsia’s picture

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.

effulgentsia’s picture

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.

brunogoossens’s picture

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']);
}
kscheirer’s picture

Status: Needs work » Reviewed & tested by the community

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.

kscheirer’s picture

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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

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

catch’s picture

Status: Needs work » Needs review

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

Fabianx’s picture

Issue tags: +Twig

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

Fabianx’s picture

sun’s picture

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

Fabianx’s picture

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.

mitchell’s picture

Status: Needs work » Patch (to be ported)

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

catch’s picture

Status: Patch (to be ported) » Needs work

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.

jenlampton’s picture

Title: Specific preprocess functions for theme hook suggestions are not invoked » Introduce hook_template_suggestions_HOOK to separate suggestions from variable preprocessing

Changing title to more closely match solution than original problem

Fabianx’s picture

Title: Introduce hook_template_suggestions_HOOK to separate suggestions from variable preprocessing » Add hook_template_suggestions_HOOK to separate suggestions from variable preprocessing to fix preprocess for suggestions

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

kscheirer’s picture

#96 is indeed a D8 test.

Fabianx’s picture

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)

tostinni’s picture

Status: Needs work » Needs review
FileSize
21.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-939462-116_2.patch. Unable to apply patch. See the log in the details link for more information. View

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.

tostinni’s picture

Status: Needs work » Needs review

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

tostinni’s picture

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

c4rl’s picture

tostinni’s picture

Status: Needs work » Needs review
FileSize
21.02 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

New patch that applies to HEAD.

tostinni’s picture

Status: Needs work » Needs review
FileSize
17.18 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Pushing the patch without the tests.

kscheirer’s picture

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.

tostinni’s picture

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.

tostinni’s picture

Status: Needs work » Needs review
FileSize
12.24 KB
FAILED: [[SimpleTest]]: [MySQL] 47,019 pass(es), 2,154 fail(s), and 15,863 exception(s). View

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')) {

tostinni’s picture

Status: Needs work » Needs review
FileSize
13.6 KB
FAILED: [[SimpleTest]]: [MySQL] 47,898 pass(es), 64 fail(s), and 37,848 exception(s). View

Incorrect patch in #129, sent corrected version.

tostinni’s picture

Status: Needs work » Needs review
FileSize
21.05 KB
FAILED: [[SimpleTest]]: [MySQL] 47,128 pass(es), 2,132 fail(s), and 15,580 exception(s). View

Last attempt for me.

mbrett5062’s picture

Assigned: Unassigned » mbrett5062
Status: Needs work » Needs review
FileSize
21.53 KB
FAILED: [[SimpleTest]]: [MySQL] 47,937 pass(es), 2,197 fail(s), and 15,787 exception(s). View

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.

mbrett5062’s picture

FileSize
8.38 KB

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.

mbrett5062’s picture

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.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
22.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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.

mbrett5062’s picture

hhmmm

OK lets look at that again.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
733 bytes
22.1 KB
FAILED: [[SimpleTest]]: [MySQL] 47,967 pass(es), 2,183 fail(s), and 15,826 exception(s). View

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

Should be able to install site for tests now.

mbrett5062’s picture

Assigned: mbrett5062 » Unassigned

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.

mbrett5062’s picture

Assigned: Unassigned » mbrett5062
FileSize
20.94 KB
FAILED: [[SimpleTest]]: [MySQL] 48,948 pass(es), 20 fail(s), and 57 exception(s). View

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.

mbrett5062’s picture

Status: Needs work » Needs review

Oopps here bot.

mbrett5062’s picture

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.

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
21.43 KB
FAILED: [[SimpleTest]]: [MySQL] 48,896 pass(es), 2 fail(s), and 0 exception(s). View

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

barraponto’s picture

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

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
22.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/theme_test/theme_test.module. View

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.

mbrett5062’s picture

FileSize
22.19 KB
PASSED: [[SimpleTest]]: [MySQL] 48,914 pass(es). View

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

mbrett5062’s picture

Status: Needs work » Needs review

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

mbrett5062’s picture

Assigned: mbrett5062 » Unassigned

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.

tim.plunkett’s picture

FileSize
19.99 KB
PASSED: [[SimpleTest]]: [MySQL] 48,985 pass(es). View

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!

tim.plunkett’s picture

Title: Add hook_template_suggestions_HOOK to separate suggestions from variable preprocessing to fix preprocess for suggestions » Specific preprocess functions for theme hook suggestions are not invoked

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.

Fabianx’s picture

Assigned: Unassigned » Fabianx

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

fubhy’s picture

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

mbrett5062’s picture

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

tim.plunkett’s picture

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.

mbrett5062’s picture

Priority: Major » Critical

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.

tim.plunkett’s picture

FileSize
19.99 KB
PASSED: [[SimpleTest]]: [MySQL] 50,558 pass(es). View

Rerolled.

sun’s picture

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

rooby’s picture

+1 for the declarations idea.

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

barraponto’s picture

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?

rooby’s picture

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.

barraponto’s picture

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?

jenlampton’s picture

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.

catch’s picture

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.

sun’s picture

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.

tim.plunkett’s picture

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.

sun’s picture

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

tim.plunkett’s picture

Selfishly reuploading a reroll of #62 for D7

xjm’s picture

Status: Needs review » Postponed

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

kevinquillen’s picture

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

Fabianx’s picture

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!

kevinquillen’s picture

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

jenlampton’s picture

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

jenlampton’s picture

Status: Postponed » Closed (duplicate)

closing as dupe.

catch’s picture

Status: Closed (duplicate) » Active

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

yannickoo’s picture

catch’s picture

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

jenlampton’s picture

Status: Active » Closed (duplicate)

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.

tim.plunkett’s picture

Version: 8.x-dev » 7.x-dev
Assigned: Fabianx » Unassigned
Status: Closed (duplicate) » Needs work
Cottser’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Postponed

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.

markcarver’s picture

Version: 8.x-dev » 7.x-dev
Status: Postponed » Needs work
Issue tags: -needs backport to D7

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

PavanL’s picture

Issue summary: View changes
PavanL’s picture

Issue summary: View changes
sun’s picture

markcarver’s picture

markcarver’s picture

Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » markcarver
Issue tags: +Twig, +theme system cleanup, +DX (Developer Experience), +FX (Front End Experience)

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.

markcarver’s picture

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

sun’s picture

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.

andypost’s picture

catch’s picture

Issue tags: +beta blocker

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

jessebeach’s picture

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

andypost’s picture

@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

markcarver’s picture

Assigned: markcarver » Unassigned

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

hass’s picture

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

effulgentsia’s picture

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.

hass’s picture

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.

tim.plunkett’s picture

Priority: Critical » Major

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.

xjm’s picture

Issue tags: -beta blocker

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

markcarver’s picture

Assigned: Unassigned » markcarver
Issue tags: +sprint, +focus
Related issues: +#2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK()
andypost’s picture

markcarver’s picture

Assigned: markcarver » Unassigned

Sorry. I did not get to this at DC Austin and I really do not have the time now to start digging into this now. Hopefully someone else can tackle it.

jhodgdon’s picture

This issue badly needs an issue summary update. I read the first few comments from 4 years ago, but ... at 208... what's the plan? What's the problem being addressed? etc.

akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
mgifford’s picture

lauriii’s picture

I did some debugging for this. Uploading what I've been testing around. Probably tests in the patch are most useful.

jhodgdon’s picture

The summary is looking much better. It's still lacking in the Problem section ... maybe an example, like "For instance, on the foo__bar hook suggestions, you'd want _____() to be called, but it isn't." or something like that. Then in Proposed Resolution, it would be helpful to state something like "Functions named ... will be automatically invoked", if that is the plan, or to state what the plan actually is. Reading the summary, I still have no clear idea what the plan is.

And when you have a patch that is ready for review, or even that you want people and/or the bot to take a look at (even if it still has problems), don't forget to set the status to "needs review".

lauriii’s picture

Issue summary: View changes
jhodgdon’s picture

Thanks, that clears it up!

lauriii’s picture

I've been wondering how this should be done.

a)

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

b)

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

With node--article.tpl.php present
1. mytheme_preprocess_node
2. mytheme_preprocess_node__article

I think if we do this way a) it will create unnnecessary complexity to it. For me it it sound like it should be more like a function level filtering so that you don't have to do the filtering inside the function.

lauriii’s picture

FileSize
7.22 KB

Still some very rough messing around but just a little more usable format.

davidhernandez’s picture

RE #219 I would say 'b'. mytheme_preprocess_node should always be run.

markcarver’s picture

Yes, "b". Preprocesses should always be accumulative, i.e.: node__article__sidebar should execute the following hooks in this order (if found):

hook_preprocess_node
hook_preprocess_node__article
hook_preprocess_node__article__sidebar

This allows node__article__sidebar to inherit whatever the base hook (and suggestion) preprocess implements. It is then up to the very specific preprocess suggestion to change/remove whatever is inherited (if needed).

@effulgentsia, could you elaborate more on #202? I'm really not sure I follow the "alter" method proposed.

markcarver’s picture

Issue tags: +needs backport to D7

Adding back the backport tag I removed in #188 when I thought this was going to be a 7.x only issue.

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

This might be impossible to implemen without breaking the caching completely. If preprocess functions would be cached with the dynamic theme suggestions we would need some kind of cache tagging etc to make it work at all. Lets see how many things I'll break with this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
15.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,791 pass(es), 6 fail(s), and 4 exception(s). View
712 bytes
lauriii’s picture

FileSize
13.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,773 pass(es), 5 fail(s), and 3 exception(s). View

Last patch was messed up :P

lauriii’s picture

Status: Needs work » Needs review
FileSize
16.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,792 pass(es), 2 fail(s), and 0 exception(s). View
3.34 KB
davidhernandez’s picture

I ran based on #230. This was literally just the default front page after installation; standard profile.

=== 8.0.x..8.0.x compared (550de0b43acdf..550de0f0bfc62):

ct  : 49,543|49,543|0|0.0%
wt  : 214,055|213,528|-527|-0.2%
mu  : 21,885,328|21,885,328|0|0.0%
pmu : 22,055,712|22,055,712|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=550de0b43acdf&...

=== 8.0.x..939462 compared (550de0b43acdf..550de112c2927):

ct  : 49,543|56,832|7,289|14.7%
wt  : 214,055|225,805|11,750|5.5%
mu  : 21,885,328|21,881,112|-4,216|-0.0%
pmu : 22,055,712|22,052,352|-3,360|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=550de0b43acdf&...

lauriii’s picture

Status: Needs work » Needs review
FileSize
16.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,772 pass(es). View
809 bytes
rteijeiro’s picture

FileSize
16.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
3.43 KB

Fixed a few nitpicks in comments.

rteijeiro’s picture

FileSize
16.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,785 pass(es). View
510 bytes

Fixed a small typo.

lauriii’s picture

I was talking with Cottser on IRC and we both agreed that this is probably something that is not possible to get inside core anymore and would need lots of refactoring for the theme registry to make it work how it should work. In order to make it work we would require that preprocess functions are being generated somewhere else than theme registry because its concidered to be static. That would cause regression for hook_theme_registry_alter. Also the fact that preprocess functions would not be cached to theme registry would cause huge impact on the performance side. Im still leaving this open if there is anything else to say.

markcarver’s picture

This isn't a "feature", it's a bug/task (other postponed issues require the ability to preprocess specific suggestions)... it needs to "go in".

I'm not sure how or why this has deviated from the proposed solution in #175 by @tim.plunkett. I'm also not sure why there the bulk of this was moved to run-time and not via the theme registry (which is cached).

Despite #231 (which relates to the run-time discovery), performance tuning can be done in this related issue (which is similar to what Tim kind of semi-implemented in #175).

tim.plunkett’s picture

#175 is actually a reroll of #62, which has been working for over three years, and was a reroll of the work done by @dvessel four years ago (see #19 and #956520-30: Available theme hook suggestions are only exposed when there are preprocess functions defined)

markcarver’s picture

Ah, my mistake. I just looked for the previous older patch (didn't really read the comment). Regardless, I would be more inclined to stay with that approach rather than the current iterations, unless I'm missing something...

lauriii’s picture

Status: Needs work » Needs review
FileSize
24.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,413 pass(es), 19 fail(s), and 5 exception(s). View

Lets see how many test failures I can cause with this :P

lauriii’s picture

Status: Needs work » Needs review
FileSize
12.4 KB
12.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,396 pass(es), 1 fail(s), and 0 exception(s). View

Little bit less crappy patch :P

lauriii’s picture

Status: Needs work » Needs review
Issue tags: +Needs profiling
FileSize
875 bytes
11.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,399 pass(es), 2 fail(s), and 0 exception(s). View
lauriii’s picture

Status: Needs work » Needs review
FileSize
328 bytes
12.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,403 pass(es). View

This should fix the tests

lauriii’s picture

Issue tags: -Needs profiling

I created a performance test on default Drupal frontpage view printing 50 of 150 nodes.

=== 8.0.x..8.0.x compared (55323a80ebb82..55323ac3d89c1):

ct  : 251,865|251,865|0|0.0%
wt  : 303,784|304,031|247|0.1%
mu  : 26,689,328|26,689,328|0|0.0%
pmu : 27,022,440|27,022,440|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55323a80ebb82&...

=== 8.0.x..preprocess-939462-251 compared (55323a80ebb82..55323afe82890):

ct  : 251,865|251,872|7|0.0%
wt  : 303,784|304,476|692|0.2%
mu  : 26,689,328|26,706,696|17,368|0.1%
pmu : 27,022,440|27,040,208|17,768|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=55323a80ebb82&...

rteijeiro’s picture

FileSize
12.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,422 pass(es). View
4.54 KB

Just fixed a few nits.

Fabianx’s picture

Issue tags: +Needs profiling

The kind of profiling we need here is cold-cache performance or benchmarking just the build of the theme registry, when there are lots of to be discovered functions and lots of modules ...

lauriii’s picture

Issue tags: -Needs profiling

Profiled this without any caching.

Run 553b9a86a14e1 uploaded successfully for drupal-perf-lauriii.
Run 553ba00bcf766 uploaded successfully for drupal-perf-lauriii.

=== preprocess..preprocess compared (553b9a86a14e1..553ba00bcf766):

ct  : 3,950,650|3,950,650|0|0.0%
wt  : 6,530,045|6,566,415|36,370|0.6%
mu  : 37,042,400|38,218,592|1,176,192|3.2%
pmu : 37,583,888|38,763,432|1,179,544|3.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=553b9a86a14e1&...

Run 553b9a86a14e1 uploaded successfully for drupal-perf-lauriii.
Run 553ba42a8e87e uploaded successfully for drupal-perf-lauriii.

=== preprocess..preprocess-939462-253 compared (553b9a86a14e1..553ba42a8e87e):

ct  : 3,950,650|3,918,322|-32,328|-0.8%
wt  : 6,530,045|6,928,196|398,151|6.1%
mu  : 37,042,400|38,722,240|1,679,840|4.5%
pmu : 37,583,888|39,263,864|1,679,976|4.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=553b9a86a14e1&...

lauriii’s picture

FileSize
14.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,837 pass(es), 2 fail(s), and 0 exception(s). View

Applied latest patch from #2339447: Improve theme registry build performance by 85% with my changes.

Run 553ba8c2c449d uploaded successfully for drupal-perf-lauriii.
Run 553bb0137a0c9 uploaded successfully for drupal-perf-lauriii.

=== preprocess..preprocess compared (553ba8c2c449d..553bb0137a0c9):

ct  : 3,950,650|3,950,650|0|0.0%
wt  : 6,619,118|6,606,084|-13,034|-0.2%
mu  : 38,856,944|38,856,944|0|0.0%
pmu : 39,402,536|39,402,536|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=553ba8c2c449d&...

Run 553ba8c2c449d uploaded successfully for drupal-perf-lauriii.
Run 553bb054a0d57 uploaded successfully for drupal-perf-lauriii.

=== preprocess..preprocess-939462-253 compared (553ba8c2c449d..553bb054a0d57):

ct  : 3,950,650|3,893,368|-57,282|-1.4%
wt  : 6,619,118|5,566,964|-1,052,154|-15.9%
mu  : 38,856,944|40,602,840|1,745,896|4.5%
pmu : 39,402,536|41,148,528|1,745,992|4.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=553ba8c2c449d&...

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,859 pass(es). View
171 bytes

Hopefully tests are passing now

markcarver’s picture

Status: Needs review » Needs work

#256 shouldn't have been uploaded with the code from #2339447: Improve theme registry build performance by 85%. That is a separate issue and really has nothing to do with this one. I understand performance is something we should be concerned with, but please try to limit the patches in this issue to the task at hand and simply reference that issue when necessary to avoid further confusion.

Edit: p.s. Thanks @lauriii! Not trying to discourage you and really do appreciate you tackling this. I just saw how easily this issue/patch could have caused mass confusion is all.

Fabianx’s picture

#259: That is not true, because we do a _lot_ more checking of prefixes now as far as I have understood so we should show at least that we can improve performance for making that lookup efficient. (and then move it again to a follow-up)

Cottser’s picture

I think this probably can't get in until #2339447: Improve theme registry build performance by 85% gets in, is what it boils down to.

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -343,6 +343,8 @@ protected function build() {
    @@ -535,30 +537,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    
    @@ -535,30 +537,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    -    // Let themes have variable preprocessors even if they didn't register a
    -    // template.
    -    if ($type == 'theme' || $type == 'base_theme') {
    -      foreach ($cache as $hook => $info) {
    -        // Check only if not registered by the theme or engine.
    -        if (empty($result[$hook])) {
    -          if (!isset($info['preprocess functions'])) {
    -            $cache[$hook]['preprocess functions'] = array();
    -          }
    -          // Only use non-hook-specific variable preprocessors for theme hooks
    -          // implemented as templates. See _theme().
    -          if (isset($info['template']) && function_exists($name . '_preprocess')) {
    -            $cache[$hook]['preprocess functions'][] = $name . '_preprocess';
    -          }
    -          if (function_exists($name . '_preprocess_' . $hook)) {
    -            $cache[$hook]['preprocess functions'][] = $name . '_preprocess_' . $hook;
    -            $cache[$hook]['theme path'] = $path;
    

    I think removing these lines entirely (as far as I can see) is a kind of API change. More specifically:

    Only use non-hook-specific variable preprocessors for theme hooks implemented as templates. See _theme().

    This logic could be re-added within postProcessExtension() I'd think.

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -535,30 +537,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   *   - 'preprocess functions': See _theme() for detailed documentation.
    

    Since this is duplicating the docs from processExtension maybe we should just refer to those docs if possible, instead of getting into this type of rabbit hole:

    This might be copy and paste but _theme() doesn't exist anymore. I'm not totally sure the best place to point to for this documentation, unless before it was just pointing to the inline comments in _theme (now \Drupal\Core\Theme\ThemeManager::theme() I believe).

  3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -535,30 +537,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   *   Current active theme.
    +  */
    +  protected function postProcessExtension(array &$cache, $theme) {
    

    Minor: Stars don't align.

joelpittet’s picture

@Cottser that performance issue should have no effect on this and if it gets in the relative performance regression shouldn't change for this bug fix.

Maybe you and @lauriii can work on pushing this bug fix out the door without that other patch?

Antti J. Salminen’s picture

While the other patch alone won't help, to me it looks like this could really benefit from using the same lookup table that would be introduced in #2339447: Improve theme registry build performance by 85%. Specifically the section of code that sun refers to in comment #165.

lauriii’s picture

Status: Needs work » Needs review
FileSize
10.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,437 pass(es). View
4.84 KB

Removed the performance improvements from this patch. I also fixed the nits from Cottser.

Fabianx’s picture

Title: Specific preprocess functions for theme hook suggestions are not invoked » [PP-1] Specific preprocess functions for theme hook suggestions are not invoked
Status: Needs review » Postponed

The patch looks ready, once we can use drupal_get_prefix_grouped_functions() from the other issue, my performance concerns are pretty much moot.

Postponing on that shortly.

lauriii’s picture

Title: [PP-1] Specific preprocess functions for theme hook suggestions are not invoked » Specific preprocess functions for theme hook suggestions are not invoked
Status: Postponed » Needs work

Not being postponed anymore

lauriii’s picture

Status: Needs work » Needs review
FileSize
10.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.25 KB

Yay, this should be a little faster now!

Antti J. Salminen’s picture

FileSize
10.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Tests seem to have failed because of something unrelated to the patch... Here's another iteration. I think this can be a further improvement for performance and gets rid of the really big regexp. Hoping this one gets to the tests now...

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

Sorry, forgot about the interdiff. :) It's attached. And marking needs review in hopes of getting a test run.

lauriii’s picture

@Antti: Thanks! We tested that in the other issue and there it seemed to be slower than running the regexp. Don't know for sure if that applies here but lets test it out

Antti J. Salminen’s picture

@lauriii: Oh okay, I think it might at least scale better and be less in danger of hitting the max limit for regexp size though. To be honest the second one is probably an unlikely issue, PCRE's limits are pretty high these days.

markcarver’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -343,6 +343,8 @@ protected function build() {
    +    $this->postProcessExtension($cache, $this->theme);
    

    This should have a comment above it explaining what it is for, just like $this->processExtension(...).

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +556,104 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   * This completes the theme registry adding missing functions and hooks.
    ...
    +    // Add missing variable processors. This is needed for hooks that do not
    ...
    +    // go missing. This will add the expected function. It also allows modules
    ...
    +        // Add missing processor to existing hook.
    

    The functions/hooks are not "missing" they're being "automatically discovered".

  3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +556,104 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   * @param array $cache
    +   *  @see ::processExtension()
    

    This should be a description, not a @see comment (which should be added at the end of the doc block).

  4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +556,104 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   * @param object $theme
    ...
    +  protected function postProcessExtension(array &$cache, $theme) {
    

    This should be a class, not object.

    I would even further surmise that the method signature should include the expected class of $theme.

  5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +556,104 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +    // Collect all variable processor functions in the correct order.
    

    Generally speaking, these are "preprocess functions" not "processor functions".

    Specifically speaking, this entire code addition is for allowing the "preprocess of theme hook suggestions".

    I only say this because it's initially confusing to read since in the history of the theme system there were separate process functions before.

    All comments in this code should really be updated to reflect the either the general or specific concept.

  6. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -287,4 +287,23 @@ function testRegionClass() {
    +  /**
    +   * Ensures suggestion preprocess functions run even for default
    +   * implementations.
    
    +++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php
    @@ -143,4 +143,12 @@ public function nonHtml() {
    +  /**
    +   * Menu callback for testing preprocess functions are being run for theme
    +   * suggestions.
    +   */
    

    Needs to be a single line comment.

Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen
Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
FileSize
10.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
3.26 KB

I realized the regexp still needed quite a bit of work. This one should work and be fairly fast. It now requires the considered processor to contain "__" unlike the older version. It would seem to me like that's needed since this is about preprocess functions for suggestions.

Also tried to address 1, 3 and 4 from @markcarver's review.

lauriii’s picture

Status: Needs work » Needs review
lauriii’s picture

Status: Needs work » Needs review
FileSize
12.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,022 pass(es), 4 fail(s), and 0 exception(s). View
2.22 KB
Fabianx’s picture

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -683,6 +690,28 @@
+    if (!$this->groupedFunctions) {
+      $functions = get_defined_functions();

This is not gonna work reliably, the theme registry can load code at any time.

You will need to diff already seen functions with new get_defined_functions() - as in my original patch on the other issue.

----

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -570,10 +570,7 @@
-    $user_func = array_combine($user_func, $user_func);
+    $grouped_functions = drupal_group_functions_by_prefix();

@@ -593,7 +590,11 @@
+      if (isset($grouped_functions[$first_prefix])) {
+        $processors += preg_grep("/^{$prefix}_preprocess_($hooks)(__)?/", $grouped_functions[$first_prefix]);
+      }

I am pretty sure tests are now failing, because it should be:

$processors['mymodule_preprocess_node__article'] = 'mymodule_preprocess_node_article'

in the end ...

That is why we had array_combine before.

Antti J. Salminen’s picture

Assigned: Unassigned » Antti J. Salminen
lauriii’s picture

Status: Needs work » Needs review
FileSize
12.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,049 pass(es). View
2.19 KB

Caching grouped functions might not be worth it so I removed that from the patch.

Antti J. Salminen’s picture

Assigned: Antti J. Salminen » Unassigned
FileSize
13.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,332 pass(es). View
2.49 KB

I believe I improved on the cached version here by using a different approach for saving the seen functions resulting in less memory used and better overall performance. I think it could be worth keeping as Iarge and complicated sites could still see something like 500-1000 ms (possibly a bit optimistic back of the envelope calculation) saved with it.

Antti J. Salminen’s picture

FileSize
13.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,422 pass(es). View
991 bytes

Just removed the wrapper function from theme.inc.

lauriii’s picture

Profiled that with the set we had on the other issue #2339447: Improve theme registry build performance by 85% and looks pretty impressive:

Time mean: 0.002711717247963 sec
Memory mean: 0.3634169960022MB
------
Time mean: 0.0020202178955078 sec
Memory mean: 0.0011368865966797MB
Fabianx’s picture

Yes, I agree. We need a comment however that this approach works, because functions can only ever be added and never vanish. (That is why this works so well.)

And yes, that was kinda my original implementation for $seen :).

We could implementation=wise directly rely on $this->seenFunctions() - no need to assign back and forth. Also makes the code a little simpler ...

Antti J. Salminen’s picture

FileSize
13.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,424 pass(es). View
1.25 KB

@Fabianx: you're right, the flipping around is not useful. I mistakenly thought it would be. I removed that and changed to just keeping the list with function name keys and boolean values like your original version. Added a comment as well.

Cottser’s picture

Issue summary: View changes

Reviewing this. I redid most of the issue summary to focus on what is actually happening now.

andypost’s picture

  1. +++ b/core/includes/theme.inc
    @@ -166,25 +166,6 @@ function drupal_find_theme_functions($cache, $prefixes) {
    -function drupal_group_functions_by_prefix() {
    

    This should be deprecated not removed

  2. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -287,4 +287,23 @@ function testRegionClass() {
    +    $this->config('system.theme')
    +      ->set('default', 'test_theme')
    

    getEditable() should there, how it passes is a question

lauriii’s picture

@andypost: We discussed about 1. with @alexpott and it can be removed. It's considered to be prioritized change since its follow-up for a major issue.

Cottser’s picture

Issue summary: View changes

Just noting the API change in the issue summary.

lauriii’s picture

FileSize
14.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,376 pass(es), 2 fail(s), and 12 exception(s). View
15.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,306 pass(es), 989 fail(s), and 383 exception(s). View
6.88 KB
Cottser’s picture

Some minor stuff, not a complete review at this point. Obviously getting the test fails fixed would be higher priority :)

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -138,6 +138,21 @@ class Registry implements DestructableInterface {
    +   * User functions grouped by the word before first underscore.
    
    @@ -588,6 +698,35 @@ public function destruct() {
    +   * Get all user functions grouped by word before first underscore.
    

    "before the first underscore"

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +572,106 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +    $prefixes = array_keys((array) $this->moduleHandler->getModuleList());
    +    foreach (array_reverse($theme->getBaseThemes()) as $base) {
    +      $prefixes[] = $base->getName();
    +    }
    

    Should we be adding 'template' to this list of prefixes? Seems like template_preprocess_suggestions__kitten should work (if it doesn't already).

  3. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -287,4 +287,32 @@ function testRegionClass() {
    +      $items = $this->CssSelect('.suggestion');
    

    Minor: cssSelect.

  4. +++ b/core/modules/system/tests/modules/theme_test/theme_test.module
    @@ -90,6 +96,20 @@ function theme_theme_test_function_template_override($variables) {
    +/**
    + * Implements hook_preprocess_HOOK().
    + */
    +function theme_test_preprocess_theme_test_preprocess_suggestions(&$variables) {
    +  $variables['foo'] = 'Theme hook implementor=theme_theme_test_preprocess_suggestions().';
    +}
    

    We should probably test modules using the specific hooks, not just themes.

lauriii’s picture

Status: Needs work » Needs review
FileSize
15.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,380 pass(es), 12 fail(s), and 4 exception(s). View
596 bytes

Just trying to fix the tests first

Antti J. Salminen’s picture

Not sure I understand what's the direction taken with the latest patches. Why was the check for the existence of the base hook removed?

lauriii’s picture

Status: Needs work » Needs review

I'm sorry I didn't remember to leave a comment what we are trying to solve here because we talked about this on the sprints at DrupalCon Los Angeles where we decided to support preprocess functions even though a template wouldn't be provided. Also the previous logic didn't work since the preprocess function got added to the base hook and that way it got triggered for all the children of the base hook. Also on the theme manager we used to replace the preprocess functions with the base hooks preprocess functions but we don't want to do that anymore because children's preprocess functions can be different now.

lauriii’s picture

FileSize
608 bytes
15.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,361 pass(es), 12 fail(s), and 14 exception(s). View
lauriii’s picture

Status: Needs work » Needs review
FileSize
16.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,366 pass(es), 12 fail(s), and 2 exception(s). View
1.6 KB
lauriii’s picture

Status: Needs work » Needs review
FileSize
16.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,345 pass(es), 941 fail(s), and 383 exception(s). View
679 bytes
hass’s picture

Issue summary: View changes
lauriii’s picture

Status: Needs work » Needs review
FileSize
16.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,389 pass(es). View
667 bytes

Lets see how many tests I'm able to break with this.

joelpittet’s picture

FileSize
2.64 KB
16.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,412 pass(es). View

From #300 I did the nit picks for you @lauriii but maybe you can respond to #300.2 and #300.4?

Nice work to get it all green!

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +571,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   * This completes the theme registry adding discovered functions and hooks.
    
    @@ -588,6 +702,34 @@ public function destruct() {
    +   * Get all user functions grouped by the word before the first underscore.
    

    These should start with a verb with an 's' like "Completes the theme..." and "Gets all user functions..."

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -588,6 +702,34 @@ public function destruct() {
    +  public function getPrefixGroupedUserFunctions() {
    

    Can we get a follow-up to add an interface for this class? Especially considering it is a service.

  3. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -287,4 +287,33 @@ function testRegionClass() {
    +   * Ensures suggestion preprocess functions run even for default
    +   * implementations.
    

    I would just remove "even", this has to fit on one line.

  4. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -287,4 +287,33 @@ function testRegionClass() {
    +  function testSuggestionPreprocessForDefaults() {
    
    +++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php
    @@ -143,4 +143,22 @@ public function nonHtml() {
    +  function preprocessSuggestions() {
    

    public function

  5. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -287,4 +287,33 @@ function testRegionClass() {
    +    $this->config('system.theme')
    +      ->set('default', 'test_theme')
    +      ->save();
    

    Any reason for not using the API? \Drupal::service('theme_handler')->setDefault('test_theme');

tuutti’s picture

FileSize
16.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,490 pass(es). View
3.71 KB

Added some more test coverage and fixed things @tim.plunkett mentioned.

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +571,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
               }
    -          // Ensure uniqueness.
    -          $cache[$hook]['preprocess functions'] = array_unique($cache[$hook]['preprocess functions']);
             }
    

    Are we sure this is still needed?

    Oh, I see, it is part of postProcessExtension now ...

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -554,14 +571,111 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +    }
    +    $prefixes[] = $theme->getName();
    +
    

    Yes, I think we should add 'template' as prefix.

Overall looks great, 'template' could be follow-up, too - if needed.

Antti J. Salminen’s picture

I believe the suggestion preprocessors must be added to the theme hook preprocessor lists in order. That is: "__kitten" must be processed before "__kitten__flamingo" independent of the prefix order. Otherwise "__kitten__flamingo" cannot pick up the "__kitten" preprocess function.

This is not currently the case. Order depends on what get_defined_functions() happened to return. Another thing is that this currently introduces base hook chaining which has not been allowed as a rule so far.

If I'm right about my observations, maybe just going with dvessel's version where just the base hook preprocessors were inherited would be better to keep things simple?

Fabianx’s picture

#317: Yes, that was my concern, too, but I thought we only allow one level anyway ...

And for all things being able to do:

hook_preprocess_foo__bar() is much better than not being able to do so ...

If we need to get crazy and support hook_preprocess_foo__bar__baz(), then we would indeed need to order by length of key to do so reliably.

I would not be opposed to base hook chaining as long as we end up with a deterministic order.

lauriii’s picture

+++ b/core/modules/system/tests/modules/theme_test/theme_test.module
@@ -110,6 +110,14 @@
+
+

Unnecessary double line change

Antti J. Salminen’s picture

FileSize
16.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,518 pass(es). View
5.55 KB

I think this should be enough to ensure all the preprocess functions are added in the right order. Removed a never-reached section from the hook processing and tried to choose more descriptive variables.

Also improved on some of the comments for #273.5 and removed the extra newline but at least #273.2 still remains.

Still probably needs work, I suspect things like suggestion hooks might not get called properly.

Cottser’s picture

Does this already handle base theme preprocessors coming before subtheme preprocessors? Either way, that probably deserves test coverage I'd say.

Fabianx’s picture

Looks pretty good to me :)

lauriii’s picture

Status: Needs review » Needs work

So we talked shortly about this on yesterdays Twig call. What people on the call agreed was that we should include the template preprocess functions here because it would be bad DX to accept using specific theme suggestions for theme preprocess but not template preprocess.

Adding template functions in the post process would add unnecessary complexity if we would test where the function is located. Currently we accept template preprocess functions only from modules. We thought we could remove that limitation because of the docs are saying "Should be implemented by the module that registers the theme hook, to set up default variables." so we already suggest people to not use it anywhere else instead of forcing (according to docs). Anyway there is already working work arounds to get that limitation out.

Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
17.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,623 pass(es). View
3.23 KB

Needs work, only marking needs review to get a test run.

Changes:

Doesn't chain the base hook definitions. The alternative is to change ThemeManager's logic for including the base hook files and calling suggestion hooks but I'm not sure about what the gain from the effort would be.

Just uses the suggestion hook's preprocess functions instead of merging with base. That should now work after a fix in this to postProcess that caused the suggestion hooks to get the preprocessors in the wrong order after they were merged in from the base hook.

Checks for the base hook again... Sorry, but I don't see a problem with this and it just seems more robust to me. If there's an argument against it I'll be happy to hear it though.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: -Increases learning curve, -FX (Front End Experience), -sprint, -focus +Needs tests

We need test coverage so that the change on #324 doesn't break anything. It is also important part of the way theme system works so its good to have later on too.

Also did a small tag clean up. Don't think we need all these.

Fabianx’s picture

#323: Could you elaborate some of what you mean there?

I don't actually get what the change will be ... Thanks!

Antti J. Salminen’s picture

FileSize
20.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,840 pass(es), 2 fail(s), and 0 exception(s). View
3.93 KB

Added a few tests for a current problem in the patch: an implementation of a suggestion without it's own preprocess functions won't inherit the preprocess functions from less specific suggestion levels.

Antti J. Salminen’s picture

Status: Needs work » Needs review
lauriii’s picture

#326: So the idea is that instead of removing template functions out of the scope of this issue we agreed on covering the template functions here to not cause any regression for the DX. The idea what we thought it would work was this:

instead of overriding the previous template functions:

<del>template_preprocess_node</del>
template_preprocess_node__article
hook_preprocess_node
hook_preprocess_node__article

or simply not adding template functions

template_preprocess_node
hook_preprocess_node
hook_preprocess_node__article

we want to call template function for each theme suggestion like this:

template_preprocess_node
template_preprocess_node__article
hook_preprocess_node
hook_preprocess_node__article
lauriii’s picture

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -556,14 +573,117 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
+            if (isset($cache[$previous_hook])) {
+              $cache[$hook] = $cache[$previous_hook];
+              // If a base hook isn't set, this is the actual base hook.
+              if (!isset($cache[$previous_hook]['base hook'])) {
+                $cache[$hook]['base hook'] = $previous_hook;
+              }
+              $cache[$hook]['preprocess functions'][] = $preprocessor;

So when we'll do this we need to check if there is available hooks on lower levels so they can be added for the same stack.

davidhernandez’s picture

Why do we need to support template_preprocess_node__article?

lauriii’s picture

Issue tags: -Needs tests

I think the biggest reason for that is the learning curve. Why would it make sense to have them for hook_preprocess_hook if not for template_preprocess_hook?

davidhernandez’s picture

Going back to the comment in #323, what I remember is we agreed that it was ok to leave the ability to use template_preprocess_* from a theme. Not because it was desired functionality, but because preventing it would be too complex as it isn't apparent where the suggestion came from. We would just state, for the record, that it was a bad idea to do.

Please correct me if I'm wrong, but my understanding is that the point of template_preprocess_* functions is that they run first and guarantee a way that the preprocessing done by the module that declares the theme implementation will always run first. All of other modules and themes should be using hook_preprocess. If so, the module that should use template_preprocess_node is the node module, and the node module would have no reason to use extended suggestions. If it did, they probably would have been declared separately.

I don't mean to say the additional suggestions be stopped, if they are a natural result of fixing the hook_* suggestions. It just didn't strike me as being worth additional complexity to add, if not already there.

Fabianx’s picture

#334: Yes, however I as a module author could have a generic theme => 'map' type and then have 'map__red' be a subtype, where I declare both template_preprocess_map and template_preprocess_map__red ...

markcarver’s picture

Yes, template_preprocess_THEMEHOOK__SUGGESTION[__SUGGESTIONS] is needed (both modules and themes need this).

For a real world use case scenario, the Icon API module could greatly utilize this:

Have to manually define "different" theme hooks just to differentiate between icon types:
http://cgit.drupalcode.org/icon/tree/includes/theme.inc#n27

Invokes an additional theme call for the specific icon bundle's renderer theme hook:
http://cgit.drupalcode.org/icon/tree/includes/theme.inc#n91

It's basically the difference of having to manually define additional theme hooks and hack in special "variation" code to deal with "inheritance"; opposed to using proper theme hook suggestions in the first place.

template_preprocess_icon()
template_preprocess_icon_sprite() <- different theme hook "icon_sprite", essentially duplicates code required to "inherit" from the above "parent" theme hook.

vs.

template_preprocess_icon() <- preprocesses all icons.
template_preprocess_icon__sprite() <- would be invoked after the above to preprocess "sprite" specific icons.

edit:

This allows the module that created theme hook (Icon API in this case) to run it's necessary preprocessing first before any module or theme decides to do it's own preprocessing to add/remove what was initially provided:

MYMODULE_preprocess_icon()
MYMODULE_preprocess_icon__sprite()

MYTHEME_preprocess_icon()
MYTHEME_preprocess_icon__sprite()

This is how basic preprocessing currently works (module that created theme hook uses template_preprocess_* and then modules/themes use hook_preprocess_*). This issue is about expanding that existing pattern to preprocess theme hook suggestions as well.

davidhernandez’s picture

Ok, that answers my question.

lauriii’s picture

I was investigating this and was able to track down the problem. The problem is that some preprocess functions are created before postProcess and therefore they have wrong values (not supported by postProcess) in their array.

rrav’s picture

Until is fixed you can use this MYTHEME_preprocess_node__SUGGESTION by adding this in your template.php :

MYTHEME_preprocess_node (&$vars) {

foreach ($vars['theme_hook_suggestions'] as $suggestion) {
        $function = implode('__', array(__FUNCTION__, $suggestion));
        dpm($function);
        if (function_exists($function)) {
          $function($vars);
        }
    };

}
Antti J. Salminen’s picture

Status: Needs work » Needs review
FileSize
21.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,789 pass(es), 134 fail(s), and 6 exception(s). View
4.95 KB

We exchanged ideas with lauriii and here's some unfinished work on fixing the current main issue with the patch based on that. Just triggering a test bot run, still needs work.

lauriii’s picture

Status: Needs work » Needs review
FileSize
21.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,240 pass(es), 20 fail(s), and 0 exception(s). View
1.09 KB

This should fix some of the existing test fails. Lets see if this introduces new one.

lauriii’s picture

Status: Needs work » Needs review
FileSize
21.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,839 pass(es). View
1.27 KB

We need to inherit the preprocess functions from base hook if base hook has been defined. This should fix some of the remaining failures.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - I don't have further performance concerns.

Nice work on re-using the new algorithm.

Xano’s picture

Status: Reviewed & tested by the community » Needs work

Glad to see the code itself has been RTBC'ed! My feedback applies to the documentation only:

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -138,6 +138,20 @@ class Registry implements DestructableInterface {
    +   * @var array
    

    @var array is not specific enough. It looks like this should be string[].

  2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -138,6 +138,20 @@ class Registry implements DestructableInterface {
    +   * @var array
    

    @var array is not specific enough. It looks like this should be bool[], with an additional description to document that keys are function names, and to explain that this array format was chosen because key lookups are faster than value lookups.

  3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -438,6 +455,12 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +        // If theme hook has base hook mark its preprocess functions always
    +        // incomplete to inherit base hooks preprocess functions.
    

    Wait, wut? This is no coherent sentence.

  4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -556,14 +579,140 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   * @param array $cache
    +   *   The theme registry.
    

    @var array is not specific enough. This needs a more specific description of what is in the array.

  5. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -556,14 +579,140 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +   * @param array $cache
    +   *   The theme registry.
    

    Same here.

  6. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -556,14 +579,140 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +    $prefixes = array_keys((array) $this->moduleHandler->getModuleList());
    

    This method always returns an array. The type cast is not necessary.

  7. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -556,14 +579,140 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
    +    // Add missing variable preprocessors. This is needed for hooks that do not
    +    // explicitly register the hook. For example, when a theme contains a
    

    Hooks registering hooks? Should maybe be modules registering hooks?

  8. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -590,6 +739,34 @@ public function destruct() {
    +   * @return array
    

    @return array is not specific enough.

cilefen’s picture

Status: Needs work » Needs review
FileSize
3.09 KB
21.92 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,827 pass(es), 1 fail(s), and 0 exception(s). View

This is 1, 2, 3, 6 and 7. I don't know how to document $cache.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

This are minor doc nits that don't need to block this as we reference the building up theme registry already since ages just as array $cache, which we build over time.

It is just an array of hooks.

If we had good documentation elsewhere on that I would agree, but we don't, so we should open a follow-up to clean up docs during RC phase. (which is still unfrozen then).

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review

I take that back, I found the docs!

"$cache: The theme registry that will eventually be cached; It is an associative array keyed by theme hooks, whose values are associative arrays describing the hook:"

So should probably be:

  array[] $cache
    The theme registry that will eventually be cached; It is an associative array keyed by theme hooks, whose values are associative arrays describing the hook:

as in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Theme%21R...

Fabianx’s picture

Issue summary: View changes

Added a proposed commit message. I would propose to give everyone credit that has more than just 1 comment as this is a really really old issue.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
22.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,840 pass(es). View

It is clear now why we need the typecast. How about this for documenting the parameter? It is so well documented in processExtension().

Xano’s picture

FileSize
1.2 KB
23.05 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,019 pass(es). View

It is clear now why we need the typecast.

We did not need the type cast. We usually don't, and when we do, we must document why.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fixes, back to RTBC!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. I would have expected something to change in theme.api.php to document that the functions listed in the IS will be discovered.
  2. +++ b/core/includes/theme.inc
    @@ -162,25 +162,6 @@ function drupal_find_theme_functions($cache, $prefixes) {
     /**
    - * Group all user functions by word before first underscore.
    - *
    - * @return array
    - *   Functions grouped by the first prefix.
    - */
    -function drupal_group_functions_by_prefix() {
    -  $functions = get_defined_functions();
    -
    -  $grouped_functions = [];
    -  // Splitting user defined functions into groups by the first prefix.
    -  foreach ($functions['user'] as $function) {
    -    list($first_prefix,) = explode('_', $function, 2);
    -    $grouped_functions[$first_prefix][] = $function;
    -  }
    -
    -  return $grouped_functions;
    -}
    
    +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -590,6 +743,34 @@ public function destruct() {
       /**
    +   * Gets all user functions grouped by the word before the first underscore.
    +   *
    +   * @return array
    +   *   Functions grouped by the first prefix.
    +   */
    +  public function getPrefixGroupedUserFunctions() {
    +    $functions = get_defined_functions();
    +
    +    // Splitting user defined functions into groups by the first prefix.
    +    foreach ($functions['user'] as $function) {
    +      if (isset($this->seenFunctions[$function])) {
    +        continue;
    +      }
    +      list($first_prefix,) = explode('_', $function, 2);
    +      $this->groupedFunctions[$first_prefix][] = $function;
    +    }
    +
    +    // The theme registry may load new code. On encountering newly defined
    +    // functions, we save the list of defined functions again. This works
    +    // because functions cannot disappear between calls.
    +    if (isset($first_prefix)) {
    +      $this->seenFunctions = array_fill_keys($functions['user'], TRUE);
    +    }
    +
    +    return $this->groupedFunctions;
    +  }
    

    I get moving the method to the object so we can unit test. But also changing the logic to store seenFunction and groupedFunctions is not necessary. There's been no discussion of the memory usage vs performance gains of this change.

Fabianx’s picture

+++ b/core/includes/theme.inc
@@ -115,7 +115,7 @@ function drupal_theme_rebuild() {
-  $grouped_functions = drupal_group_functions_by_prefix();
+  $grouped_functions = \Drupal::service('theme.registry')->getPrefixGroupedUserFunctions();

@@ -162,25 +162,6 @@ function drupal_find_theme_functions($cache, $prefixes) {
-function drupal_group_functions_by_prefix() {
-  $functions = get_defined_functions();

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -138,6 +138,22 @@ class Registry implements DestructableInterface {
+  protected $groupedFunctions = [];
...
+  protected $seenFunctions = [];

@@ -590,6 +743,34 @@ public function destruct() {
+  public function getPrefixGroupedUserFunctions() {
+    $functions = get_defined_functions();

Discussed with alexpott, lets make the statefulness a follow-up and remove the seen* and groupedFunctions* state from the class.

It is just one more call to get_defined_functions(), which should be fine for now.

lauriii’s picture

Status: Needs work » Needs review
FileSize
23.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,671 pass(es). View
2.94 KB

Added some documentation to theme.api.php and reverted the logic change

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Thanks and back to RTBC.

catch’s picture

Do we still need the first three entries in system_theme() once this is in? https://api.drupal.org/api/drupal/core!modules!system!system.module/func...

Pages