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/Motivation
A theme hook can specify a 'base hook' in order to inherit the preprocess functions.
This currently only works if the theme hook uses the __ pattern:
function mymodule_theme() {
return [
'foo__bar' => [
'base hook' => 'foo',
],
];
}
Proposed resolution
Specifically handle the theme hooks not using a double underscore by merging directly, while leaving the while() loop handling in place for double underscores.
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#51 | 2559825-preprocess-51.patch | 20.37 KB | tim.plunkett |
#51 | 2559825-preprocess-51-interdiff.txt | 1.86 KB | tim.plunkett |
Comments
Comment #2
lauriiiComment #3
aspilicious CreditAttribution: aspilicious commentedThis broke a lot of the DS functionality.
DS uses a plugin system to "field" templates.
Due to this change the preprocess field hooks aren't called anymore.
If we use '#theme' => 'field' the field preprocess function should be called if 'base theme' is equal to 'field'.
Comment #4
aspilicious CreditAttribution: aspilicious commentedI maybe have a workaround for DS, but we clearly need a CR for this...
I'll post a link to the issue once it is fixed.
Comment #5
aspilicious CreditAttribution: aspilicious commentedhttps://www.drupal.org/node/2575865
Fixed it for us, I wonder if we still need the base_hook stuff than...
Comment #6
star-szrSorry @aspilicious, there are so many (too many IMO) ways to do theme suggestions, I agree we need more tests.
Comment #7
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedAdded tests for this issue and better coverage for postProcessExtension in general. The patch fixes this issue as well as some more the tests came up with and rewrites completeSuggestion to be more readable.
Comment #9
lauriii@Antti J. Salminen: Thanks for the patch and especially (again) for the extended test coverage! It is for sure fixing a major bug in the theme system but I don't think its fixing the bug described in this issue. Would you mind opening new issue for the 'base hook' issue you are fixing in the patch? In this issue we should focus on removing hardcoded use case of __ separator pattern for specific theme suggestions.
Comment #10
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedlauriii: It does try to fix the issue that aspilicious references and that caused the creation of this issue. Correct me if I'm wrong but looked like it was caused by a hook_theme entry with a base hook but not utilizing the suggestion patterns at all.
The test called testManualBasehook() attempts to test that situation. The function mergePreprocessFunctions() that I added is called for hooks that aren't using the suggestion pattern.
Of course this touches on things other than that since I ended up fixing more than just this... I did consider making a separate issue for all the other changes but after the refactoring this one would be like a 2-3 line change with all the work already done... could still go with that if you prefer.
Comment #11
joelpittet@Antti J. Salminen that would likely be better to fix just the one small issue and move the other changes to follow-up as it will be easier to review and commit I'd bet.
Comment #12
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commented@joelpittet Sure, when claiming it would be tiny I thought it wouldn't be possible quite as nicely as it actually was in the end. The attached patch is now reduced in scope to just the parts directly relevant to this issue.
Comment #14
lauriiiThis should be in the end of the file instead, otherwise the patch looks good now
Comment #15
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedFixed a couple of things based on lauriii's review/feedback and reduced mergePreprocessFunctions to only merge in the preprocess functions. Doing more will require specific handling of keys and seems out of scope for this issue.
Comment #16
lauriiiLooks good for me. Waiting for feedback from @aspilicious if this addresses problems he was having.
Comment #17
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedRerolling. It's been a while so maybe we can move forward anyway. Even in the somewhat unlikely case that aspilicious' issue it turned out to be a different one, this patch still does fix an issue and provides tests for it.
Comment #18
aspilicious CreditAttribution: aspilicious commentedGot no time to verify if this works.
I trust the hero's above.
Comment #19
lauriiiDiscussed with @catch, @alexpott, @effulgentsia, @xjm , @Cottser and @joelpittet. We all agreed this should be downgraded to a normal bug. The reason why this was first opened as a major bug was because it was considered as a regression / contributed project blocker. Now given the time, the modules I could think of were broken have worked around this. If this should be considered as a contributed project blocker, please feel free to update the issue.
Comment #20
Antti J. Salminen CreditAttribution: Antti J. Salminen as a volunteer commentedI assume it should still be set as needs review even if normal.
Comment #22
tim.plunkettWe now have a valid use case in core for this, it's currently being worked around.
See #2834019: Remove the "layout__" prefixing of theme hooks.
We could roll that into this issue if desired.
In the meantime, here is an updated patch fixing the indentation and some other issues.
Comment #24
tim.plunkettPatch ran against 8.2.x, hm.
Comment #25
tim.plunkettGetting sloppy at the end of the week.
Comment #26
tim.plunkettCleaned up the test methods to use @dataProvider
Comment #27
andypostLooks ready to go, only minor nit.
s/hook's./hook.
Comment #28
aspilicious CreditAttribution: aspilicious commentedNot sure about the comment in #27.
This is blocking DS for having a working test suite on the 8.x-3.x branch. (https://www.drupal.org/node/2821201#comment-11821081)
Comment #29
tim.plunkettThe comment means "Merges the preceding hook's preprocess functions into the current hook's [preprocess functions]", and is valid grammar.
Comment #30
tim.plunkettUpdated the issue summary
Comment #31
alexpottPreceding or complete? I'm not sure either makes much sense. How about mergeBaseHookPreprocessFunctions() and
$base_hook
Is it not possible to have both __ and have
$info['base hook']
set too? Or should we throw an exception if this is the case?Nice test coverage and nice to see this being cleaned up.
Comment #32
tim.plunkettAfter thinking about this more, I decided this approach was incomplete (hah!) and wrote some more test coverage.
There are 2 places that completeSuggestion() is called from, and each needs base hook support.
This rewrite makes #31.1 obsolete
#31.2, we should handle both! This does so, and adds coverage for it.
Comment #34
tim.plunkettRestoring some more the original approach, I think this is a happy medium.
The do-not-test patch is the changes to Registry without the new variable names and with -w, just to show how little is changing
Comment #35
tim.plunkettOkay great, that passes as it should.
This clarifies the logic (second check is redundant) and docs for the new code.
Comment #36
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis doesn't look like desired behavior. If the base hook is
'unrelated_hook'
, then we shouldn't be expecting the template to be set to'test-hook'
or to be accumulating preprocess functions from'test_hook'
and'test_hook__suggestion'
.Possibly related, this patch makes some nice improvements to
completeSuggestion()
, butpostProcessExtension()
still has regex and string parsing on'__'
that doesn't seem to account for'base hook'
.Comment #37
tim.plunkettThe question boils down to "Should the base hook be in addition or instead of suggestions?"
When I was rewriting this I took a step back and tried to think of what would be least surprising.
Right now the base hook is ignored and the suggestions are used. The goal of the issue is to allow preprocess functions from the base hook to fire. But does that really mean we want to prevent hooks from suggestions from firing? That seems like a BC break, and not what I'd expect to happen.
Comment #38
tim.plunkettSetting to NR for now. Let's discuss!
Comment #39
tim.plunkettFrom the docs for hook_theme() in theme.api.php
So you're absolutely correct. But since that's not currently true, is it safe to switch to that behavior?
Comment #40
tim.plunkett@effulgentsia and I stepped through this on a call, and came to the conclusion that the patch is correct, and that the docs (which neither agree with HEAD or the patch) should be updated.
I've also included an addition to the unit test that we came up with while discussing.
Comment #42
tim.plunkettThanks random failures
Comment #43
effulgentsia CreditAttribution: effulgentsia at Acquia commentedUnclear why some functions are in 'defined_functions' and others aren't.
That seems wrong, but maybe out of scope for this issue? This patch doesn't change code that deals with 'template', so perhaps fixing bugs and adding test coverage for that should be a separate issue?
In addition to above, I reworked the tests to be clearer on what they're testing and why. Please review to see if it captures everything we want captured.
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIf we're doing that first part in this issue, then I think we also need to be able to uncomment that commented-out test and make it work.
Comment #45
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think the logic in both mergePreprocessFunctions() and its caller is correct (or at any rate, compatible with what's in HEAD), but the name and docs of
$parent_hook
is then incorrect, since what we're passing in is$cache[$hook]
, not$cache[$cache[$hook]['base hook']]
. I'm not sure yet what a better name would be. In HEAD, it's pretty convoluted as to what "previous" means, especially when you look closely at the order of the lines of code.Comment #46
Berdir@aspilicious mentioned in IRC that this issue (and I guess the follow-up in to remove the layout prefix for layout templates) is now officially a blocker for using the new field layout module in DS.
The same is true for my Widget project (https://www.drupal.org/project/widget) and I guess everyone who wants to avoid having that prefix, which is especially important if you have existing templates.
This is already major and tagged with contributed project, so not changing anything there :)
Comment #47
tim.plunkettI really don't understand #43-45, or what is needed here.
#40 seemed fine to me.
Comment #49
aspilicious CreditAttribution: aspilicious commentedI prefer #40 for now as it is the only patch that is ready to commit as is.
Display Suite needs this patch (for 8.3!) if we want a working Display Suite for the next half year....
I tested the patch yesterday and it worked.
Comment #50
tim.plunkettComment #51
tim.plunkettAHA! Couldn't figure out why that didn't work, since the logic looked fine. Turns out we need to check either the source or destination for the incomplete flag, and then everything works.
Comment #52
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTicking some credit boxes.
Comment #55
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI tried figuring out if this is sensible or not, but wasn't able to convince myself one way or the other. However, it seems to fix #44, and doesn't break any other test, so in my opinion, better to get this into the 8.3 alpha than not. So, pushed to 8.4.x and cherry picked to 8.3.x.
I'm concerned that there might be unexpected bugs caused by it, but partly why we have alphas is to find stuff like that. For both #45 and the difficulty in evaluating the above line of code, I opened #2848242: Improve variable names and inspect and document logic of Registry::completeSuggestion() and Registry::mergePreprocessFunctions() for some brave soul to wade into.
Comment #56
aspilicious CreditAttribution: aspilicious commentedThnx a lot, just to confirm that one of the failing tests (related to this issue) is now working on the 8.3 branch.
Comment #57
andypostMaybe change record needed to notify about "incomplete process functions" in 8.3?
Comment #58
tim.plunkettThat was added in #939462: Specific preprocess functions for theme hook suggestions are not invoked, this just used it.
Comment #60
tim.plunkettFollow-up: #2886187: Document the regex in Registry::postProcessExtension() used to find preprocess functions