Problem

  • Even though some module uses theme suggestions in the form of foo__bar__baz, a module cannot simply implement HOOK_foo__bar__baz().
  • Suggestions that are not applied via preprocess functions do not appear in $variables['theme_hook_suggestions'] so themes have no way of telling what suggestions are available.

Goal

  • Make those theme suggestion patterns work as is.
  • Pass the available suggestions $variables['theme_hook_suggestions'] so themes can actually become aware of and use them.

Details

  • It currently does not work, because, although the calling + implementing module specifies theme('foo__bar__baz'), it only registers 'foo' in its own hook_theme().
  • Therefore, 'foo__bar__baz' is unknown to the theme registry, and even if it exists, it is not called.
Files: 
CommentFileSizeAuthor
#31 before_patch_front_page.png166.9 KBdvessel
#31 after_patch_front_page.png167.45 KBdvessel
#30 theme_suggestions_956520_30.patch18.62 KBdvessel
PASSED: [[SimpleTest]]: [MySQL] 31,844 pass(es). View
#16 956520-theme_hook_suggestions-16.patch2.58 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 27,376 pass(es). View
#10 ths.zip1.74 KBJacine
#14 test_theme.suggestions.patch1.25 KBJacine
PASSED: [[SimpleTest]]: [MySQL] 27,413 pass(es). View
#13 test_theme.suggestions.patch1.25 KBJacine
PASSED: [[SimpleTest]]: [MySQL] 27,449 pass(es). View
#12 ths.zip1.54 KBJacine
#9 956520.patch966 byteschx
PASSED: [[SimpleTest]]: [MySQL] 27,367 pass(es). View

Comments

sun’s picture

Thanks for reminding of this problem. I hope that effulgentsia can shed some light on the details.

sun’s picture

Assigned: Unassigned » sun

Assigning to myself for now, just to ensure that this is tackled. Feel free to re-assign, if you want to work on it.

himerus’s picture

subscribe... just am running across this

effulgentsia’s picture

I hope that effulgentsia can shed some light on the details.

I will. Soon.

David_Rothstein’s picture

I ran into this today (or at least some version of it). Is this two issues in one, or are they the same issue?

The followups at #241570: Theme preprocess functions do not get retained when using patterns seem to be discussing the issue that when theme('foo__bar') is called, MYMODULE_preprocess_foo__bar() isn't.

But I think Jacine's second point above (and the issue I just encountered) is that even if you implement MODULE_preprocess_foo() itself, you cannot know if it was theme('foo'), theme('foo__bar'), or theme('foo__something_else') that was called; you don't have that context in $variables['theme_hook_suggestions'] or anywhere else.

I talked this over with @effulgentsia a bit and he pointed out that in many cases you can get the context from some other variable that was passed in. However, we then realized that's not always the case. An example would be this function:
http://api.drupal.org/api/drupal/modules--node--node.module/function/nod...

This results in a call to theme('item_list__node') but there is nothing it provides that would (cleanly) allow preprocess functions to distinguish it from all other calls to theme('item_list').

The root of the problem seems to occur early in theme(), with this code:

    while ($pos = strrpos($hook, '__')) {
      $hook = substr($hook, 0, $pos);
      if (isset($hooks[$hook])) {
        break;
      }
    }

Which basically destroys all memory of what the passed-in $hook originally was, as it strips parts of it off. If we retained some record of the original suggestions in some variable that would eventually get passed to the preprocess and process functions as part of the $variables array, that might be a quick and dirty solution for at least part of this issue.

sun’s picture

Yes, two in one. That other issue was unknown at the time of creation.

We can provide that original hook name info as theme variable, using the 'theme_' namespace, since that namespace is owned by the theme system. $variables['theme_hook'] would make sense, I guess.

mattyoung’s picture

~

mlncn’s picture

Priority: Normal » Critical

Marking this as critical for before-release-candidate attention. There are situations where this cannot be worked around. Maybe fixing it is not considered an API change? But still better pre-RC.

chx’s picture

Status: Active » Needs review
FileSize
966 bytes
PASSED: [[SimpleTest]]: [MySQL] 27,367 pass(es). View

Let there be patch!

Jacine’s picture

FileSize
1.74 KB

Thanks :)

It needs to be in $variables['theme_hook_suggestions']. I'm also still not getting what's expected. If you try this with the example theme, which overrides theme_preprocess_exposed_filters(), you should get:

