In D7, we started using t() in our error handler. D6 did not do this. Unfortunately, t() uses theme('placeholder') which in turn initializes the theme system if it hasn't initialized already. It is quite possible to trigger a handled error before all the modules are loaded. When this happens, we rebuild the theme registry (if needed) and get quite a tiny, useless theme registry in our cache. This obviously kills the presentation of all future page views served with this theme registry.
I stumbled into this with drush, which triggers an expected 'already sent headers' warning when we emit our http header during full bootstrap. The attached patch removes t() from drupal_error_handler().
Comment | File | Size | Author |
---|---|---|---|
#74 | theme_registry.patch | 1.64 KB | catch |
#73 | theme_registry.patch | 1.43 KB | catch |
#64 | theme-reg-592008-64.patch | 4.42 KB | David_Rothstein |
#59 | theme_reg.diff | 3.71 KB | moshe weitzman |
#57 | theme_reg.diff | 3.71 KB | moshe weitzman |
Comments
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedThis one goes more to the heart of the problem. Don't ever save the theme registry until we loaded all modules.
Comment #3
chx CreditAttribution: chx commentedInstead of a separate function what about adding memory to module_load_all? http://drupalbin.com/11638
Comment #4
chx CreditAttribution: chx commentedOr instead you want a static $full and flip it to TRUE when $bootstrap is FALSE and return that. Works as well and returns less weird.
Comment #5
Dries CreditAttribution: Dries commentedCertainly needs code comments, regardless of the solution we chose to go with.
I'm in favor of #2 as it provides a re-usable API.
Should it be when all modules are loaded or when Drupal is fully bootstrapped?
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedI'm in favor of #2 as well. I added some comments and rerolled.
For most purposes, module load complete is all that matters. Those are the files which can implement hooks and provide registry like info. I'm not opposed to checking for bootstrap complete, but I don't see much benefit either.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedchx prefers to keep all the logic in module_load_all() so here is an alternative that does that.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedoops - wrong patch.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedI think Dries can now pick between #6 and #8.
Edit: Actually, if you want #6, can you replace system_menu with node_menu? We do an explicit load of system.module in installer so we could get false positive.
Comment #10
Dries CreditAttribution: Dries commentedThe delta between all modules being loaded and the end of the full-bootstrap is pretty small. The only thing that happens in between is (i) setting a custom theme if any and (ii) calling hook_init(). If we don't want to introduce a new function, we could also use
(drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL)
.Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedThats excellent. Patch attached. I added a little doxy to drupal_bootstrap().
Comment #12
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedChange title for posterity
Comment #14
chx CreditAttribution: chx commentedSo now using theme() in hook_init is a performance blow?
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedHmmm. Thats true. Maybe we should go back to a prior iteration. Or move hook_init() to index.php, right before execute_active_handler()
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedOK, back to #6, using node_menu instead of system_menu.
Comment #17
chx CreditAttribution: chx commentedI still do not get why we do not want to ask module_load_all whether it loaded all but if we do not want this is a lot better.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedI chose not to do that because it is a small WTF to have a function return a boolean in some cases and in other cases, load files and then return nothing. True, we are choosing between small WTF here. Thanks for the RTBC.
Comment #19
sun1) Function summary needs to be on 1 line.
2) @return should come before @see
3) I agree with chx - I just need a require_once modules/node/node.module to break this.
Comment #20
sunmoshe mentioned an earlier patch as alternative:
a) This would work, if $has_run is only set when $bootstrap is FALSE. Otherwise, we still don't have the full set of modules.
b) I wonder whether using a drupal_static() here would be acceptable.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedOK, Back to the chx approach. Not using drupal_static as there is no benefit. You can't uninclude files in php.
Comment #22
sunuhm, module_load_all() now seems to have a completely broken indentation. :-/
I know that code can't be unloaded. I specifically questioned the drupal_static() to perhaps decrease the WTF-factor:
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedfixed indentation
thats another option, with other WTF. Sticking with #21 approach for now.
Comment #24
chx CreditAttribution: chx commentedHm, lots of whitespace errors and lots of unnecessary code too.
Comment #25
sunMuch better now. :)
Comment #26
Dries CreditAttribution: Dries commentedmodule_load_all(NULL) is not documented? specifically having to pass NULL seems like a WTF to me.
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedIt is documented. See the @return for module_load_all(). We could restate this if anyone has ideas to improve clarity.
And yes, it is a small WTF. We have tried all the various permutations here. They all have small WTF to them. IMO whats needed to fix this properly is to add a small drupal_set_page_context() and drupal_get_page_context() or just rename the drupal_static system like that. Then I would do a drupal_set_page_context('module_loaded', TRUE) from within module_load_all() and then check drupal_get_page_context('module_loaded') when saving theme registry. This is similar to tha_sun's suggestion. If Dries wants that, I am happy to provide it immediately.
Otherwise, please it would be ideal if Dries' provides some direction. The alternative approach is #16.
Comment #28
sunThis patch is required for #591794: Allow themes to alter forms and page
Comment #29
catchWe have a choice here, nothing specifically needing work.
Comment #30
Dries CreditAttribution: Dries commentedIn #14, @chx wrote "So now using theme() in hook_init is a performance blow?" but unlike him and Moshe, I don't see how that is the case. The big thing the previous patch did, was conditionally store the registry. I'd think that 99% of the time, _theme_load_registry() would succeed in getting a copy of the registry from the database cache. @chx seems to suggest, but I could be wrong given the briefness of his comment in #14, that loading the registry from the cache might result in many cache misses.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commented@Dries is right. We are really only fixing the 1% of the time when there is no cached registry. In that case, every theme() call during hook_init() will rebuild the full theme registry. I think thats a problem worth solving, #24 is our best effort IMO.
Comment #32
Dries CreditAttribution: Dries commentedThe trade-off is: do we want to fix a rare performance regression at the cost of making our API more ugly. Performance vs DX.
For me, the big question is: is it 1% or is it 0.01% or is it 5%? It feels like it is closer to 0.01% than 1% so my personal preference would be to suck up the small performance regression in favor of a cleaner API. That said, I haven't benchmark the miss/hit rate of the theme registry cache.
Comment #33
alpritt CreditAttribution: alpritt commentedaha, please read especially comment #5 and #7 from #600422: WSOD if theme() called before bootstrap completes.
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commentedAll true. On most sites it will be .01% of requests. The problem is that we still cache_clear_all() rather frequently, and that includes the theme registry. So when this does happen, it happens at the very worst time. At least theme registry rebuild taxes the web server and not the DB server ... More granular cache clearing has to be an important goal for D8.
I propose that we use the term 'DX' to refer to ugliness in our APIs. The patch here is isolated to Drupal guts. No contrib developer would ever care. So what we have here is code ugliness, not DX. I think there is a difference.
IMO, we should fix this before release, but it isn't the most important issue right now.
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedRunning theme() in hook_init() is not supported *at all* (it triggers the initialization of the theme system, which breaks about everything from the administration theme to the block configuration page). Fixing that in not in the scope of this issue.
Comment #36
moshe weitzman CreditAttribution: moshe weitzman commentedWell, if that is so then theme() needs to give a fatal error if you try. We can't have these secret traps.
Comment #37
Damien Tournoud CreditAttribution: Damien Tournoud commented@Moshe: we have had that secret trap for years now. I agree that's not nice, but this is really not in the scope of this issue.
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedThe whole point of inventing hook_boot in D6 was so that developers could know that hook_init() is working with a fully bootstrapped drupal and can do whatever it wants. The secret trap is a bug. This patch is one step toward fixing that bug and keeping up performance during cache flushes. I disagree with marking this fixed because other bugs exist. We won't make good progress that way.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedTwo spin-off patches inspired by this one:
Comment #40
sunI must really wonder since when we simply ignore race conditions like this, especially, since we already know that the introduced helper function here is required for another issue. (#591794: Allow themes to alter forms and page)
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe previous status change was a cross-post, sorry about that. I think the correct status at this point is CNR.
Comment #42
mattyoung CreditAttribution: mattyoung commented#35
>Running theme() in hook_init() is not supported *at all* (it triggers the initialization of the theme system, which breaks about everything from the administration theme to the block configuration page). Fixing that in not in the scope of this issue.
also breaks maintenance page theming: http://drupal.org/node/374645#comment-1262479
the theme system cannot be touched in hook_init() and a whole lot of things can indirectly cause the theme system to be init'ed like getting a form, calling t(). A lot of people get blank maintenance page and it's always some module's hook_init() touching the theme system some how.
This is really bad and there is no warning of this in the API doc: http://api.drupal.org/api/function/hook_init/6
init_theme() should call drupal_get_bootstrap_phase() and blow itself up if it's before hook_init()!
#405578: Need to prohibit hook_init() from initializing theme system
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedThis is no longer true in Drupal 7, due to #553944: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed. Running theme() in hook_init() works perfectly well... Or rather, it works perfectly well when the patch in #24 is applied, because otherwise, this issue causes a WSOD in those cases :)
I just spent quite a while tracking that down while working on a module for Drupal 7... because it's not even a "real" WSOD, really just literally a blank page.
I agree with Moshe - it is reasonable to tell people they can't do certain things in hook_boot(), but not hook_init() - people should be able to do whatever they want there. We should fix that here, and probably add a test to make sure that this bug doesn't crop up again.
Comment #44
moshe weitzman CreditAttribution: moshe weitzman commentedBack to RTBC for #24. We have no perfect solution here. I replied to Dries' #32 in #34.
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedsubscribing
Comment #46
webchickOk, moshe's response seems reasonable. Agreed that this is an internal change, rather than a developer-facing one, and that being able to use the full range of API functions in hook_init() is expected behaviour in terms of DX.
Committed #24 to HEAD.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedAs per above, we really need a test for this... The attached test correctly fails when the patch in #24 is reverse applied, but passes with the current D7 codebase.
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedActually, this fix is still incomplete. Now we don't persist the theme registry which is good but we don't handle the theme call that kicked off this badness. It just fails since there is no theme registry. We need to pass back our temporary registry to theme(). I don't think David's test is quite right. Let me see if I can get it to fail and then we can fix the bug for sure.
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedJust want to give another plug for #620452: Optimize drupal_static(): we shouldn't sacrifice simpletest coverage for performance and vice versa, which would let us keep the registry in a resettable static and let theme() access it quickly, instead of maintaining its own copy that needs resetting.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedWell, if someone is calling theme() that early - for example, in hook_boot() - we want it to fail somehow, I think, because otherwise the theme system gets initialized too early and we aren't even necessarily using the correct theme for that page.
However, I guess the current situation is a little weird, where in those cases it returns blank page output (rather than throwing an exception or something) on page requests where it is trying to populate the registry, but doesn't cause any direct error (?) on other page requests.
Comment #51
sun.core CreditAttribution: sun.core commented#47: theme-module-bootstrap-test-592008-47.patch queued for re-testing.
Comment #52
aspilicious CreditAttribution: aspilicious commentedIt's green: http://qa.drupal.org/pifr/test/16095
Comment #53
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, the patch thats pending here is just adding a test. There is a critical bug described in #48 thats needs resolving
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commentedLets attack this languishing critical.
Here is a simple patch that rejects early theme() calls. Lets see what bot thinks.
Comment #56
David_Rothstein CreditAttribution: David_Rothstein commentedSeems pretty reasonable. Shouldn't we include something like my test from #47 in here? That's complementary since it tests that the theme system does work immediately after this stage - and which definitely didn't work correctly in Drupal 6.
Note that Syslog module has some weird issues with theme() getting called too early in the page request; it has been nicely filling up the error logs in Drupal Gardens. See: #319844-17: Watchdog should work, even if the error is too early, before module.inc is loaded. It shouldn't affect this patch one way or the other since in that case theme.inc isn't even loaded yet - just thought it was an interesting related point to mention :)
Comment #57
moshe weitzman CreditAttribution: moshe weitzman commentedHere it is, with David's test (slightly updated since theme_placeholder was removed).
Comment #58
catchPatch looks sane enough to me, minor whitespace issue:
Comment #59
moshe weitzman CreditAttribution: moshe weitzman commentedWithout extra line
Comment #60
catchPatch looks good, thanks for putting the module_load_all() check before the !defined(), comes with tests, rtbc.
Comment #61
webchickCould we just get a little one-or-two-liner comment above that new theme() chunk to explain how that can happen? I feel like there's information buried in this issue that isn't making its way to the code. (Also module_load_all(NULL) makes you go ???)
Comment #62
catchmodule_load_all(NULL) is not invented here. I'm not sure what the conditions are which trigger this any more, after t() being fixed, so will leave that to those who are.
Comment #63
webchickRight, no, I realize that. I just meant a comment would help soften the WTF blow. ;)
Comment #64
David_Rothstein CreditAttribution: David_Rothstein commentedHere's an attempt at a code comment. I'm not sure we need to identify a specific scenario where this can happen (any time before all modules are loaded can encompass a lot of things in theory) but I did try to add a comment explaining why we need to bail out in this case.
Comment #65
catchWorks for me.
Comment #66
Dries CreditAttribution: Dries commentedI looked at the test, and it looks a bit weird -- it doesn't seem to test the bug. Instead, it seems to test hook-init?
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedYes, we are testing the fact that the theme system actually does work at one of the earliest points in the bootstrap cycle that we expect it to. That didn't work in Drupal 6, and didn't work for a while in Drupal 7 - but was allowed to start working when one of the previous patches in this issue was committed (and possibly it was also one of the patches before that in this issue that had broken it for D7 - can't remember). We need that test :)
I guess that in addition there could be a test to ensure that the theme system doesn't work earlier than that - i.e. that calling theme() in hook_boot() triggers an exception and doesn't result in the theme registry being populated. Perhaps not as important to test, but wouldn't hurt I suppose.
Comment #68
Dries CreditAttribution: Dries commentedI like the bugfix but I'm not sure about the extra tests. It is not clear we actually need them -- this might well be tested already through other code. I'm going to commit the bugfix, and then maybe we can create a new issue for the tests.
Comment #69
Dries CreditAttribution: Dries commentedThanks all. I committed the fix but not the tests. Let's move the test to a new issue -- they are not testing the bug to begin with.
Comment #70
David_Rothstein CreditAttribution: David_Rothstein commentedDries, no, the point is they are testing the bug that one of the earlier patches fixed. See #47. And I doubt we have test coverage for this, because if we did, the patches that kept breaking it would not have been committed :) This has broken more times than I can count.
#764094: Add tests to check that the theme system works in hook_init (i.e., as early as possible)
Since I moved the patch there, pretty sure there's nothing more to do here, so marking it fixed.
Comment #72
Damien Tournoud CreditAttribution: Damien Tournoud commentedI suggest we backport this. The fact that an incomplete theme registry can be saved just caused me some grief.
Comment #73
catchI just debugged this on a D6 site. When this happens it renders the entire site completely blank.
Here's an untested backport of the original patch for D6 (not 100% direct backport but close). Adds an optional argument to module_load_all() which is different to the optional argument in D7, can't do a lot about that I think.
Comment #74
catchSlightly better version - prevents static caching of the invalid registry too.