[drupal-devel] [bug] Block regions not initiated for newly enabled
theme, so theme unusable
Dries
drupal-devel at drupal.org
Sun Aug 28 06:40:21 UTC 2005
Issue status update for
http://drupal.org/node/29506
Post a follow up:
http://drupal.org/project/comments/add/29506
Project: Drupal
Version: cvs
Component: system.module
Category: bug reports
Priority: critical
Assigned to: nedjo
Reported by: nedjo
Updated by: Dries
Status: patch (code needs work)
There is something wrong with the PHPdoc of
system_initialize_theme_blocks():
+ * particular region isn't available in the new theme, the block is
assigned
+ * instead to the new theme's default region.
(I can fix that for you when I commit the patch.)
Dries
Previous comments:
------------------------------------------------------------------------
Wed, 24 Aug 2005 03:10:30 +0000 : nedjo
Following the introduction of extentendable regions, see this issue [1],
several users have reported problems:
"A fresh install of Drupal with bluemarine displays blocks correctly,
but when I switch the default theme to pushbutton, all the blocks
disappear. They do show up in ?q=admin/block, but just with the yellow
helper boxes, no blocks are really displayed.--Goba
"
"Same problem as Goba.
The "region" field in the blocks table is sometimes blank. This
makes many themes almost unusable in a Drupal default install. I don't
know how the fields are blank, but the following SQL helped me get back
to theme development:
UPDATE blocks SET region="left" WHERE region=""--Thox
"
"another confirmation. I found out about the exact same sql query as
Thox to fix it:)
We came across this in an update. The theme had the regions defined,
but since it was a non-default theme the blocks dissapeared. A drupal
site, without a navigation block is simply useless.--Ber
"
The issue suggests a bug in the initiation of theme blocks. When a new
theme is enabled, it's supposed to be initiated with blocks based on the
default theme's blocks. I'll post a fix ASAP.
[1] http://www.drupal.org/node/16216
------------------------------------------------------------------------
Wed, 24 Aug 2005 04:11:57 +0000 : nedjo
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():
<?php
// 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?
------------------------------------------------------------------------
Wed, 24 Aug 2005 18:02:44 +0000 : nedjo
Attachment: http://drupal.org/files/issues/theme-region-error-catching.patch (5.73 KB)
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.
------------------------------------------------------------------------
Thu, 25 Aug 2005 14:07:38 +0000 : Thox
The problem occurs with all of the core themes except Bluemarine. That
means .theme (chameleon) and phptemplate (pushbutton) themes are both
affected equally.
------------------------------------------------------------------------
Thu, 25 Aug 2005 15:33:37 +0000 : Thox
When I enable a theme, it goes through system_initialize_theme_blocks(),
which assigns the system_default_region() to the new theme.
Unfortunately, that's blank.
There's no themes loaded (disabled by system_listing_save()), so
there's no regions available, so there's no default region to use.
Your patch stops old themes from working, can we not simply set the
default region (if not found) to "left"? This works fine for me to fix
this problem.
------------------------------------------------------------------------
Thu, 25 Aug 2005 16:26:03 +0000 : nedjo
Attachment: http://drupal.org/files/issues/theme-region.patch (6.2 KB)
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.
------------------------------------------------------------------------
Thu, 25 Aug 2005 17:57:34 +0000 : nedjo
Attachment: http://drupal.org/files/issues/theme_region2.patch (8.02 KB)
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.)
------------------------------------------------------------------------
Thu, 25 Aug 2005 18:41:37 +0000 : stefan nagtegaal
Nedjo,
unfortunatly your patch isn't conform the drupal guidelines.
See some comments here:
<?php
+ if ($result) {
+ $ret[] = array('1', check_plain("UPDATE {blocks} SET theme = '$default_theme'") ."\n<span class=\"success\">OK</span>\n");
+ }
?>
Why is this?? If this is _really_ needed, please use something like:
<?php
+ if ($result) {
+ $ret[] = array('1', check_plain("UPDATE {blocks} SET theme = '$default_theme'") ."\n<span class=\"success\">". t("Success") ."</span>\n");
+ }
?>
Secondly:
<?php
+ $ret[] = array(0, t('The theme %theme failed to initialize its blocks and so has been disabled. The theme may need to be upgraded.' . "\n<span class=\"failure\">FAILED</span>\n", array('%theme' => $theme)));
?>
Should be something like:
<?php
+ $ret[] = array(0, t('The theme %theme failed to initialize its blocks and so has been disabled. The theme may need to be updated.', array('%theme' => theme('placeholder', $theme)) . "\n<span class=\"failed\">". t('Failed') ."</span>\n"));
?>
The difference is "Failed" instead of "FAILED". We do not use UPPERCASE
errors. And I think if we use sentences as "Failed", we should use
"Success" either instead of "OK"..
Then you didn't made these messages translatable, and lacked the use of
theme('placeholder', $text).
Same here:
<?php
+ $ret[] = array(1, t('Blocks initiated for %theme theme.') . "\n<span class=\"success\">OK</span>\n", array('%theme' => $theme)));
?>
Do it like this:
<?php
+ $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n<span class=\"success\">". t('Success') ."</span>\n"));
?>
So, in short:
- Try to minimise HTML in user translatable strings;
- Don't use CAPITAL ERRORS, we don't do that atm;
- Use theme('placeholder', $string) where needed;
Though, I did not test the functionality of the patch (yet)..
------------------------------------------------------------------------
Thu, 25 Aug 2005 19:22:20 +0000 : nedjo
"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.
------------------------------------------------------------------------
Thu, 25 Aug 2005 22:25:32 +0000 : chx
No, no, no! Fix core themes, write something to update.inc if need to be
and document changes. Never ever put update code in the running part of
Drupal.
------------------------------------------------------------------------
Thu, 25 Aug 2005 23:08:59 +0000 : nedjo
I think there's some confusion here. The portion of my patch in
question *is* to update.inc--not "the running part of Drupal".
------------------------------------------------------------------------
Thu, 25 Aug 2005 23:34:55 +0000 : chx
I stand corrected -- sorry. But please provide a new update_XXX function
instead of patching the current the one.
Stefan, in update.inc we use FAILED yes and I can't see placeholder
calls.
------------------------------------------------------------------------
Fri, 26 Aug 2005 00:01:36 +0000 : drumm
Updates should not make the return value themselves.
<?php
+ $result = db_query("UPDATE {blocks} SET theme = '%s'", $default_theme);
+ if ($result) {
+ $ret[] = array('1', check_plain("UPDATE {blocks} SET theme = '$default_theme'") ."\n<span class=\"success\">OK</span>\n");
+ }
+ else {
+ $ret[] = array('0', check_plain("UPDATE {blocks} SET theme = '$default_theme'") ."\n<span class=\"failure\">FAILED</span>\n");
+ }
?>
I can't see how this is different from using update_sql().
<?php
+ if (!system_initialize_theme_blocks($theme)) {
+ update_sql("UPDATE {system} SET status = 0, WHERE type = 'theme' AND name = '%s'", $theme);
+ $ret[] = array(0, t('The theme %theme failed to initialize its blocks and so has been disabled. The theme may need to be upgraded.' . "\n<span class=\"failure\">FAILED</span>\n", array('%theme' => $theme)));
+ }
+ else {
+ $ret[] = array(1, t('Blocks initiated for %theme theme.' . "\n<span class=\"success\">OK</span>\n", array('%theme' => $theme)));
+ }
?>
Use drupal_set_message(t(...), 'error') instead. Monor issue: sentences
should not have double spaces between them.
And another update should be written to patch up whatever the last
update left behind since many developers have the broken code and
should not be running the same update again.
------------------------------------------------------------------------
Fri, 26 Aug 2005 00:02:17 +0000 : drumm
Updating the status.
------------------------------------------------------------------------
Fri, 26 Aug 2005 05:24:31 +0000 : nedjo
Attachment: http://drupal.org/files/issues/theme-regions.patch (5.65 KB)
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
------------------------------------------------------------------------
Fri, 26 Aug 2005 08:34:38 +0000 : Thox
Attachment: http://drupal.org/files/issues/default-regions.patch (617 bytes)
Your patch works, but I don't see why it needs to be so complicated. Can
you please explain why something simple such as my attached patch is not
enough? Note that it would allow allow all of the old themes to work as
usual and fixes the troubles with new themes.
------------------------------------------------------------------------
Fri, 26 Aug 2005 09:23:47 +0000 : robertDouglass
Nedjo, I tried your most recent patch and it worked well. I haven't
compared approaches, but was happy to get my site up and running with
this *awsome* new feature. I had fun popping the login box into the
footer, or header or content area. This is a whole new dimension of
Drupal excitement.
------------------------------------------------------------------------
Fri, 26 Aug 2005 09:30:15 +0000 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/theme-regions_0.patch (5.66 KB)
Updated patch to use theme('placeholder', $string) and deleted the
double space between a translatable sentence..
Functionality seems to work..
One thing I do not like is the wording you use, though I do not have a
better idea (yet)..
Can someone who's native language is English look at the
drupal_set_messages()'s, please?
Stefan
------------------------------------------------------------------------
Fri, 26 Aug 2005 16:28:36 +0000 : nedjo
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
[2]. I'll post a "bug-fix only" patch as soon as I can.
[2] http://drupal.org/node/29002
------------------------------------------------------------------------
Sat, 27 Aug 2005 21:19:12 +0000 : nedjo
Attachment: http://drupal.org/files/issues/theme_region3.patch (4.37 KB)
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).
More information about the drupal-devel
mailing list