On /admin/content:
$variables['theme_hook_suggestions'] = array( 'exposed_filters__node');

On /admin/people:
$variables['theme_hook_suggestions'] = array('exposed_filters__user');

EDIT: Corrected the expected results.

Jacine’s picture

Status: Needs review » Needs work

Also, just to clarify what's the problem is, in case it's not clear:

Themers need a way to find out what alternate theme functions/template names exist. The information needs to be presented in a consistent way. Right now that way is printing dpm($vars['theme_hook_suggestions']);.

Right now, outside of suggestions set in preprocess functions, we get nothing. Unless you really get what's going on behind the scenes you'll have no idea if suggestions exist, let alone what they are.

Jacine’s picture

FileSize
1.54 KB

Err, ignore the attachment in #10. I must be loosing my mind. :P

Jacine’s picture

FileSize
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 27,449 pass(es). View

Here are two examples, added to test_theme/template.php.

The first one works: test_theme_preprocess_node().
The second does not and is what we are trying to fix: test_theme_preprocess_exposed_filters().

I used krumo because debug() didn't appear to do anything. :P

Jacine’s picture

FileSize
1.25 KB
PASSED: [[SimpleTest]]: [MySQL] 27,413 pass(es). View

krumo() & print_r() work, but debug() does nothing as far as I can see. Maybe I'm just using it wrong.

Here's one with print_r().

chx’s picture

debug() is probably just too late from templates. It sets a drupal message.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2.58 KB
PASSED: [[SimpleTest]]: [MySQL] 27,376 pass(es). View

Not like any of us want to add more complexity to theme(), but this *is* a problem as others on this issue have eloquently explained. We'll want to revisit the render and theme layers in D8 to see if we can simplify things, because the implementation of building and using the registry really is getting out of hand.

As David explains in #5, my goal in #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance and other issues has been to make the following as similar as possible with respect to everything that happens later in the pipeline:

theme(array('links__node__comment', 'links__node', 'links'), ...);
theme('links__node__comment', ...);
theme('links', ...);
...
function template_preprocess_links() {
  if (...) {
    $variables['theme_hook_suggestions'][] = 'links__node';
  }
  if (...) {
    $variables['theme_hook_suggestions'][] = 'links__node__comment';
  }
}

Basically, for various legacy reasons, we have these 3 techniques for doing essentially the same thing. And each one is employed somewhere (see template_preprocess_field() for an example of the last one, and the way drupal_prepare_form() sets $form['#theme'] for an example of the first one). While there are subtle reasons for why one technique is employed in one place, and another one is employed in another place, I think it would be best for them to work identically with respect to themeing.

This issue discovered one area in which they needlessly differ, and that is that only in the last case do hook_preprocess_*() implementations have access to a $variables['theme_hook_suggestions'] variable containing all of the suggestions requested up until that point. This patch fixes that. It needs a test added though. Any volunteers?

At first I was surprised by this. I thought of $variables['theme_hook_suggestions'] as a 1-way variable for preprocess functions to add to, and theme() to use, not for later preprocess functions to branch off of. After all, in the case of something like 'node__article', a later preprocess function doesn't need to check $variables['theme_hook_suggestions']. It can check $variables['node']->type. But as David points out in #5, via some short sightedness on our part, we don't always invoke theme functions with all necessary context information that might be useful. In the case of 'item_list__node', $variables['theme_hook_suggestions'] is the only variable available containing this useful context information, so I think this patch is the correct approach, despite the added implementation ugliness.

The latest work on #241570: Theme preprocess functions do not get retained when using patterns is a separate matter, and something I'm resistant to, but haven't taken the time to articulate why. I'll do so in that issue though. It shouldn't hold up this one.

webchick’s picture

Priority: Critical » Major

This doesn't look critical to me. I don't understand why this couldn't be fixed after RC1, or even in 7.2 for that matter.

Also, if we're screwing with theme() we need benchmarks.

Jacine’s picture

@effulgentsia, Thank you. This does the job. :)

Without this patch, theme hook suggestions, which are supposed to be useful and helpful for themers, are totally hidden from them under many circumstances. While I agree it's not critical, it makes me really sad to hear that (a) we need benchmarks for this and (b) this is not considered important enough to even get in 7.0, especially when we have a working patch. Right now, I see absolutely no way of getting at this information, short of grepping through core an looking for #theme, and well that is just a shame.

