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
HOOK_theme() offers an option 'override preprocess functions'. Here's the relevant documentation. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...
* - override preprocess functions: Set to TRUE when a theme does NOT want
* the standard preprocess functions to run. This can be used to give a
* theme FULL control over how variables are set. For example, if a theme
* wants total control over how certain variables in the page.html.twig are
* set, this can be set to true. Please keep in mind that when this is used
* by a theme, that theme becomes responsible for making sure necessary
* variables are set.
This is not implemented at all.
Proposed resolution
I have two propositions.
- Remove the feature.
- Remove documentation.
- Remove the code that mentions this parameter. It doesn't actually do anything relevant. See below.
- Fix the feature.
Here's the only place where this parameter is used in the code:
\Drupal\Core\Theme\Registry
// Check for the override flag and prevent the cached variable
// preprocessors from being used. This allows themes or theme engines
// to remove variable preprocessors set earlier in the registry build.
if (!empty($info['override preprocess functions'])) {
// Flag not needed inside the registry.
unset($result[$hook]['override preprocess functions']);
}
elseif (isset($cache[$hook]['preprocess functions']) && is_array($cache[$hook]['preprocess functions'])) {
$info['preprocess functions'] = array_merge($cache[$hook]['preprocess functions'], $info['preprocess functions']);
}
Seems that this code doesn't do it's job anymore.
Steps to reproduce
For example set the 'override preprocess functions' for the html hook like this:
core/includes/theme.inc
/**
* Provides theme registration for themes across .inc files.
*/
function drupal_common_theme() {
return array(
// From theme.inc.
'html' => array(
'render element' => 'html',
'override preprocess functions' => TRUE,
),
...
Rebuild the cache and put {{ dump() }} inside core/themes/classy/templates/layout/html.html.twig. See how all the variables are added from the template_preprocess() and template_preprocess_HOOK() functions.
Remaining tasks
Let's first discuss how we should resolve this and then write the decided tasks here.
User interface changes
None.
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 2561343-11.patch | 2.33 KB | marvin_B8 |
Comments
Comment #2
jiv_e CreditAttribution: jiv_e as a volunteer commentedComment #3
jiv_e CreditAttribution: jiv_e as a volunteer commentedFix typos.
Comment #4
lauriiiBumping to major
Comment #5
jiv_e CreditAttribution: jiv_e as a volunteer and commentedThe following issue introduces the code https://www.drupal.org/node/166681.
See also @merlinofchaos's and @Dries's comments. https://www.drupal.org/node/166681#comment-592091
At least I understood that setting the parameter like this would disable all the preprocess functions.
But it doesn't. All the preprocess functions are run regardless of the 'override preprocess functions'. Or then I don't understand what the documentation says. Which is not so big of a problem if everyone else does. Do you?
I have to take my earlier claim in the issue summary back a little bit. I wrote about the current implementation "This (code) doesn't actually do anything because the unset array element is not used or referred anywhere." This is not necessarily the case because there's an elseif structure. I edit the issue summary to match my current understanding. Still it seems to be broken on Drupal 8.
Based on @dvessel's last comment on the pre-mentioned issue I think this could be removed. If I understand correctly we have more control on this kind of stuff on Drupal 8. Couldn't a theme even provide a custom ThemeManager service if it has these exotic needs?
Comment #6
valthebaldWhat's the consensus? Remove documentation about non-working feature?
Comment #7
star-szrI think we need theme system maintainers to weigh in. Since I'm one of them I'll start:
Considering this no longer works we should probably just remove the documentation. Unless I'm missing something this can be accomplished if there is really a use case (it seems like an edge case) via hook_theme_registry_alter().
Digging deeper…
This was added before hook_theme_registry_alter() existed, and before that hook could be called from themes. See #166681-17: Overriding template preprocess functions not possible..
We can also grep contrib to see how often this was used in D7.
Comment #8
star-szrComment #9
lauriiiThis has been a way to do micro optimization for performance in the past. There is no performance gain anymore on run time for using
'override preprocess functions' => TRUE,since preprocess functions are cached in the theme registry. I can't think of the use cases where this is needed. I would consider just removing docs and any remaining pieces of the functionality.Comment #10
star-szrYup if it's broken it's not doing anyone any good. I'd also like to add that there is also the 'preprocess functions' key that could be used for this type of use case and I verified recently that it works :)
Comment #11
marvin_B8 CreditAttribution: marvin_B8 as a volunteer and at comm-press commentedis that right ?
Comment #13
Wim LeersComment #14
jiv_e CreditAttribution: jiv_e as a volunteer and commentedComment #15
alexpottIs this actually broken? It does not look broken to me. Given that removing this is API change and is probably only eligible for Drupal 9. It does look untested :)
Comment #16
star-szrDid some digging on this today and I agree with @alexpott I think it does still work.
I ran
git log -S "override preprocess functions"and only saw docs and the relevant code being moved around but not being functionally changed/broken. I'm adding what I think is the most relevant related issue that actually changed the behaviour here. The issue summary from this related issue isn't accurate because the code snippet there is showing an array of "override preprocess functions" rather than a boolean but looking past that this part is pretty relevant:So changing 'html' in drupal_common_theme() will have no effect since everything else will be running after that. You'd have to do it from later, not from the hook_theme that is defining the theme hook (we need better names for these things).
Based on that and @alexpott's comment I'm changing this to a normal task to add some test coverage for this feature because we definitely don't have any.