Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#31 | before_patch_front_page.png | 166.9 KB | dvessel |
#31 | after_patch_front_page.png | 167.45 KB | dvessel |
#30 | theme_suggestions_956520_30.patch | 18.62 KB | dvessel |
#16 | 956520-theme_hook_suggestions-16.patch | 2.58 KB | effulgentsia |
#10 | ths.zip | 1.74 KB | Jacine |
Comments
Comment #1
sunThanks for reminding of this problem. I hope that effulgentsia can shed some light on the details.
Comment #2
sunAssigning to myself for now, just to ensure that this is tackled. Feel free to re-assign, if you want to work on it.
Comment #3
himerus CreditAttribution: himerus commentedsubscribe... just am running across this
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedI will. Soon.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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.
Comment #6
sunYes, 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.
Comment #7
mattyoung CreditAttribution: mattyoung commented~
Comment #8
mlncn CreditAttribution: mlncn commentedMarking 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.
Comment #9
chx CreditAttribution: chx commentedLet there be patch!
Comment #10
JacineThanks :)
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.
Comment #11
JacineAlso, 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.
Comment #12
JacineErr, ignore the attachment in #10. I must be loosing my mind. :P
Comment #13
JacineHere 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
Comment #14
Jacinekrumo() & 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().
Comment #15
chx CreditAttribution: chx commenteddebug() is probably just too late from templates. It sets a drupal message.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedNot 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:
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.
Comment #17
webchickThis 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.
Comment #18
Jacine@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.
Comment #19
webchickI 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.
Comment #20
JacineI 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.
Comment #21
JacineImproving title and changing status since benchmarks and tests are needed for this.
Comment #22
mortendk CreditAttribution: mortendk commentedprety 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)
Comment #23
ogi CreditAttribution: ogi commentedsubscribe
Comment #24
tinefin CreditAttribution: tinefin commentedsubscribe
Comment #25
Jeff Burnz CreditAttribution: Jeff Burnz commentedLove 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.
Comment #26
Alexander Allen CreditAttribution: Alexander Allen commentedSubscribing.
Comment #27
dvessel CreditAttribution: dvessel commentedI'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.
Comment #28
dvessel CreditAttribution: dvessel commentedOn 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.
Comment #29
dvessel CreditAttribution: dvessel commentedGoing to try tacking this.
Comment #30
dvessel CreditAttribution: dvessel commentedI'm relearning how to apply a core patch so I hope this is correct.
The patch does the following:
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.
Comment #31
dvessel CreditAttribution: dvessel commentedHere'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.
I also attached screen shots of webgrind.
Comment #32
dvessel CreditAttribution: dvessel commentedI haven't been explaining myself clearly and how it relates to the issue at hand.
From the original post by Jacine:
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:
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.
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.
Comment #33
JacineThe 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.
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. Becausecontextual_element_info()
sets'#theme' => 'links__contextual'
, I can implementtheme_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 insidehook_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. :(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 iflinks__contextual
exists in that array, which gives me context instead of having to duplicate the entire theme function just for a small change.Comment #34
dvessel CreditAttribution: dvessel commentedAh, 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.
Comment #35
JacineLOL. 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.
Comment #36
dvessel CreditAttribution: dvessel commentedConcerning 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.
Comment #37
dvessel CreditAttribution: dvessel commentedI moved the patch here: http://drupal.org/node/939462#comment-4491226
Comment #38
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #39
Bojhan.core CreditAttribution: Bojhan.core commentedhttp://drupal.org/node/956520#comment-3860490 seems to be the current state of affairs.
Comment #40
tim.plunkettComment #41
jherencia CreditAttribution: jherencia commentedComment #42
jenlamptontagging for learnability
Comment #43
Fabianx CreditAttribution: Fabianx commentedMarked as duplicate of #939462-109: Specific preprocess functions for theme hook suggestions are not invoked.
Comment #44
Fabianx CreditAttribution: Fabianx commentedtagging
Comment #45
Fabianx CreditAttribution: Fabianx commentedOpened 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.
Comment #46
sunThis 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.
Comment #47
Fabianx CreditAttribution: Fabianx commentedOkay, 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.
Comment #48
steveoliver CreditAttribution: steveoliver commentedAny progress, Fabian? Just trying to get theme work going again.
Comment #49
Fabianx CreditAttribution: Fabianx commented#16 worked fine locally for me, but probably needs some more thought of what this will output.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedtag update.
Comment #51
jenlamptonPostponing for now. I think this is going to end up being a duplicate of #2004872: [meta] Theme system architecture changes.
Comment #52
jenlamptonI think this is officially now a dupe of #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Comment #52.0
jenlamptonUpdated to reduce scope to #16