TBH, I'd love to see how devel_themer is going to deal with this, if the patch doesn't get in, at least so I can attempt to find another reliable way to get at this information and be able to explain it to others.

webchick’s picture

I didn't say it won't get fixed in D7? It's not like we stop fixing bugs once Drupal 7.0 ships, and there's nothing in here at all that breaks APIs.

But this isn't done until benchmarks and tests are done.

Jacine’s picture

I hear you, I just think it's important for people upgrading and writing themes to be able to access this information. It may not be a core API change, but having vs. not having this information can mean pretty big changes for a 1.0 to 1.1 theme release.

If someone is willing to guide me a little with the tests, I'd be happy to help.

Jacine’s picture

Title: Theme hook suggestions applied via preprocess are not contained in the theme registry. » Theme hook suggestions are only available when defined in preprocess functions
Assigned: sun » Unassigned
Status: Needs review » Needs work

Improving title and changing status since benchmarks and tests are needed for this.

mortendk’s picture

prety pretty pretty please we really need this one in core.

It an old problem for themers that we dont have a clue of whats actually given to us. So kinda walk around in the darkness, until by accident someone finds the right template or theme function (which can take days in a larger drupal site)

ogi’s picture

subscribe

tinefin’s picture

subscribe

Jeff Burnz’s picture

Love to see this go in, its a pretty big wtf and pita and a bunch of other unmentionable acronyms... sorry I cannot help with tests, wouldn't know where to start I'm afraid.

Alexander Allen’s picture

Subscribing.

dvessel’s picture

I'm quite confused on why the passed $hook is needed inside 'theme_hook_suggestions'. theme() finds the closest registered hook available. I don't see the point in carrying all the misses into the suggestions since they don't exist to begin with.

"theme_hook_suggestions" should be one way. Variable processors should only add to them and let theme() handle the rest.

And some of the code within theme() is just too much. A lot can be handled while building the theme registry. This could include the HOOK_foo__bar__baz() from a module (as Jacine originally mentioned) even if nothing explicitly defines the patterned hook.

Here's how it could possible happen.

Collect all functions with get_defined_functions() and filter it down checking based on existing hooks that hold a 'base hook'. drupal_find_theme_functions() does this but it's specific to theme functions.

If the patterned hook is not explicitly defined, take all the info from the base hook and carry it onto this new patterned hook and register it.

There would still be something very iffy about 'theme_hook_suggestions'. Defining it from a variable processor is a little late. If one of the suggestions catch on, it will not have its own specific var process functions invoked because it's in the middle of var process to begin with.

dvessel’s picture

On a side note, In drupal 8 we should to get rid of the idea of setting 'theme_hook_suggestions' from a variable process function. Variable process functions should only be for processing variables. Suggesting alternate hooks should be its own thing due to what I mentioned above and it simply doesn't make sense when we talk about a function for processing variables.

dvessel’s picture

Assigned: Unassigned » dvessel

Going to try tacking this.

dvessel’s picture

Status: Needs work » Needs review
FileSize
18.62 KB
PASSED: [[SimpleTest]]: [MySQL] 31,844 pass(es). View

I'm relearning how to apply a core patch so I hope this is correct.

The patch does the following:

  • Hook suggestions have a full set of variable processors. It no longer does the weird switching of hooks in the middle of a theme() call just to have the base hook processors from running. Since each hook suggestion has a full set, that means 'name_preprocess_hook' *and* 'name_preprocess_hook__suggestion' will be invoked.
  • If a module or a theme supplies a variable processor named as a pattern and that pattern was never explicitly registered, it will automatically register a hook based on the pattern and inherit all the attributes of the base hook. For example, a module or theme supplying 'foo_preprocess_bar__suggest' for the variable process function will register 'bar__suggest' as a new hook inheriting everything from the 'bar' hook. Of course, if 'bar' doesn't exist then it will do nothing.
  • A few performance optimizations for theme(). It's more straight forward removing some blocks of code and handling it in the registry build.

Overall, the performance is about the same. Using Apache bench, it showed no improvement which I find confusing because when profiling with xdebug and webgrind, theme() is consistently faster by a small margin.

dvessel’s picture

Here's a diff, --before and ++after. Done with xdebug disabled. I'm no pro at performing benchmarks but here are the results.

