Problem/Motivation
hook_custom_theme()
fails to work under certain circumstances.
Call stack that causes the issue:
- On boot the redirect module checks for redirect:
redirect_load_entity_from_path()
- This builds the node, which attaches the fields
- Having a text field with media token support triggers
media_token_to_markup()
- Loading the file to create the markup triggers
file_entity_file_load()
- This triggers
token_replace()
for the alt / title tags. - Token replacement triggers
system_token_info()
- This creates a link using
l()
l()
wants to know the current theme thusdrupal_theme_initialize()
is called.drupal_theme_initialize()
defines global variables e.g.$theme
,$theme_key
. Once those variables are set the main part of the function is skipped. Unfortunatelydrupal_theme_initialize()
doesn't forcemenu_get_custom_theme()
to initialize.
Now what's the problem with that:
The call of the redirect module is made before menu_set_custom_theme()
was called, which is normally called after drupal_path_initialize()
in _drupal_bootstrap_full()
.
And the redirect module acts on hook_url_inbound_alter()
which is triggered by drupal_path_initialize()
.
Because that call is skipped hook_custom_theme()
never is invoked and we lose the control over the theme.
Proposed resolution
I see two approaches:
- Add a force flag to
drupal_theme_initialize()
to be able to re-set the global theming variables when calling the function in_drupal_bootstrap_full()
- Change
drupal_theme_initialize()
to force the initialization when callingmenu_get_custom_theme()
I think the first one is more unobtrusive and is the one I've added as patch.
Remaining tasks
Reviews needed.
User interface changes
None.
API changes
The function signature of drupal_theme_initialize()
has changed but in a transparent way. This means already existing code will work as before.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#66 | bootstrap_marker-2086335-66.patch | 1.68 KB | KeyboardCowboy |
#58 | drupal-n2086335-58.interdiff.txt | 3.2 KB | DamienMcKenna |
#58 | drupal-n2086335-58.patch | 7.61 KB | DamienMcKenna |
#34 | drupal-bootstrap_theme_init_issue-2086335-34.patch | 11.53 KB | das-peter |
Comments
Comment #1
mkalkbrennerProbably related:
#1222536: Bug in Redirect module causes ThemeKey issue - Must page reload in browser to switch theme
#1702008: file_load() in media.module overrides themekey setting
Comment #2
das-peter CreditAttribution: das-peter commentedIRC Conversation with Daniel:
According to this we probably should change the approach to fix this and ensure that
drupal_theme_initialize()
callsmenu_set_custom_theme()
.Comment #3
smitarai CreditAttribution: smitarai commentedI am encountering similar issues on a project.
To replicate on vanilla Drupal, I created a module (garland and stark are enabled themes)
<?php
function mymodule_custom_theme() {
if (arg(0) == 'node' && is_numeric(arg(1))) {
$node = node_load(arg(1));
if ($node->type == 'page') {
return 'garland';
}
else if ($node->type == 'article') {
return 'stark';
}
}
}
function mymodule_entity_load($entities, $type) {
if ($type == 'node') {
foreach ($entities as $entity) {
/** this is going to initialize the theme and break everything * */
$entity->foo = l(t('foo'), '');
}
}
}
I am including a patch which seems to get this working.
What the patch does it, it does not let the drupal_theme_initialize or theme_get_registry get loaded from anywhere else apart from
_drupal_bootstrap_full
Comment #5
srclarkx CreditAttribution: srclarkx commented#3 looks a lot like the first patch. The first patch didn't work for me, I ended up with a page in 2 different themes. #3 worked. Unfortunately it didn't pass SimpleTest. At a glance it looks like the difference is #3 prevents some code from running in theme_get_registry.
Comment #8
srclarkx CreditAttribution: srclarkx commentedsmitarai, Just for grins, I replaced the global with variable_set and variable_get to see if the patch would do better with SimpleTest
Comment #9
srclarkx CreditAttribution: srclarkx commentedSubmitted for testing
Comment #10
das-peter CreditAttribution: das-peter commentedvariable_set()
writes the value into the db. And we really don't want this as this is a flag for the current request only and nothing that has to be shared with other requests.So either a global (php) variable or
drupal_static()
is the way to go for request specific status flags.Comment #11
srclarkx CreditAttribution: srclarkx commentedIf I get a few minutes I'll update the patch or smitarai can do so. I read that global variables have to be put in the $GLOBALS array.
Comment #13
das-peter CreditAttribution: das-peter commented@srclarkx smitarais patch already uses global variables:
global $drupal_theme_init;
is the same as using$GLOBALS['drupal_theme_init']
.We should try as in #2 proposed:
Unfortunately I hadn't time to do so. But as I just recently came across some a ThemeRegistry "cache poisoning" in one of our sites I might can spend some time on it soon - error could be related to this issue.
Comment #14
smitarai CreditAttribution: smitarai commented@das-peter
Even if you ensure that menu_set_custom_theme gets called from drupal_theme_initialize() what happens in the case is that two different themes are loaded. The culprit is as I mentioned in my comment any code in entity_load which will in turn cause the registry to load, will also initialize the theme, and this will be done before the actual call if drupal_theme_initialize in _drupal_bootstrap_full(), and if you see drupal_theme_initialize, if it finds the global theme set, its not going to do any more processing. So the only way to stop it is making sure that is theme initialization is only through _drupal_bootstrap_full.
I can't figure out why my patch is failing simpletest because its not doing anything related to mysql at all. The only viable change that I do see in the patch is moving the global initializing to inside the not maintenance or update loop in _drupal_bootstrap_full.
Comment #15
smitarai CreditAttribution: smitarai commentedLets see if this one passes QA.
And I give up.
Comment #16
smitarai CreditAttribution: smitarai commentedComment #18
srclarkx CreditAttribution: srclarkx commentedsmitarai, I have a bare-bones drupal with simple tests running and I put together a custom module using the instructions in #3. I was able to confirm that that the tests for the patch are failing because of the changes in theme_get_registry(). I removed the check for drupal_theme_init from the theme_get_registry function and put a check in l(). This will prevent theme_get_registry from being called. I verified a few of the tests and they seem to pass. I haven't done enough debugging to submit a patch yet. I'm going to hand over a snippet for you and see if you have time to work on it.
I think the simpleTests are failing because the return value from theme_get_registry is not correct. Fixing the l function will not solve this issue. I did notice a few other modules were fixed to work around this issue, when the right solution would probably be to figure out how to prevent the theme from re-loading.
Comment #19
srclarkx CreditAttribution: srclarkx commentedThis is just a fix up of smitarai's patch from #15. It should be just about the same with some verification of the global before it's used. I don't know how to make the test button appear. This doesn't work for the issue I'm looking at. It does pass the tests and does work in my bare-bones example. It will at least give someone an idea of how to fix up the patch.
Comment #20
srclarkx CreditAttribution: srclarkx commentedOK, now I'm ready to test a patch. I have a subset of the simpleTests passing on my system and the patch appears to work on my problem. Fingers crossed.
Comment #21
das-peter CreditAttribution: das-peter commented@srclarkx Thank you very much for working on this! And it's green!! :)
For now here's just a visual review - most of the stuff is feedback from Code Sniffer - I highly recommend to integrate it with your IDE as it saves you from such Coding standards feedbacks.
Further I think the code-bits need documentation to make clear why this stuff is in place and what happens if it's not there.
No space found before comment text; expected "// create global variable for theme init" but found "//create global variable for theme init"
Inline comments must start with a capital letter
Inline comments must end in full-stops, exclamation marks, or question marks
Wouldn't this be better a boolean?
Expected 1 space before "="; 0 found
Expected 1 space after "="; 0 found
I propose:
Comment #22
srclarkx CreditAttribution: srclarkx commenteddas-peter,
I made most of the changes you asked, added comments and made the global a boolean. I couldn't get it to pass if I replaced the isset($drupal_theme_registry) with the empty($GLOBALS['drupal_theme_init']) in the theme_get_registry function. It doesn't like the return value in the conditional. I probably won't be able to get back to this for a while.
Comment #23
das-peter CreditAttribution: das-peter commented@srclarkx Awesome, thanks! Hope I can test it soon :)
Comment #24
srclarkx CreditAttribution: srclarkx commentedThe initialization of the $drupal_theme_init in the _drupal_bootstrap_full function is causing problems. I'm going to see if I can get it to pass simpleTests without initializing that global variable. This is closer to what Smita submitted.
Comment #25
srclarkx CreditAttribution: srclarkx commentedI'm going to mark this as still needs work. The patch will cause other issues especially when adding files and images. I'll continue to work on it in my spare time.
Comment #26
srclarkx CreditAttribution: srclarkx commentedGuess I had a brain cramp, I have a bug in the last patch.
should be
I'm not submitting another patch because the patch breaks the Syntax Highlighter.
Comment #27
das-peter CreditAttribution: das-peter commentedHad time to investigate this further.
Created a test and overhauled the patch. Still not sure if this is the best / only approach.
Test-only patch should fail.
Comment #30
das-peter CreditAttribution: das-peter commentedAdjusted patch:
_drupal_maintenance_theme()
has to set the global variable as well.Comment #31
dawehnerI'm curious how many of those accidentally theme initialization before proper bootstrap level do we have.
Are there other examples than l() and t()? I'm curious whether we could explicitly fix them there, which means those places would never trigger the theme initialisation but
instead just fallback on the non-theme implementations?
Comment #32
mkalkbrenner@dawehner: I don't know if I understood you correctly.
I didn't count, but I opened a lot of issues for contrib modules to avoid l() and t() in early boot strap phases, which break ThemeKey. I even wrote a debugger to find these trouble causing modules.
From my point of view it's impossible to fix all this erroneous code and new one will be written as long as we only state in the documentation that it is not allowed to use function like l() or t() for example in hook_boot.
Therefor I prefer that workaround in core.
On the other hand, the solution I would really like to see is to modify l() and t() themselves. If they are called too early they should throw a hard error. That's the only way to get rid of all the erroneous code.
But as that would break existing installations until these contrib modules are fixed, it is also possible to just create watchdog entries to force people to fix their code and have a workaround like #30 in core to get rid of the problems these contrib modules are causing.
Comment #33
das-peter CreditAttribution: das-peter commentedI agree and also second that it is almost impossible to fix all the occurrences. Especially since
l()
ort()
are just the most obvious triggers here.As the one described in the issue summary there are much complexer scenarios which are almost impossible to fix.
I tried to change the approach to have something like
drupal_theme_initialize($reset = FALSE)
where_drupal_bootstrap_full()
then usesdrupal_theme_initialize(TRUE)
. However this failed miserably because_drupal_theme_initialize()
already adds css / js usingdrupal_add_css()
/drupal_add_js()
which makes a clean reset almost impossible.So I'm out of ideas for other less invasive approaches - but for now denying to initialize the theme stuff before a certain boot level seems the most unobtrusive way to handle this.
Comment #34
das-peter CreditAttribution: das-peter commentedChanged approach:
drupal_theme_initialize($final_init = FALSE)
and_drupal_theme_initialize($theme, $base_theme = array(), $registry_callback = '_theme_load_registry', $register_files = FALSE)
.$final_init
/$register_files
should only be set by_drupal_bootstrap_full()
or when there's no full bootstrap but output.I'd say this is a bit clearer than dealing with global variables. And the documentation is easier to find.
First tests look good - let's see what the test-bot says.
Comment #35
dawehnerThis patch seems to be a really good solution under the boundary condition of not changing behaviour of the system.
Comment #36
srclarkx CreditAttribution: srclarkx commentedWe have a large drupal site and this issue is a priority. We will do some testing using this patch today and give feedback.
Comment #37
srclarkx CreditAttribution: srclarkx commentedWe tested this and the theme issues seem to be fixed. If we don't see any issues in the next week, I'm going to call it good.
Comment #38
srclarkx CreditAttribution: srclarkx commentedI reviewed the patch and the comments. We patched our system and have been running the patch for close to 2 weeks.
We use custom theming on a per node basis. We were having problems with the theme coming up initially in the wrong theme. When we tried the initial patch we were getting more than 1 theme loaded on a page. Patch #34 works for us. Nodes seem to be loading with the correct theme consistently.
Comment #39
cilefen CreditAttribution: cilefen commentedI changed loose to lose in the issue summary.
Comment #40
cilefen CreditAttribution: cilefen commentedComment #41
jOksanen CreditAttribution: jOksanen at Realityloop for Monash University commentedThis fixed the custom_theme() issue for me.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis is some interesting work, and being able to switch themes mid-request is a really nice goal. Thank you for working on it. But I'm worried it's more complicated than this. What about other code out there that might cache information based on the current theme, without any expectation that the theme could change later on?
I'm tagging this issue for more review, since it seems like this is a really fundamental change to the theme system and it needs more careful review to see if it's possible to do in a stable release.
In terms of a quicker fix (that might fix the immediate problem but not the underlying cause) I agree with @dawehner in #31:
Note that in Drupal 7 t() should not initialize the theme system (at least not as far as I know), so that just leaves l(). And l() only needs to initialize the theme system in a very specific way, when a site has chosen to override the default theming of links. So it should be possible to change the code in l() to just fall back on the default theming whenever it's called too early. That would mean that if l() gets called too early those particular links might not be output the way the site expects them to be, but it seems like a relatively small price to pay especially since you really shouldn't be running too much complicated code before hook_init() to begin with... Drupal just isn't set up all the way yet then.
Comment #43
Alan D. CreditAttribution: Alan D. commentedIt's been a few years since I last looked at the code, but doesn't the menu system need to be fully executed to ensure the correct theme?
One random one in the mix is the Text module during entity load from a wildcard callback. This calls _text_sanitize() and the filters that are enabled could invoke a theme callback.
i.e. The Image resize filter was enabled on the body field. So text_field_load() called _text_sanitize() to check_markup() to theme('image_resize_filter_image').
Found this out debugging for #1870890: Use hook_domain_bootstrap_full() to set $conf['theme_default'] rather than hook_custom_theme().
Comment #44
Chris Burge CreditAttribution: Chris Burge commented#34 resolved the issue relating to hook_custom_theme() for me. In my particular case, I am using the OG Theme module, which makes use of hook_custom_theme(). After a site cache clear, the initial page load of a group node would result in the wrong theme being loaded due to hook_custom_theme() failing to execute as expected.
Comment #45
DamienMcKennaIt seems like #2483811: Call to l() in system_token_info() causes theme to be initialised early is a band-aid for this exact bug.
Comment #46
DamienMcKennaIt might be worth extending the test to verify that CSS or JS is only loaded from the active theme and not one of the others.
Comment #47
tnathanjames CreditAttribution: tnathanjames at Mediacurrent commentedI tried #34. It worked for me, but I really wanted to only process things differently in the special circumstance where the custom theme is set for the current page and a different theme was initialized before the call to drupal_theme_initialize in _drupal_bootstrap_full() of common.inc.
So, I am uploading a patch and interdiff to #34. This only changes current behavior in the circumstances previously mentioned. I also added a test for loading the correct theme if hook_custom_theme does not return a custom theme and updated the current tests to look for a file specific to the theme instead of just the theme name (just in case).
Comment #49
tnathanjames CreditAttribution: tnathanjames at Mediacurrent commentedCorrected patch - directly from git this time :)
Comment #51
DamienMcKennaComment #52
DamienMcKennaA correct interdiff for #49.
Comment #53
DamienMcKennaSorry, I bungled the interdiff, here it is again.
Comment #54
DamienMcKenna.. which makes my interdiff completely redundant. Sorry tnathanjames.
Comment #55
DamienMcKennaThis needs to be extended to cover additional scenarios:
Comment #56
DamienMcKennaFYI the themes won't enable in the correct order if there are other themes installed, so running CustomThemeBootTestCase locally may fail.
Comment #57
DamienMcKennaThis patch just changes how the themes are enabled to use theme_enable().
Comment #58
DamienMcKennaThis adds an extra set of tests after CSS and JS aggregation is enabled, and refactors the code a little bit.
Comment #59
jtwalters CreditAttribution: jtwalters commentedThis part seems like it's not doing anything:
Have not tested it though.
Comment #60
Alan D. CreditAttribution: Alan D. commentedThis is ensuring that the stored array is empty, the & bit in the first line is the reference to the stored value.
Comment #61
jtwalters CreditAttribution: jtwalters commentedThanks for clarifying. It just seems odd to reference an array index without first declaring the array, but I recall PHP allows that.
I tested out this patch and it works for my custom "hook_custom_theme" implementation. It was the only thing that worked for me.
Comment #62
jtwalters CreditAttribution: jtwalters commentedAlso, needed to drupal_static_reset a little more, interdiff is:
Comment #63
Fabianx CreditAttribution: Fabianx as a volunteer commented#59:
Can be written directly as:
to initialize to an array() when not set.
Comment #64
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedBtw. this issue is one of the reasons why I advocate to deprecate theme_link() (even if we can't remove it) and urge strongly to use theme_link = FALSE in $conf for all our sites.
It also means we really should use an alter() hook for links to avoid early instantiation when not needed. (#2562731: Add hook_link_alter)
The patch in general looks good to me, but another idea would be:
- When the theme is not yet negotiated, do not initialize it.
- Ensure that theme_link static in l() is not populated till BOOTSTRAP is completed.
Comment #65
davidburnsThis fixed my issue where ThemeKey would load multiple themes clearing cache or re-saving a node! Thank you.
Comment #66
KeyboardCowboyI like @Fabianx's recommendation of not initializing the theme while in another routine of the bootstrap phase. I'm not sure if this is more performant than the solution in #58, but it seems simple enough and it works.
Comment #67
Rudi Teschner CreditAttribution: Rudi Teschner commentedI've had trouble with loading the default theme after a cache clear and patch #66 solves the issue for me.
Comment #70
joseph.olstadPlease do not be alarmed by "composer Command Failed" - this is not an indication of a problem with the patch.
It occurs since approximately may 4th, there is a known testrunner issue. @mixologic (responsible for the testrunner) is on vacation
#2970950: D7 test runner not working since may 4th 2018 'Composer command failed'
Hopefully @mixologic gets back from his vacation soon.
Comment #71
DamienMcKennaSomewhat related: #1934192