Don't save theme registry before modules are included

Instead of a separate function what about adding memory to module_load_all? http://drupalbin.com/11638

Or 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.

Certainly 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?

The 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).

Committed to CVS HEAD.

So now using theme() in hook_init is a performance blow?

I 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.

 /**
+ * Determines whether we have passed the module_load_all() stage of full 
+ * bootstrap.
+ * 
+ * @see _theme_load_registry()
+ *
+ * @return boolean
+ */

1) Function summary needs to be on 1 line.

2) @return should come before @see

+function module_is_loaded_all() {
+  // Look for an arbitrary function within a required module.
+  return function_exists('node_menu');
+}

3) I agree with chx - I just need a require_once modules/node/node.module to break this.

moshe mentioned an earlier patch as alternative:

 function module_load_all($bootstrap = FALSE) {
-  foreach (module_list(TRUE, $bootstrap) as $module) {
-    drupal_load('module', $module);
+  static $has_run = FALSE;
+  
+  if (is_null($bootstrap)) {
+    return $has_run;
+  }
+  else {
+    $has_run = TRUE;
+    foreach (module_list(TRUE, $bootstrap) as $module) {
+      drupal_load('module', $module);
+    }
   }
 }

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.

uhm, 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:

function module_load_all($bootstrap = FALSE) {
  $has_run = drupal_static(__FUNCTION__, FALSE);
...
function _theme_load_registry() {
...
   if (drupal_static('module_load_all', FALSE)) {

Hm, lots of whitespace errors and lots of unnecessary code too.

Much better now. :)

module_load_all(NULL) is not documented? specifically having to pass NULL seems like a WTF to me.

This patch is required for #591794: Allow themes to alter forms and page

We have a choice here, nothing specifically needing work.

In #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.

The 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.

aha, please read especially comment #5 and #7 from #600422: WSOD if theme() called before bootstrap completes.

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.

@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.

Two spin-off patches inspired by this one:

• #601548: Loosen the dependency between t() and the theming layer
• #318636: Make l() themable

I 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)

The previous status change was a cross-post, sorry about that. I think the correct status at this point is CNR.

#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

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.

This 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.

subscribing

Ok, 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.

As 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.

Just 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.

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.

Well, 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.

#47: theme-module-bootstrap-test-592008-47.patch queued for re-testing.

It's green: http://qa.drupal.org/pifr/test/16095

Seems 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 :)

Patch looks sane enough to me, minor whitespace issue:

+}
+
+

Patch looks good, thanks for putting the module_load_all() check before the !defined(), comes with tests, rtbc.

Could 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 ???)

module_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.

Right, no, I realize that. I just meant a comment would help soften the WTF blow. ;)

Here'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.

Works for me.

I 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?

Yes, 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.

I 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.

Thanks 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.

Dries, 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.

I suggest we backport this. The fact that an incomplete theme registry can be saved just caused me some grief.

I 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.

Slightly better version - prevents static caching of the invalid registry too.