1000 calls, 5 concurrent.
Page caching off
Block caching on
css/js aggregation on
Opcode cache cleared before each test then primed with 50 calls before the full 1000 calls on the front page.

--- /Users/joon/Desktop/before_patch.txt	2011-05-17 01:37:15.000000000 -0400
+++ /Users/joon/Desktop/after_patch.txt	2011-05-17 01:37:20.000000000 -0400
@@ -21,35 +21,35 @@
 Server Port:            80
 
 Document Path:          /drupal_7/
-Document Length:        36327 bytes
+Document Length:        36331 bytes
 
 Concurrency Level:      5
-Time taken for tests:   41.704958 seconds
+Time taken for tests:   42.148533 seconds
 Complete requests:      1000
 Failed requests:        0
 Write errors:           0
-Total transferred:      36739703 bytes
-HTML transferred:       36363327 bytes
-Requests per second:    23.98 [#/sec] (mean)
-Time per request:       208.525 [ms] (mean)
-Time per request:       41.705 [ms] (mean, across all concurrent requests)
-Transfer rate:          860.28 [Kbytes/sec] received
+Total transferred:      36707000 bytes
+HTML transferred:       36331000 bytes
+Requests per second:    23.73 [#/sec] (mean)
+Time per request:       210.743 [ms] (mean)
+Time per request:       42.149 [ms] (mean, across all concurrent requests)
+Transfer rate:          850.47 [Kbytes/sec] received
 
 Connection Times (ms)
               min  mean[+/-sd] median   max
-Connect:        0    0   0.3      0       6
-Processing:    82  207  69.1    195     451
-Waiting:       74  188  66.8    174     444
-Total:         82  207  69.1    195     451
+Connect:        0    0   0.0      0       1
+Processing:    84  209  59.4    199     419
+Waiting:       74  192  57.9    183     413
+Total:         84  209  59.4    199     419
 
 Percentage of the requests served within a certain time (ms)
-  50%    195
-  66%    218
-  75%    234
-  80%    249
-  90%    314
-  95%    354
-  98%    392
-  99%    422
- 100%    451 (longest request)
+  50%    199
+  66%    220
+  75%    237
+  80%    248
+  90%    297
+  95%    344
+  98%    370
+  99%    381
+ 100%    419 (longest request)

I also attached screen shots of webgrind.

dvessel’s picture

I haven't been explaining myself clearly and how it relates to the issue at hand.

From the original post by Jacine:

Even though some module uses theme suggestions in the form of foo__bar__baz, a module cannot simply implement HOOK_foo__bar__baz().

My patch doesn't address that directly but it does allow a module to use HOOK_preprocess_foo__bar__baz() since it is focused on variable process functions.

Allowing the theme *function* of HOOK_foo__bar__baz() to work from a module doesn't fit conceptually for me since all modules are equal and they don't stack on top of each other like themes do and theme functions are the end of the line. There can only be one active theme function for a given hook.

Variable process functions on the other hand do have a pattern of stacking or layering on top of each other.

Example:


$output = theme('foo__bar__baz');

/**
 * foo theme function. This can also be implemented as a template. Doesn't
 * make a difference.
 */
function theme_foo() {

}

/**
 * foo pattern preprocessor based on foo.
 *
 * Since the module supplies this function, 'foo__bar__baz' must
 * be registered for it to be available and the patch does it
 * for you automatically if it doesn't exist already.
 *
 * This can also be done from a theme. Makes no difference.
 */
function module_name_preprocess_foo__bar__baz(&$vars, $hook) {

}

/**
 * foo hook preprocess function.
 */
function theme_name_preprocess_foo(&$vars, $hook) {
  // theme('foo__bar__baz') will result in $hook being 'foo__bar__baz'.
  // This is only because 'module_name_preprocess_foo__bar__baz()' was
  // provided by the module forcing the hook to be registered and
  // 'foo__bar__baz' inherits all the base hook variable processors.
  // This can also be your context.
  print var_dump($hook);
  // theme('foo') will result in $hook being 'foo' as expected.
}

Under the hood, when 'foo__bar__baz' is never registered but a variable process function based on that pattern is discovered, it finds the base hook and copies all the meta into the new hook.

If the 'foo' base hook is a template, we'll have a foo.tpl.php file but the same idea will apply. That preprocess function based on the pattern will create the new hook and use that same foo.tpl.php file. When foo--bar--baz.tpl.php is created, it's seen as an explicit override and use that instead.

Suggestions that are not applied via preprocess functions do not appear in $variables['theme_hook_suggestions'] so themes have no way of telling what suggestions are available.

I might have missed something but I'll repeat my understanding on this.

Let's say a theme call of theme('root__derivative') is invoked. The 'root' is a known hook but 'root_derivative' was never registered. If that's passed into $variables['theme_hook_suggestions'], what can be done with it? If it was never registered, it's dead from the start.

The only thing you would miss are the less specific hooks that might actually exist but I don't see the why anyone would want that.

Jacine’s picture

Let's say a theme call of theme('root__derivative') is invoked. The 'root' is a known hook but 'root_derivative' was never registered. If that's passed into $variables['theme_hook_suggestions'], what can be done with it? If it was never registered, it's dead from the start.

The problem here is that as a themer, I don't know that "derivative" exists as a possibility unless it was originally created in a preprocess function.

  1. Say I need to change something in theme_links(), but only for when it's being used for contextual links. In Drupal 7 that's entirely possible. Because contextual_element_info() sets '#theme' => 'links__contextual', I can implement theme_links__contextual().

    This is critical information and it's completely hidden from the themer. I want to be able to see it in $variables['theme_hook_suggestions'] when printed inside hook_preprocess_root. Right now, I can only see this information consistently when it has been set in a preprocess function. Any #theme set in a render array or for a form is lost. Tools like Devel themer don't show this information either. It goes into a black hole and dies. :(

  2. Also, let's say I just need to make a small tweak to theme_links() using the same example. If the $theme_hook_suggestions was created during preprocess, I can check to see if links__contextual exists in that array, which gives me context instead of having to duplicate the entire theme function just for a small change.

dvessel’s picture

Ah, yea. I must be loosing touch. You're looking at it in terms of understanding what your options are as a theme developer then acting on that information. I was more concerned about having a complete theme registry so it acts on the passed hooks more consistently. I kinda jacked your thread, didn't I. I'll open this in a new issue.

Jacine’s picture

LOL. I doubt you are loosing touch ;)

Not positive, but the answers/discussion you are looking for might be in this issue: #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance.

dvessel’s picture

Status: Needs review » Needs work

Concerning the approach taken before I got here.. It should not be passed to 'theme_hook_suggestions' since it wouldn't do anything useful. Since it's more about context for the themer, just pass it into $variables. Using theme_hook_suggestions would incur a tiny performance hit since theme() does all that checking with suggestions.

dvessel’s picture

Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7
Bojhan.core’s picture

http://drupal.org/node/956520#comment-3860490 seems to be the current state of affairs.

tim.plunkett’s picture

jherencia’s picture

jenlampton’s picture

tagging for learnability

Fabianx’s picture

Status: Needs work » Closed (duplicate)
Fabianx’s picture

Issue tags: +Twig

tagging

Fabianx’s picture

Issue tags: -Twig

Opened follow-up for the non-duplicated part of this issue:

#1836142: Provide suggestions to (pre)process functions

This issue has been side-tracked too much for it to be still useful.

sun’s picture

Title: Theme hook suggestions are only available when defined in preprocess functions » Available theme hook suggestions are only exposed when there are preprocess functions defined
Status: Closed (duplicate) » Active

This is actually a discrete bug from that. It is only about exposure, not invocation. Slightly adjusting title to clarify.

#16 holds a simple solution attempt. I didn't quite understand why #30 started to revamp the entire theme registry processing, and it's perfectly possible that these patches and discussions are indeed duplicating #939462: Specific preprocess functions for theme hook suggestions are not invoked.

Fabianx’s picture

Assigned: dvessel » Fabianx
Issue tags: +Performance, +Twig

Okay, I reduced the scope in the issue summary and marked freshly created #1836142: Provide suggestions to (pre)process functions as duplicate.

Working here then and start with #16.

steveoliver’s picture

Any progress, Fabian? Just trying to get theme work going again.

Fabianx’s picture

#16 worked fine locally for me, but probably needs some more thought of what this will output.

beejeebus’s picture

Issue tags: +Needs profiling

tag update.

jenlampton’s picture

Status: Active » Postponed

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

jenlampton’s picture

Status: Postponed » Closed (duplicate)
jenlampton’s picture

Issue summary: View changes

Updated to reduce scope to #16