Themes cannot always participate in drupal_alter()

1) We need actual data for when/how often/in what requests themes are not initialized at all currently, so as to be able to evaluate the performance impact of always initializing themes early.

2) I originally stated the idea of loading vs. initializing, but I'm actually not sure whether that's possible or whether there's a real difference at all? If this is possible, then it sounds like the right solution attempt to me, since just loading 1-2 files more (template.php + base theme) in every request shouldn't have a large performance impact.

3) I wouldn't support rolling back the entire feature. Sounds like turning around after walking to 99%.

Either way, this will likely result in an API change.

Oye. So basically we have a phantom alter API in the theme. Even worse, its an unpredictable phantom of mysterious habits.

How many strange alter functions are there between bootstrap and return $output;? Would it maybe make sense to start by verifying what's supposed to run figuring out test cases for them?

So... http://api.drupal.org/api/function/drupal_theme_initialize/7 just spotchecking this function i gotta admit that i feel like something smells fishy. Why is it being called in so many places? Would it be to safe to say that finding it in places like the l() function is related to this issue?

Sorry about the glitch sun (i didn't change that title lol) And you'll have to add back the API change tag - my comment totally ate it.

subscribe.

subscribe

Yeah, I agree with adding a $load_registry param and with you swatting away any collateral damage :). I actually think there won't be any damage. Seems like you just have take your patch in #591794-94: Allow themes to alter forms and page and pass FALSE in both calls to drupal_theme_initialize().

Based on the above data, it takes 2.4ms just to retrieve and unserialize the theme registry (22.1-19.7). Thats a problem that gets much worse as you add more modules (i.e. more theme hooks). Lets see what we can do here for D8.

-moshe

This is perfect, IMO. Please wait for green before commit.

I concur with Moshe. And its green btw.

#9: theme-intialize-early-812016-9.patch queued for re-testing.

#9: theme-intialize-early-812016-9.patch queued for re-testing.

#9: theme-intialize-early-812016-9.patch queued for re-testing.

Back to rtbc by #10 and #11

#9: theme-intialize-early-812016-9.patch queued for re-testing.

#9: theme-intialize-early-812016-9.patch queued for re-testing.

#9: theme-intialize-early-812016-9.patch queued for re-testing.

#9 = green ... #10 = RTBC (moshe) ... #20 = still green

#9: theme-intialize-early-812016-9.patch queued for re-testing.

Thanks for the nice issue summary, and for figuring out a way to fix it without an API change. :)

+++ includes/theme.inc	29 May 2010 02:04:59 -0000
@@ -68,11 +68,20 @@ function _drupal_theme_access($theme) {
+function drupal_theme_initialize($load_registry = TRUE) {

We need to document this new parameter.

And I really, really would love a test for this. Maybe hitting an autocomplete URL and ensuring that the theme system hasn't fully initialized, somehow?

Powered by Dreditor.

Your wish is my command.

Oh. You wanted a comment too. Sure.

What about something like this instead?

I don't understand why we have a push system here, instead of a pull one. This whole area should be refactored, but this is a minimal fix.

That's a tricky way to do it but no public API changed at all so that's even better! Private functions can change at will, no probs.

To recap, Damien's patch stores the registry callback in theme_get_registry and makes it the registry load on demand. So it's only actually calling theme_get_registry that loads the registry and that happens on the first theme() call. Previously, on theme initialize we fetched the registry and pushed it into theme_get_registry. Edit: so instead of pushing the whole registry we only push the registry callback and because of that a private API change happens, the registry pusher is gone and we introduce a registry callback pusher. All these pushers, "I got things you won't believe" :p

I think this makes sense. And I guess if there are any fall-out from moving theme initialization earlier, testbot would catch it.

However, this no longer applies. Can I get a re-roll, please?

Here is a re-roll.

+++ modules/simpletest/tests/theme_test.module	5 Aug 2010 14:42:02 -0000
@@ -24,15 +24,36 @@ function theme_test_menu() {
+    $GLOBALS['theme_test_output'] = theme('more_link', array('url' => url('user'), 'title' => 'Themed output generated in hook_init()'));

Does this line need to be array('url' => 'user') it seems that I may have missed that change. Here is the changed patch just in case.

Powered by Dreditor.

Although I can't properly review the patch, I'm so relieved to find this issue and just want to say THANK YOU to everyone working on it! I was going insane trying to figure out the proper way to change the formatter of a taxonomy term field (from link to plain text), thinking this hook_field_display_alter() was one of the alter hooks that wasn't permitted for themes. Phew. :)

With this patch applied, update.php uses the default theme instead of the admin theme like it's supposed to.

@webkenny, I, and some others just looked into this a bit.

It looks like update.php does this:

drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
drupal_maintenance_theme();

and then drupal_maintenance_theme() refuses to set the theme to the maintenance theme because with the current patch the theme has already been set in the bootstrap - that is probably the cause of the bug Jacine found.

This is sort of due to drupal_maintenance_theme() using global variables to set the theme (rather than the new methods in D7), but given how much of a special case maintenance pages are that might not be easy to fix....

Another option would be to change how the drupal_theme_initialize() call is added by this patch. Currently the bootstrap does this:

  // Set a custom theme for the current page, if there is one. We need to run
  // this before invoking hook_init(), since any modules which initialize the
  // theme system will prevent a custom theme from being correctly set later.
  menu_set_custom_theme();
  // Let all modules take action before menu system handles the request
  // We do not want this while running update.php.
  if (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') {
    module_invoke_all('init');
  }

The patch here adds drupal_theme_initialize() after the menu_set_custom_theme() call but outside the if() statement. Maybe it should be in the if() statement as well, for the same reason the module_invoke_all('init') call is? Not sure if there are any bad side effects to that.

Either way, I also noticed that the code comment above (the part about "... since any modules which initialize the theme system...") is out of date as a result of this patch, since we aren't relying on modules to initialize the theme system but rather are now explicitly doing it ourselves.

I just (happily) tested the patch in #34 with a few themes and all appears to be working well! :)

yay, thanks!

For D8, we should make the maintenance mode context set a custom theme, just like any other context.

Looks good - a minor point is that menu_set_custom_theme() already has an internal check for MAINTENANCE_MODE, so putting menu_set_custom_theme() inside the if() statement with the rest of them isn't necessary, and just leads to a duplicate check.

However, for the purposes of code readability, it is probably reasonable to move it, since the other theme stuff has to be there.

Alright. Committed to CVS HEAD.