Updated: Comment #177
Problem/Motivation
The preprocess phase is responsible for too many aspects of the theme system, including providing alternate theme hook suggestions. This makes things tangled and makes critical issues like #939462: Specific preprocess functions for theme hook suggestions are not invoked more difficult to solve than they should be. Ideal solution discussed at #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK() has been postponed to D9.
Proposed resolution
Move the creating/altering of theme hook suggestions to separate hooks, and handle the altering of theme hook suggestions before any preprocess functions are called.
Remaining tasks
Patch needs profiling.API change needs approval by a core maintainer.Patch needs profiling again.Docs in theme.api.php fleshed out with @code samples.Moved to #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()Test coverage for use cases when an array of hooks is passed to theme().Moved to #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
User interface changes
n/a
API changes
New hooks introduced:
hook_theme_suggestions_HOOK(array $variables)
hook_theme_suggestions_HOOK_alter(array &$suggestions, array $variables)
Removed the ability to change template suggestions in preprocess functions by altering $variables['theme_hook_suggestion']
and $variables['theme_hook_suggestions']
.
Code that altered $variables['theme_hook_suggestions'] or $variables['theme_hook_suggestion'] in preprocess needs to be moved to suggestion hooks:
From the module that is providing the theme hook (implementing hook_theme()
):
Suggestions code is moved from template_preprocess_HOOK() to returning an array in hook_theme_suggestions_HOOK().
Before:
/**
* Prepares variables for search results templates.
*/
function template_preprocess_search_results(&$variables) {
$variables['theme_hook_suggestions'][] = 'search_results__' . $variables['plugin_id'];
}
After:
/**
* Implements hook_theme_suggestions_HOOK().
*/
function search_theme_suggestions_search_results(array $variables) {
return array('search_results__' . $variables['plugin_id']);
}
Any other module or theme:
Suggestions code is moved from HOOK_preprocess_HOOK() to manipulating the suggestions array in hook_theme_suggestions_HOOK_alter().
Before:
/**
* Implements hook_preprocess_HOOK() for node templates.
*/
function MYTHEME_preprocess_node($variables) {
$variables['theme_hook_suggestions'][] = 'node__' . 'my_first_suggestion';
$variables['theme_hook_suggestions'][] = 'node__' . 'my_second_suggestion';
}
After:
/**
* Implements hook_theme_suggestions_HOOK_alter().
*/
function MYTHEME_theme_suggestions_node_alter(array &$suggestions, array $variables) {
$suggestions[] = 'node__' . 'my_first_suggestion';
$suggestions[] = 'node__' . 'my_second_suggestion';
}
Altering $variables['theme_hook_suggestion'] (singular) is now just a matter of adding the suggestion to the end of the suggestions array in hook_theme_suggestions_HOOK_alter(). If you need to ensure that your suggestion is at the end, implement hook_module_implements_alter().
Before:
/**
* Implements hook_preprocess_HOOK() for node templates.
*/
function MYTHEME_preprocess_node($variables) {
$variables['theme_hook_suggestion'] = 'node__' . 'my_suggestion';
}
After:
/**
* Implements hook_theme_suggestions_HOOK_alter().
*/
function MYTHEME_theme_suggestions_node_alter(array &$suggestions, array $variables) {
$suggestions[] = 'node__' . 'my_suggestion';
}
Related Issues
#939462: Specific preprocess functions for theme hook suggestions are not invoked
#2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK()
#2004872: [meta] Theme system architecture changes
Original report by @mikl
Currently, we have the chicken-and-egg situation, where what template preprocessors to run are determined by the theme hook suggestions, which are set in template preprocessors.
That means that it's currently not possible to:
- Suggest a new template for nodes – say node--thursday
- Have the theme layer run template_preprocess_node__thursday
This problem has also been discussed in #939462: Specific preprocess functions for theme hook suggestions are not invoked.
In this issue, we propose to solve this by making suggestions returned in a separate (and earlier) phase than preprocess.
Effulgentsia gave me this task on the DrupalCon München sprint, he will probably be able to elaborate further.
Comment | File | Size | Author |
---|---|---|---|
#146 | 1751194-146.patch | 51.04 KB | star-szr |
#146 | interdiff.txt | 1.32 KB | star-szr |
#143 | 1751194-143.patch | 50.49 KB | star-szr |
#143 | interdiff.txt | 3.17 KB | star-szr |
#141 | 1751194-141.patch | 50.67 KB | star-szr |
Comments
Comment #1
mikl CreditAttribution: mikl commentedHere's an updated version of the patch. We're now (ab)using variable_process_phases to invoke hook_template_suggestions_HOOK instead of a manual process.
Additionally, I have gone through all the core modules and moved their suggestions to implementation of the new hook.
The template suggestions introduced in theme.inc still need to be moved to system.module.
Comment #2
mikl CreditAttribution: mikl commentedRemoved the final `theme_hook_suggestion` stuff from theme.inc.
Comment #3
mikl CreditAttribution: mikl commentedThis should ostensibly work without breaking things :)
Comment #5
mikl CreditAttribution: mikl commented#2: 1751194-introduce-hook_template_suggestions_hook-2.patch queued for re-testing.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedreroll and minor fix.
Comment #9
mikl CreditAttribution: mikl commentedAnother reroll, trying to fix some of the failing tests.
Comment #11
mikl CreditAttribution: mikl commented@effulgentsia what did you change? the hook_template_suggestions_HOOK implementations no longer get the correct variables (ie. what would go into the templates) but rather a render array like this:
That obviously causes a lot of errors :/
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedThis, or an alternate solution to #939462: Specific preprocess functions for theme hook suggestions are not invoked will be needed for #1804614: [meta] Consolidate theme functions and properly use theme suggestions in core, so tagging accordingly.
Comment #13
barraponto CreditAttribution: barraponto commentedThis needs work, right? Patch is not "review" quality ;)
Comment #14
kscheirerYeah, this is colliding with some of the TWIG template code that's going in. Can someone figure out how to reroll this? There's a test in #939462: Specific preprocess functions for theme hook suggestions are not invoked we can use for this case too.
Comment #15
Fabianx CreditAttribution: Fabianx commentedTagging with Twig, so I can find this again
Comment #16
Fabianx CreditAttribution: Fabianx commentedFirst I was opposed to this, but now it makes much sense.
We need to have this information during the building of the registry to be performance efficient.
Maybe we need rather an _info hook, because I also need to add some information into the mix for render arrays and we can probably find more ...
Comment #17
catchWhy is this a separate task and not in #939462: Specific preprocess functions for theme hook suggestions are not invoked?
Comment #18
Fabianx CreditAttribution: Fabianx commentedYeah, lets please move the patch over to #939462: Specific preprocess functions for theme hook suggestions are not invoked. It is a bug fix and not a feature request.
Comment #19
star-szrI'd like to revive this issue as part of #2004872: [meta] Theme system architecture changes, so re-opening.
Comment #20
star-szrAnd re-titling to reflect the hook names proposed in #2004872: [meta] Theme system architecture changes.
Comment #21
star-szrWorking on this, should have a new patch up tomorrow.
Comment #22
star-szrNot quite ready for testbot yet but here is a rough work-in-progress patch.
This is based on #9 and reworked to match what has been proposed in #2004872: [meta] Theme system architecture changes. There are some theme_hook_suggestions in Views and Views UI that need to be converted still. I'm hoping to have a patch for testbot by the end of the weekend.
Comment #23
star-szrComment #24
star-szrPlenty of @todos left but let's see what happens.
Comment #26
star-szrFirst patch fixes the failing test - removed one line too many when moving things around.
Second patch no longer adds 'theme_hook_suggestions' to the $variables array (this felt wrong), instead it passes $suggestions to the ENGINE_render_template() function. This way we can still display the theme suggestions in Twig debug output.
Comment #27
star-szrThis fixes up the Views suggestions hook implementations and makes HOOK the hook implementation that is actually used. So if theme('comment__node_article', …) is called, hook_theme_suggestions_comment_alter() is invoked instead of hook_theme_suggestions_comment__node_article(). You can still check $variables['theme_hook_original'] to get e.g. 'comment__node_article'.
Comment #28
star-szrTagging for tests, and more work.
Comment #29
star-szrNow passing the derived hook to hook_theme_suggestions_alter(), casting alter hook params to arrays, and adding initial version of API docs.
I also created a Twig debug bugfix issue as a result of working on this patch: #2029225: Twig debug output does not show the base or derived hooks when a theme suggestion is called directly
Still working on tests so leaving at CNW for now.
Comment #30
jenlamptonWow, this is looking great. I wish I were better at writing tests, but let me know if there's anything else I can do help here :)
Also, changing to a critical bug since this will solve #939462: Specific preprocess functions for theme hook suggestions are not invoked
Marking that issue as a dupe of this one.
Comment #31
star-szrI'm back onto this over the next few days.
Comment #32
star-szrReroll first, small update needed in node_theme_suggestions_node_alter() to catch up with HEAD.
Comment #33
star-szrTests added to verify the ability of both modules and themes to alter suggestions for template files and theme functions.
Comment #35
star-szrI will do the routerification and controllerification of the test callbacks once #2030457: Warning: Missing argument 3 for template_preprocess() for theme functions overridden by templates lands.
Other than that, here are some needed documentation updates. This is ready for review.
I'm still trying to understand the implications of #939462: Specific preprocess functions for theme hook suggestions are not invoked, read through the whole issue this morning. As it is, this patch does not solve the problem(s) in that issue. I think it would make the problem much easier to solve, though.
Comment #36
star-szr#2030457: Warning: Missing argument 3 for template_preprocess() for theme functions overridden by templates is in so this needs a small update. That will happen tonight or tomorrow.
Comment #37
star-szrAh, and @tim.plunkett re-opened #939462: Specific preprocess functions for theme hook suggestions are not invoked which reminded me that it should probably still be postponed since the test case in #939462-96: Specific preprocess functions for theme hook suggestions are not invoked still fails with this patch. More code would be needed (maybe part of or after #1886448: Rewrite the theme registry into a proper service?) to solve the preprocess issue.
So reverting status based on the fact that this is not (yet) fixing critical bugs.
Comment #38
star-szrFirst a reroll, no changes.
Comment #39
star-szrConverted test page callbacks to controller methods and routes.
Comment #41
star-szr#39: 1751194-39.patch queued for re-testing.
Comment #42
benjifisherThe patch from #39 does not apply locally. I will reroll it.
Comment #43
benjifisherOK, it was pretty simple to reroll the patch. The only conflict was c094757 from #1963942: Change all instances of $vars to $variables. Update attached.
It seems that HEAD is broken, so I expect some tests to fail ...
I am reviewing the patch manually.
Comment #44
benjifisherGenerally, the changes look good. I like replacing the verbose usage
$variables['theme_hook_suggestions'][] = ...
with
$suggestions[] = ...
I do have some suggestions for improvement:
Why use array_merge()? AFAICT, this has the same effect as
The lines
in
template_preprocess_maintenance_page()
have been replaced byin
system_theme_suggestions_maintenance_page_alter()
. This seems to miss the case described in the comment:(from
_template_preprocess_default_variables()
). I think you could add the try/catch statements totemplate_preprocess_maintenance_page()
.The lines
in
block_theme_suggestions_block_alter()
were copied fromtemplate_preprocess_block()
, leading to a few lines of code duplication. I am afraid that, down the line, one of these will be modified and then the CSS id attribute will be out of sync with the template suggestion. Maybe refactor those few lines into a separate_block_*()
function?The lines
were removed from
testBlockThemeHookSuggestions()
. I agree that they do not belong there, but perhaps they should be added to another test class, or maybe we need a new test class.Comment #45
star-szrThanks very much @benjifisher!
I'm looking into all your points but also noticing that this needs another reroll so I'm going to start with that.
Comment #46
star-szrComment #47
star-szrThat's all I can manage for today but I will come back to this in a day or two unless someone else wants to grab it.
Comment #48
star-szrComment #49
star-szrPoints #1 and #2 from comment #44 have been fixed. For #3 I asked in #drupal-contribute and then opened #2043527: Theme name is included in block machine name but should be stored as a key instead so I consider that taken care of :)
For #4 (testing the block classes), I just did a copy and paste into a new test class, couldn't find anything similar.
Comment #50
Fabianx CreditAttribution: Fabianx commentedLooks very great to me already! (I spent hours on that already way back after DrupalCon and this solution is really nice. I probably wanted to do too much (tm).)
But I found some things that unfortunately need a little work:
This looks right, but it is not :-).
Consider calling
#theme => 'node__article'
which would call
hook_theme_suggestions_node__article_alter instead of hook_theme_suggestions_node_alter as expected
if node--article.html.twig exists.
I think however using the $info['base hook'] should work here.
Is it correct to _replace_ the $processor functions here instead of add to it?
This is nice.
However: Is this an API change?
FWIW, I would be fine with adding
$variables['theme_hook_suggestions'] = $suggestions;
at this point to keep some BC here.
array_merge seems slower here (1 additional function call) than adding the suggestions to the end.
As they are used within reverse priority order, just adding to it, should be fine.
Again, an unneeded function call.
Please keep this up, this is a great start!
Comment #51
markhalliwellEven though this technically is an API change, it's a necessary one if we're going to introduce these hooks. We really only want one place to alter theme suggestions, we don't need two. If we don't take out
$variables['theme_hook_suggestions']
then (in theory) this would introduce a lot of cruft in contrib modules: some would add via these hooks and others would still add them via$variables['theme_hook_suggestions']
.... let's not do that please :)Comment #52
markhalliwellScratch my above comment, misunderstood.
Comment #53
Fabianx CreditAttribution: Fabianx commentedI absolutely agree that there should be only one way to define suggestions, that is a necessary change.
I asked if the change to the engine definition is necessary and that templates don't have $vars['theme_hook_suggestions'] available anymore.
Both are rather big and unnecessary changes as that line could be replaced with:
from:
to:
So while it is no longer possible to change the suggestions via $variables, it is still possible to print them in templates and the engine definition remains the same.
Comment #54
star-szrThis patch should address most of #50 but I still want to add some more tests around '#theme' => 'node__article' scenarios.
Regarding the discussion from #51 to #53 about including theme_hook_suggestions in $variables, I didn't want to include those and not have them be alterable. I could be convinced to go either way on that but I thought including them would be more confusing to people who don't know about these new hooks who would then try to change that array and nothing would happen.
As for this note from #50:
To fix #939462: Specific preprocess functions for theme hook suggestions are not invoked I think we would need to change this behaviour and add new tests, but AFAIK currently the suggestion-specific implementation never has its own preprocess functions so $info[$processor] is always empty. The existing functionality is covered by Drupal\system\Tests\Theme\ThemeTest::testPreprocessForSuggestions() and those tests fail if you delete these lines. This patch adds a clearer comment that explains the change and the code but the functionality is covered by existing tests.
Comment #55
star-szrNeeds more tests.
Comment #56
star-szrOkay, I now understand after talking to @Mark Carver on IRC what @Fabianx was proposing in #53 and I agree. We can just add the theme hook suggestions into $variables right before the render engine function is called. My concern
iswas that preprocess functions would see this and try to alter it, and this would make twig_debug easy to break.So NW for that as well :)
Comment #57
Fabianx CreditAttribution: Fabianx commentedThe $suggestions being an empty array feels a little strange, because there already is a suggestion coming in as either
a) array or
b) direct theme callback
via the first argument to theme().
I am fine with that being follow-up though, because this is really tricky to unwrangle and hook implementations can look at $theme_hook_original to get that information.
Besides the BC needed for theme_hook_suggestions, I am fine with the changes here :).
Thanks!
Comment #58
star-szrShould have time to work on this tonight or tomorrow.
Comment #59
star-szrHere are the changes in line with #53 so that we pass in the template suggestions right before calling the template render function. Much simpler (and the patch is smaller), thanks @Fabianx :)
Still needs work after this to add a few more test cases.
Comment #60
star-szrHere are the tests for calling specific theme suggestions like node__article to verify the correct suggestions alter hook is fired.
Also cleaned up some of the existing tests by getting rid of unnecessary assertion messages, the default '"@text" found' will do just fine :)
Comment #61
star-szrMoving these unrelated changes out to #2050883: Incorrect function references in views_preprocess_comment() and views_preprocess_node() to make this patch easier to review and more focused.
Edit: I also cancelled a couple of the tests above since the queue is a bit backed up. So this should be the corresponding green to the red test-only patch above :)
Comment #63
markhalliwell#61: 1751194-61.patch queued for re-testing.
Comment #64
Fabianx CreditAttribution: Fabianx commentedTwo things need to happen here:
* The issue summary needs to be updated with the template.
* This is an API change for changing beahvior in preprocess (albeit a necessary one), so we need to go through the approval process.
Approval process
Theme system maintainers need to agree that this is
a) major
b) necessary
c) this patch is they way to solve this.
Then we need to get approval from a core committer and add the "API change approved" tag.
There is a flow-chart for that, see:
https://drupal.org/node/2045345
Thanks!
Comment #65
Fabianx CreditAttribution: Fabianx commentedThis could use a comment that this is needed to keep backwards-compatibility for templates and render engines.
Besides that looks fine to me, is almost RTBC (from my POV) besides the tasks I outlined above.
Comment #66
Fabianx CreditAttribution: Fabianx commentedRe-adding tags ...
Comment #67
star-szrRerolled to fix conflicts after #1843650: Remove the process layer (hook_process and hook_process_HOOK) was committed, no other changes.
Comment #68
star-szr@Fabianx pointed out in IRC that the logic in views_preprocess_node() got changed, this should at least be consistent with what was there previously.
Comment #68.0
star-szrUpdated issue summary.
Comment #69
star-szrIssue summary updated, going to try and profile this.
Comment #70
star-szrFirst round of profiling, standard homepage with 10 nodes.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f5bb078e5a7&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f5bb078e5a7&...
Comment #71
Fabianx CreditAttribution: Fabianx commentedMore profiling results (50 nodes, up to 4 comments per node):
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f628573214c&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f628573214c&...
Comment #72
Fabianx CreditAttribution: Fabianx commentedAnd the same data from XHProfCli:
As we can see the data matches well.
Comment #73
Fabianx CreditAttribution: Fabianx commentedAgain it is the EntityBCDecorator that makes a seemingly fast operation slower:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=views_theme_...
As expected the majority of the time is spent in ModuleHandler::alter:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51f628573214c&...
Comment #73.0
Fabianx CreditAttribution: Fabianx commentedUpdate issue summary to use template and explain changes
Comment #74
Fabianx CreditAttribution: Fabianx commentedI am setting RTBC status, because we deem it correct and "funtionality finished" by now.
We have tests, profiling and a nice issue summary.
The API change is needed to be able to properly support calling preprocess or prepare_alter functions that include suggestions and is part of the general theme system cleanup.
The only BC breaking change is that suggestions can no longer be set within preprocess, but can be set within the new hook_theme_suggestions_alter or hook_theme_suggestions_BASE_THEME_ID_alter(). (BASE_THEME_ID == theme HOOK used, like "node" or "comment" or "container").
Comment #74.0
Fabianx CreditAttribution: Fabianx commentedProfiling completed
Comment #75
webchickMark Carver brought this to me tonight for approval for the API change here. Here's a summary of the conversation.
This issue, in combination with #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK(), will fix #939462: Specific preprocess functions for theme hook suggestions are not invoked, which is currently classified as a critical issue. So that's a point in its favour. It also makes 2035055 much easier to solve, so there's another one. And, it's a much nicer and intuitive API to work with, so there's a third.
However, there are tons of .module hunks in here where code is being removed and re-added as hooks. This means that we're breaking module developer APIs, which is exactly what http://buytaert.net/drupal-8-api-freeze tells us we shouldn't be doing at this point. I also can't quite suss out what about 939462 makes it a literally release-blocking issue; most likely it's just a major one, which subtracts a point from above. And finally, the people working on the theme system knew the same deadlines as all the people working on every other system, so it's unclear to me why theme system changes would be granted special exceptions to the code freeze date. At best, this confuses people, at worst it can create feelings of resentment among contributors.
That said, the risk to committing this and actually breaking someone's ported D8 module out there are incredibly low, especially if it happens soon, the benefits are obvious and clear, and even if 939462 is only major, closing a major issue is still a win.
So I think I find myself on the side of the fence that would add the "approved API change" tag to this issue, but I'd like to sleep on it first, and/or a second opinion from one of the other core maintainers.
Comment #76
star-szrNeeded a quick reroll.
Comment #78
star-szrFixed up the one line in the test after #2033383: Provide a default plugin bag landed and also got rid of a deprecated config() in favour of \Drupal::config().
Comment #79
markhalliwellPatch was rerolled, test before last was fixed and this test is now green. Back to RTBC. Also FWIW: we haven't released anything since July 1st, so that's another plus to get this in asap.
Comment #80
mikl CreditAttribution: mikl commentedYeah, would be great if we can get this in before it's one year anniversary – good job on keeping the patch alive for so long, with Drupal changing so much in the meantime…
Comment #81
webchickSince catch voiced concerns about theme-related changes that affect module developers, I'd like him to give final sign-off on this. Assigning.
Comment #82
star-szrStraight reroll, some context in the patch was removed in #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off so the patch no longer applied.
Comment #84
star-szr#82: 1751194-82.patch queued for re-testing.
Comment #85
Fabianx CreditAttribution: Fabianx commentedPassing so back to RTBC
Comment #86
catchFine with the API change - this is necessary to fix #939462: Specific preprocess functions for theme hook suggestions are not invoked so should really have been critical.
I had a look through and nothing jumped out, but not enough to commit tonight so unassigning in case someone beats me to it.
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedRaising priority and tagging per #86.
Comment #88
webchickThanks, catch!
It is a bit bizarre for a module to implement an alter hook on behalf of itself. In hook_perm(), for example, you just return a list of permissions you want to add to the list; you don't take the list of permissions by reference and alter it.
It seems to me we might want to either rename hook_theme_suggestions_alter() to just hook_theme_suggestions(), or possibly add it in addition. Not sure what the performance impact of that would be, though.
Like, these actually make sense as alter hooks, because system.module is altering things defined by the theme system and views.module is altering things defined by node.module/comment.module.
There's nothing in theme.api.php that explains what "theme suggestions" are, so we should talk about it here.
This could do with a plain English description, and a @code example to explain the concept.
Comment #89
star-szrThe more I think about this, hook_theme_suggestions() AND hook_theme_suggestions_alter() seems like the way to go. That needs to be profiled to see what the cost is. I don't think it's adding another phase, just refining the suggestions phase.
The biggest advantage I see is that the module implementing the theme hook would always have its suggestions come first (be less specific) which makes sense to me. If everyone is implementing alter hooks then it's just based on alphabetical order and hook_module_implements_alter().
Comment #90
webchickYep, and that's a pretty standard pattern used across lots of modules: "build the thing," "alter the thing," "alter specific things of the thing" (uh. ;))
Comment #91
tstoecklerLet's name it hook_theme_suggestions_info() and hook_theme_suggestions_info_alter(), then please. That's the standard used throughout core.
Comment #92
webchickAgreed.
Comment #93
star-szrAnd we still want hook_theme_suggestions_info_alter() (the generic alter for all hooks), right?
Comment #94
mikl CreditAttribution: mikl commented#91, #92, that seems pretty backwards.
The reason for the _info suffix on, say, hook_block_info or hook_entity_info, is to differentiate it from hook_block_view and the other hook_block_*
We don't need any differentiator here, since there is no hook_theme_suggestions_view (nor would it make sense to have one).
Also, all the other _info hooks provide metadata for something. This case doesn't. This hook does not provide info(rmation) about suggestions. It provides suggestions.
So I'd urge you to reconsider. _info makes no sense here, and only serves to make this hook name even longer.
Comment #95
a.ross CreditAttribution: a.ross commentedWhile anyone in their right mind would generally support keeping function names short, I think applying naming conventions is more appropriate here. That convention, I assume, doesn't hinge merely on differentiating between various similarly named functions.
Comment #96
mikl CreditAttribution: mikl commentedNo, the _info suffix also serves to denote those hooks who provide metadata:
I don't think `hook_theme_suggestions` (and its alter variation) fits the pattern here. It does not provide Drupal with new information. It does not declare new templates. It does not provide metadata. It only serves to indicate which templates Drupal should use for what purpose.
Comment #97
markhalliwell@mikl is correct.
_info
means we're returning an associative array of information, which in this case we are not. We are only providing a simple array of template suggestions.Comment #98
Fabianx CreditAttribution: Fabianx commentedAgree on #96:
We have theme suggestions, we return them and we alter them. No need for info, which returns structured data.
In general the new thing would be:
* hook_theme_suggestions() - like hook_theme_prepare() should be defined by the owning module or a module extending a base type.
* hook_theme_suggestions_alter() - can be defined by "foreign" modules and themes.
So hook_theme_suggestions() would return:
return array(
'node__1',
'node__foo'
);
and moduleHandler->invoke is used.
While the alter hook would alter that array.
Works for me.
Comment #99
jenlamptonI think if we're going to add *another* new phase to the theme system - hook_template_suggestions() - then we need to remove something. Right now we have 4 ways to do the same thing.
If you want to add a theme_hook_suggestion in Drupal 8, you can do the following
In Drupal 7, the equivilents would have been:
Comment #100
jenlamptonAfter much discussion in IRC, I decided I could be convinced to do this if I was allowed to kill something.
So: #2063793: Remove the ability to provide a 'pattern' in hook_theme()
Comment #101
jenlamptongrr dreditor. retagging.
Comment #102
tstoecklerNo, #96/#97 are not correct. The _info suffix is (AFAIK) consistently applied throughout core currently, wherever we have a "build information" step and then a separate "alter information" step.
This is not correct. Whether template suggestions are or are not "information" is in the eye of the beholder. I would certainly answer that question with yes. Whether info hooks return associative arrays or not is an implementation detail.
Furthermore, I would actually suggest to use associative arrays to allow for easier altering. Otherwise one will have to use array_search().
Please let's just use the existing standard here, please. If you disagree please open a separate issue. I am totally open to a discussion about improving our current standards, but while we have them let's use them.
Comment #103
Fabianx CreditAttribution: Fabianx commentedRe-adding tags
Comment #104
Fabianx CreditAttribution: Fabianx commentedI am not convinced on the _info, but I don't care enough about naming either.
I just care about:
* The alter is after the THEMEID
* We stick with theme_suggestions instead of template_suggestions for D8 as we still have theme functions and suggestions work for those as well (even if discouraged).
This is not true as the last implemented hook/THEMEID in the array wins.
So you can just _add_ to the suggestions in the alter without having to remove anything.
But having an associative array on the other hand (on what) would not help either, so array_search it would need to be.
Comment #105
mikl CreditAttribution: mikl commented#102: You can't just say we are incorrect without providing actual examples. Please point out where an info hook is not used to provide metadata in nested arrays.
There's no standard, or even a convention defining that that all the stuff that can be _alter()'ed must be named info, and there's plenty of cases where it's not the case: hook_js_alter(), hook_css_alter(), hook_admin_paths_alter(), hook_batch_alter(), hook_file_url_alter(), hook_form_alter(), need I go on?
Please, in the future, try to provide a cohesive argument for your position (you know, with facts and examples), rather than simply stating that the well-reasoned position of others is “incorrect”. If not, you're just inflaming the issue and wasting everyone's time.
Comment #106
tstoecklerCertainly you *can* but the whole point of the _info/_info_alter() separation is that you add in _info() and *never* in _alter()!
As I tried to explain the type of data structure returned by an info hook is irrelevant. You might be right that they all return associative arrays, I'm not sure. But that's not the point. The point is the separation of "build" and "alter".
That said, I don't feel as strongly about this as others, so I won't fight it in any way. I still think it would be inconsistent to lose the _info suffix, but I won't lose sleep over it.
Comment #107
mikl CreditAttribution: mikl commented#106:
Did you actually read my post where I named a whole handful of examples of _alter() hooks? Most of these do have a build step as well.
So you just posted to point out that you're right, and we're deluded by our emotions? That's nice. You might want to read “How to disagree” at some point.
Comment #108
tstoeckler@mikl: Sorry, if I come off as condescending, that was not my intention. I don't think you're deluded by your emotions. I realize that you have made a technical case for your opinion. I tried to make mine. And we disagree. That happens. And it seems a resolution of this conflict is not immediate. So instead of continuing the discussion I'll just defer to the people that have actually put effort into this issue in favor of actually making progress.
Yes, I did read your post multiple times. Apparently I did not do a very good job of explaining my position. I did not rebutt your reasoning in detail because, as I tried to explain, I personally think it's coming from the wrong angle. But again, I defer.
In case you still feel wronged by me, maybe we can resolve this in IRC?
Comment #109
webchickGuys, please remember we're all on the same side here and want to make Drupal better. :) Here, have a ponycorn:
FWIW though, I agree with mikl that this seems to fall more in-line with the examples he cited in #105 than hook_entity_info(), etc. Now let's please put harsh feelings aside and get 'er done!
Comment #110
mikl CreditAttribution: mikl commentedFor the record, I don't feel wronged. We're just arguing about computer code here :)
Duly noted.
Comment #111
tstoecklerOMG, that is the Web 3.0 version of the
<blink>
tag... :-)Comment #112
mikl CreditAttribution: mikl commentedRetagging.
Comment #113
mikl CreditAttribution: mikl commentedFollowing up on #88, should we simply drop the _alter suffix, since in many cases we're not altering, but simply adding?
Comment #114
Fabianx CreditAttribution: Fabianx commentedThere is another case for dropping _info:
The function signature of _info is:
hook_element_info() {
}
Our function signature is / would be:
hook_theme_suggestions($variables, $hook) {
}
As the suggestions almost always depend on the $variables contextual information. (node__article is only possible if you know $variables['#node'])
That was also the case for having only the _alter case, because altering is usually done within some kind of context _and_ we already come in with suggestions to theme - even if one hook had been pre-selected already at this point.
There is two ways to supply theme suggestions to theme() directly (which can be combined):
a__b__c__d__...__z
or
array('a__c', 'a__b', 'a')
which is very confusing in itself as only the first suggestion is stripped from start to end, but the rest must exist as is in the theme registry.
So technically if I call:
#theme => array('node__mysuggestion'),
Then before the hook_suggestions_alter there is already a matching hook selected, which has the highest priority and which we currently don't allow to alter ... (are we missing test coverage here? Do we need to allow to alter the default suggestion if it is found? Has this been possible before?)
But if I call:
#theme => 'node',
Then the suggestions is empty at the beginning and modules "alter" the suggestions with their own.
I wanted to avoid digging into how suggestions that are passed to theme() are altered, etc. and liked the beauty and scope of the original patch but it seems the discussion is necessary to have something that makes sense overall especially in the situation of _alter vs. non-alter.
Thanks,
Fabian
Comment #115
Fabianx CreditAttribution: Fabianx commentedWe definitely miss test coverage for the different arguments to theme():
* #theme => 'hook__suggestion', => hook--suggestion.html.twig should be used.
* #theme => 'hook__suggestion', module adds hook--suggestion2 => hook--suggestion.html.twig should be used.
* #theme => 'hook__suggestion', module alters it specifically => altered version should be used.
And it gets even more complicated if we pass an array of suggestions to #theme ...
Comment #116
thedavidmeister CreditAttribution: thedavidmeister commentedNot sure if anyone noticed this yet, but other hooks sometimes have a _info() prefix, maybe we should think about doing that here ;)
Comment #117
catchYes no _info() suffix please, if there needs to be a suffix it could be _build() possibly. _info() hooks are mostly for statically defining stuff which is no what's happening here - if I saw _info() I'd expect it to be more like hook_theme().
I'm not really sure what the value of two stages is here anyway - given adding a suggestion will win.
Comment #118
thedavidmeister CreditAttribution: thedavidmeister commentedThe reality is that each "phase" we add will incur some overhead so if we can remove one without causing problems, maybe we should do that.
Comment #119
webchickCan we do this with just one phase (hook_template_suggestions())? Is there a use case for "I want to remove module X's theme suggestions" as opposed to "I as 'custom' module want to add some template suggestions that just so happen to be named 'node__' something"?
Comment #120
mikl CreditAttribution: mikl commentedI would be all for doing away with the
_alter()
step, but I think we should still dohook_template_suggestions[_HOOK]
. Perhaps, we should do that exclusively. Is there a use case for usinghook_template_suggestions
to add suggestions to all theming hooks? I would expect in almost all cases, you want to add suggestions for a specific hook.Comment #121
a.ross CreditAttribution: a.ross commentedIf there is no other use case, it'd still be useful for debugging I'd say.
Comment #122
star-szrThe issue I see with doing away with the _alter() step is this (IMO quite common) scenario:
Yes, you can change the module or hook weight but this seems like a pretty common use case that would be painful to accomplish.
Previously, template_preprocess_node() was serving as the "build" step (always runs first), and mymodule_preprocess_node() allowed manipulating/adding to the suggestions array.
So roughly the mapping of old API to new (not worrying too much about the naming of the new hooks):
template_preprocess_node() -> node_theme_suggestions_node()
mymodule_preprocess_node() -> mymodule_theme_suggestions_node_alter()
Comment #123
Fabianx CreditAttribution: Fabianx commented#122: And that is very consistent with the rest of the theme system re-architecture like theme prepare phase.
+1 on
template_* -> hook_theme_suggestions[_HOOK]()
mythingy_* -> hook_theme_suggestions[_HOOK]_alter()
Comment #124
webchickMake it so!
Comment #125
Fabianx CreditAttribution: Fabianx commentedYes, that works perfectly, just changing title from "template" to "theme" as in D8 we still support theme functions.
Unless someone strongly thinks otherwise ...
There was a x-post before on this ...
Comment #126
webchickYep, sorry, I was distracted when I made that title change. Let's make it happen. :)
Comment #127
jenlampton<3
Comment #128
star-szrI'm back onto this over the weekend :)
Comment #129
star-szrFirst a reroll since the patch no longer applied.
Comment #129.0
star-szrUpdated issue summary.
Comment #130
star-szrMany changes here. Main ones:
Regarding #115:
[Changed hook__suggestion to template__suggestion to make the following easier:]
The HOOK in hook_theme_suggestions_HOOK is always 'template', never 'template__suggestion'. 'template__suggestion' would always be available in $variables['theme_hook_original'] (as mentioned in #27, #57). Is that the "specific alter" case (third case)? Maybe I'm misunderstanding but I don't think the second case rings true, if you add a new suggestion to the end of the array (always present, not based on anything in $variables) and the template exists in the theme, it should be used. This is equivalent to the old $variables['theme_hook_suggestion'] or simply adding to the end of $variables['theme_hook_suggestions'].
I can add some more tests for when theme() is given an array of suggestions, I need to think through the use cases a bit more.
Updated the issue summary with a bit more information about the before/after API change and remaining tasks.
Comment #131
Fabianx CreditAttribution: Fabianx commentedUhm, nope.
We should remove dependency on theme registry, not add new to it.
This should be just:
$suggestions = Drupal::moduleHandler()->invokeAll()
and merge in the arrays.
with a follow-up to change it to:
Drupal::themeHandler()->invokeAll()
The reason being:
If I have a map, and the map module defines a suggestion, then if I now create a more specific map (bundle).
I want to add my suggestions just for this bundle I created, I just implement the suggestions hook as these suggestions are independent of any other suggestions.
So it is okay if several modules / themes implement theme_suggestions_HOOK.
Comment #132
Fabianx CreditAttribution: Fabianx commented#130:
One more thing:
The currently selected suggestion needs to be added to the the end before the altering.
Use-Case (and I have node--1--mynode.html.twig):
#theme => 'node__1__mynode'
* Suggestion is traversed
* node__1__mynode found in theme registry
node__1__mynode => used suggestion
Now suggestions are added:
node__article, node__1
In the old system the preprocess would get the node__1__mynode as theme_hook_suggestion.
In the new system it instead needs to be added at the end before the altering.
Obviously that is not true for a base hook.
And such cases need test coverage. I expect with current drupal they succeed, with the patch they fail as I cannot see how we preserve an original suggestion. (Might be missing something)
Comment #132.0
Fabianx CreditAttribution: Fabianx commentedUpdat remaining, add more information about API change
Comment #133
star-szrThanks @Fabianx, that all makes sense. Implemented the feedback from #131 and #132 and added test coverage to make sure node__1__mynode is added to the $suggestions array before the alter.
Edit: also some docs tweaks.
Comment #133.0
star-szrUpdate when the issue summary was updated
Comment #133.1
star-szrUpdate remaining
Comment #135
star-szrSo the test failures are because hook_theme_suggestions_HOOK is now being invoked during updates which is not allowed: https://drupal.org/node/2026515
Comment #136
Fabianx CreditAttribution: Fabianx commented#135: Does that mean we need to create an upgrade safe theme() function now? Or would we check the instance of the ModuleHandler?
I think we need an upgrade safe theme function or disallow usage of theme() during updates ...
Comment #137
star-szrI'm not sure the best way forward, it seems we may need to disallow theme() during updates. I'm only seeing these theme hooks being called in the test failures:
task_list
maintenance_page
update_task_list() calls drupal_render() on '#theme' => 'task_list' which seems like it could be refactored to late render after updates have completed. The maintenance_page calls may be much more difficult to refactor.
@Fabianx - Would the upgrade safe theme function be something similar to the old st() in Drupal 7?
Comment #138
thedavidmeister CreditAttribution: thedavidmeister commentedThese issues seem related #2060441: Convert update_results_page() to return a renderable array rather than early render #2072639: Convert update_info_page() from a string concatenation function to a render array builder
Comment #139
star-szrPatch no longer applied, here is a reroll. No changes.
Comment #141
star-szrFixes for:
Comment #143
star-szrFix for module_enable() being gone and a better check for theme API hooks as discussed in IRC - whitelisting any theme_ hooks from the system module.
Comment #144
markhalliwellYay! Finally back to green! I would love to RTBC this (again), but it still needs profiling.
Comment #145
Fabianx CreditAttribution: Fabianx commentedTo be more backwards compatible we should probably also provide the theme_hook_suggestion used.
for preprocess / prepare we will pass that as context instead though.
---
Besides this one nitpick, this is good to go from my side as well.
Now onto performance testing :).
Comment #146
star-szrHere is the BC fix, suggestions on improving the inline comments and/or code welcome of course :)
I stepped through an example before and after the patch and it works exactly the same.
Comment #147
mikl CreditAttribution: mikl commentedHere's a code example for the change note:
Suppose we wanted to use a different template for nodes that a have the
awesome field checked, here's how it could be done.
Before:
After:
Comment #148
joelpittetScenario:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f26a22745b&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f26a22745b&...
Comment #148.0
joelpittetUpdate when the issue summary was updated.
Comment #148.1
star-szrAdd before/after example code
Comment #148.2
star-szr(Update when the issue summary was updated)
Comment #148.3
star-szrSmall update to remaining tasks
Comment #149
mikl CreditAttribution: mikl commentedThe increase in function calls is a bit larger than I expected. Does this test really render more than thousands of templates, or is there something bad going on?
Comment #150
star-szrAs far as I can see it's almost all ModuleHandler.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f26a22745b&...
Comment #151
Fabianx CreditAttribution: Fabianx commentedhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=theme%403&ru...
This is the function to check.
60 calls to ModuleHandler more.
We knew we would need the new function calls and that that costs some performance, but there are no obvious bottlenecks, but it makes our rendering pipeline much straighter. (and any OO system would have even more overhead ...)
This has an overhead of 26 ms for this calls.
This should be fine and there is no performance regression.
Ready to be committed :-).
Comment #152
markhalliwellwoo hoo!!
Comment #153
webchickCool! Just assigning to catch to check over, since there's a performance impact here.
Comment #154
catchAre we saying the performance impact would be 13ms instead of 26ms without the alter hook?
I don't really understand why we have the two hooks (it was split into two quite late in the patch) and 13 ms is worth saving if we don't need both.
Comment #155
mikl CreditAttribution: mikl commentedI'm not a great fan of the _alter variant, but without it, we would remove the ability to mess with/react to suggestions added by other modules/themes, which is possible with the old _preprocess hook.
That could be said to be an improvement. So bottom line, +1 for removing it. I talked to mortendk, he agrees.
Comment #156
c4rl CreditAttribution: c4rl commentedCatching up with this issue at the Prague sprint.
It seems to me if we are going to simplify this patch to only one hook we should actually *keep* the hook_theme_suggestions[_HOOK]_alter() and remove hook_theme_suggestions[_HOOK]().
Reasons:
1. The main purpose of this patch is to decouple suggestions from the preprocess phase. The _alter() hook alone does just that.
2. In D7, we *only* had altering (via preprocess) and that seemed to be fine (in terms of use-cases, that is). Though I know in #90 we adhere to the "define/alter/alter specific" pattern, we are not really solving an existing problem and are introducing a new hook (which could actually be *more* confusing rather than less confusing).
3. The base template definition is always in hook_theme() and part of the registry, so this seems like a fine definition phase for most use-cases.
Reducing the performance impact by half seems worthwhile. Rationale for two hooks seems more philosophical rather than practical. Unless there are objections, I'll plan to work to refactor this to simply contain the _alter() variant.
Comment #157
jenlamptonI actually prefer only having the _alter hook (as originally proposed), especially if there are performance implications of adding more than one new phase. (updating issue title)
Comment #158
mikl CreditAttribution: mikl commentedMaybe a philosophical issue, but do we need the the _alter suffix when there's just the one hook? While it can be used for altering, it will likely be used for simply adding suggestions most of the time.
Comment #159
c4rl CreditAttribution: c4rl commentedI would advise the _alter stay since we are indeed passing suggestions via reference (and this is the classic alter pattern in Drupal) and I think it is good DX to have function/method names describe what they are physically doing as best as possible rather than what they are intended for or when they occur.
Comment #160
Fabianx CreditAttribution: Fabianx commentedUhm, the call to the alter is actually just 3 ms, which is probably not worth saving for.
(http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?symbol=Drupal%5CCor... and those are 25 legit calls)
Also there is a problem when removing _alter.
----- How do you decide if you want to override the default suggestion node__article or not?
The _alter and non-alter came in quite late, but for test coverage reasons.
Lets keep this as is. Back to RTBC for catch to re-review.
Edit:
The _alter phase supports the use case of what before had been overwriting the single $variables['hook_theme_suggestion'].
Comment #161
c4rl CreditAttribution: c4rl commented@Fabianx #160 Hm, I'm recommending we *keep* hook_theme_suggestions[_HOOK]_alter() and remove hook_theme_suggestions[_HOOK]() instead. Your comment seems to be addressing the opposite. :)
What are your thoughts given my remarks in #156?
Comment #162
Fabianx CreditAttribution: Fabianx commented#161: We need to keep _both_.
If you keep only one, you can either not alter or you can not not-alter (i.e. you are always altering).
Case 1 - only alter:
* article__node comes in
* You alter it automatically because you add to the end.
* => your node--whatever.html.twig (the node suggestions) is _always_ overriding the initial suggestion.
Case 2 - no altering, only adding:
* You cannot override an incoming suggestion.
Therefore you need both. Lets please keep this standard drupal pattern.
Comment #163
c4rl CreditAttribution: c4rl commentedI thought about this some and realized that two hooks are indeed necessary.
In D7, imagine foo.module and theme_foohook() being defined in foo_theme():
If we only implemented the _alter hook, this is the equivalent of only supporting "foo_preprocess_foohook()" and not "template_preprocess_foohook()."
* template_preprocess_foohook() serves as a definition phase because this always runs before any other preprocessor.
* foo_preprocess_foohook() serves as an alter phase (though an unusual implementation).
The naming is what threw me off since "template_preprocess_x" vs "foo_preprocess_x" have different execution order, i.e. "template_preprocess_x" is not technically a hook since it runs first and is irrespective of module weight. :P
So, if we only had an _alter hook, hooks run in weight order, so definition as a pre-cursor to other implementations isn't possible.
Indeed we need both. Apologies for the confusion here.
Comment #164
jenlamptonRetitling :)
Comment #165
markhalliwellYes, _both_ are needed. Mind you the goal here is implement (and alter) suggestions _before_ the prepare (preprocess) phase: #2035055: Introduce hook_theme_prepare[_alter]() and remove hook_preprocess_HOOK(). That issue will effectively be deprecating (eventually replacing) the preprocess layer entirely though. We cannot invoke these new hooks without having proper suggestions (and altered ones) first.
Yes :) RTBC.... please let's get this in, I have much work to do now!
Assigning back to catch per #160.
Comment #166
catchOK that's fair enough, just wanted to check.
Committed/pushed to 8x, thanks!
Comment #167
markhalliwellI'm soooo happy right now!!! :D
Comment #168
mikl CreditAttribution: mikl commentedSee #147 for a code example for the change notice.
Comment #169
mikl CreditAttribution: mikl commented*sigh* retagging.
Comment #170
steveoliver CreditAttribution: steveoliver commentedCreating change notice.
Comment #171
star-szrThanks @mikl, I also updated the issue summary with code examples based on yours :)
And I think the change notice itself is "only" major: #2083415: [META] Write up all outstanding change notices before release
Comment #172
star-szrCrosspost. Go @steveoliver go!
Comment #173
steveoliver CreditAttribution: steveoliver commentedCreated change record: New hooks for theme suggestions.
Leaving active because there are still remaining tasks.
Comment #174
c4rl CreditAttribution: c4rl commentedRE: This @todo. Already includes a code sample, yes? So can we remove the @todo's and cross-off that remaining task on the issue?
Comment #175
Fabianx CreditAttribution: Fabianx commentedIf I see this correctly all @todos are addressed, but leaving for Cottser to decide this.
Comment #176
star-szrSee @webchick's review in #88 for the @code bit - some other hook docblocks use this, hook_page_alter() is an example. I won't be able to get to the remaining tasks anytime soon so anyone feel free to jump in.
Comment #177
c4rl CreditAttribution: c4rl commentedTo preserve the intent of this issue, I've created #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter() as a follow-up and marking this one as fixed.
Comment #177.0
c4rl CreditAttribution: c4rl commentedMinor tweak to proposed resolution
Comment #178.0
(not verified) CreditAttribution: commentedMoving remaining tasks to separate issue to mark this one as fixed.
Comment #179
c4rl CreditAttribution: c4rl commentedI think this has possibly introduced the need for a follow-up, based on a DX problem.
Formerly, available suggestions could be grokked by inspecting the respective
$variables
parameter via a preprocessor function. Understanding what suggestions are available is a necessary task for themers to decide what templates and hooks they wish to use.As the suggestions have moved, the original method of inspection no longer exists. Presently, you have to dump
$suggestions
conditionally based on$base_theme_hook
right afterhook_theme_suggestions_HOOK_alter()
runs intheme.inc
. I believe 'hacking-core-as-a-method-of-inspection' is bad DX.Therefore, (as a stopgap until the theme system is redeveloped to be OO, i.e. D9+) it seems plausible that we should add something like
$variables['theme_suggestions']
that would be read-only, i.e. have no adverse effects ontheme()
or child function calls. In this way, the method of inspection is preserved.Thoughts?
Comment #180
c4rl CreditAttribution: c4rl commentedAnother potential problem. Preprocessors are available to both modules and themes. However,
ModuleHandler::invokeAll()
*only* calls out to modules and *not* themes. Since this has moved from a preprocessor to a hook called viainvokeAll()
that means a theme cannot provide suggestions, which breaks functionality we once had.In that regard, since this issue was once Critical, should we address this problem in #2111079: Add @code sample and test coverage per hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter(), also bumping it to Critical?
Related #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler)
Comment #181
c4rl CreditAttribution: c4rl commentedAfter some discussion in IRC, Cottser has provided some insights.
Regarding #179, a theme can implement
hook_theme_suggestions_alter()
to discover what templates are available.Regarding #180, Because themes generally invoke hooks after all modules have been called, the use-case for implementing
hook_template_suggestions()
doesn't particularly apply in most cases since it is physically impossible for anything to alter what a theme has provided. Nevertheless, since themes can indeed callhook_theme()
, andhook_theme_suggestions()
is bound fairly explicitly tohook_theme()
, I would prefer to have consistency across modules and themes here: Someone new to Drupal leans abouthook_theme()
andhook_theme_suggestions()
in the realm of modules. So yay. Then they are writing a theme, and invokehook_theme()
in this theme, but can't dohook_theme_suggestions()
, they have to dohook_theme_suggestions_alter()
. So this seems like something else they have to understand, rather than just providing a consistent API.Minimally, we could provide a note in the documentation about
hook_theme_suggestions()
as a stopgap until #2029819: Implement a ThemeHandler to manage themes (@see ModuleHandler) is doneComment #182
star-szrComment #183
akalata CreditAttribution: akalata commented