Block regions not initiated for newly enabled theme, so theme unusable

I can't reproduce the reported issue with a clean install.

Are the themes causing these problems "updated" (are they phptemplate-based or, if not, do they have _regions() functions)?

The specific problem seems to be that the region field isn't being set when block regions are initiated. Here's the relevant portion of function system_initialize_theme_blocks():

      // If the region isn't supported by the theme, assign the block to the theme's default region.
      if (!array_key_exists($block['region'], $regions)) {
        $block['region'] = system_default_region($theme);
      }

This is executed if a newly-enabled theme doesn't have blocks already assigned to it. It gets the default region's settings--except the region. This code portion tests whether the default region's region is available in the newly-enabled theme, and, if not, substitutes the theme's default region. This would all fail if the theme wasn't returning any regions--but any phptemplate theme, or the core ones, should be....

Anyone see what's going wrong?

I'm still not quite sure what the issue is, but clearly better error handling is needed. A particular issue is the scenario where a site admin upgrades a Drupal install without removing non-upgraded themes, or, on a new install, tries to install non-upgraded themes. As things stand, either of these could lead to unusable themes being the default, and all blocks disappearing.

The attached (initial, untested) patch adds error handling to both updates.inc and system.module.

New for allinstalls:

* theme region initiation return FALSE on failure TRUE on success.
* on initiation failure, theme is disabled, and, if it was set to default theme, bluemarine is substituted, with message given

Updating:

* tests if current default theme has been upgraded, and if not sets bluemarine as the default, issuing a notice
* tries to initialize blocks for each enabled theme and, on failure, disables the theme.

This should help prevent upgrade problems. If anyone has a chance to review and test this, I'd appreciate it, as I don't at present have a test install to upgrade.

This bug came from a recent change to function list_themes(), which now lists only enabled themes. function system_listing_save() fetched available regions (using, indirectly, list_themes()) *before* a theme was enabled.

As well as the error handling (discussed above), designed to prevent non-upgraded themes from being enabled and to give meaningful error messages, the attached patch moves theme region initiation to after themes are enabled. On my testing, this fixes the reported error. Please test and report back.

I haven't yet tested the error handling changes to updates.inc. Anyone able to do that? To test it, try to upgrade a site that has some default (and therefore, with an upgrade, updated) themes, and some themes (already enabled) that are not upgraded. The expected behaviour is that the upgraded themes have their regions initiated, while any non-upgraded themes fail, are disabled (i.e., have their status reset to 0), with messages given.

Theme initiation was still broken for styles (unless the theme they are based on was already initialized). This is a perhaps unintended side-effect of limiting list_themes() to enabled themes--data for a theme on which a style is based are not available if that theme is not enabled.

I've changed the code to load theme data directly rather than through list_themes(), so it's now I believe working. (Also fixed a minor bug in error reporting.)

unfortunatly your patch isn't conform the drupal guidelines.

Well, okay. I'm just following what updates.inc already does, see function update_sql(), so that messaging is consistent. Perhaps you'd like to submit a patch to fix the allcaps issue in updates.inc. I'm not going to do that now, as my aim is to get this particular (critical) problem fixed.

I think there's some confusion here. The portion of my patch in question *is* to update.inc--not "the running part of Drupal".

Thanks for the tips, though they kinda miss the point.

My inclusion of update enhancements is evidently distracting from the main issue--so I've deleted it from the patch. There is, in any case, no bug or broken code to fix in the past update.

The issue here is that the "list_theme() improvements" patch has broken theme block initialization--before anyone had a chance to see the new region functionality in action. The attached patch fixes this. Please review and apply ASAP.

Aside: my manual messaging in the updates was an admittedly crude workaround to two limitations: (1) update_sql doesn't allow additional arguments (i.e., string or number query parameters) beyond the sql, and (2) since updates may do more than simply issue queries, there's a need for more flexible reporting. I agree that drupal_set_message() *could* be used for this, but I think it makes more sense to have flexible inline messages. So I've roughed in a patch to do that (and also address some of stefan's requests), see http://drupal.org/node/29002

Thanks for the tweaks, Stefan.

Can you please explain why something simple such as my attached patch is not enough?--Thox

It's a good question. My basic reasoning is that we shouldn't make upgrading themes optional, and shouldn't support non-complying themes. It's better to require upgrades, as we do for everything else, because failing to do so encourages lax coding and uneven support.

Also, because the suggested workaround doesn't fix the bug, all themes would return simply left and right regions--and we'd be right back where we started!

Overall, I'm thinking now I was mistaken to include error handling in this patch. Whether to allow non-complying code to be enabled is a worthy but separate issue. With modules, for example, we don't evaluate whether they work before enabling them--we rely on errors and the good sense of site admins to remove a non-complying module. I was thinking, "Themes are different, since setting a broken/nonupgraded theme as default could make a site unusable, since menus disappear, and novices don't know the right url to use to reset the theme to a supported one." But my approach was awkward and limited (e.g., it tested only one part of required theme functionality before disabling). If we're going to test for compliance before enabling (and I'm not convinced we should), we need to do so in a full, even way--which is to say, not in this patch! (The best, easiest thing would be simply to introduce a way for modules and themes to advertise their version support, as has been suggested and I believe begun elsewhere.)

Updates *should* include better error handling; so I'll try to get in a separate update patch before the 4.7 code freeze.

At any rate, the key point is to fix the bug introduced by issue #29002. I'll post a "bug-fix only" patch as soon as I can.

Okay, here's a stripped down fix. This should be ready to apply--but I'll wait for someone else to affirm that before setting the status!

(Aside: I gave an incorrect link, above, to the issue I created to improve update_sql() and messaging. The issue is actually at: http://drupal.org/node/29701).