August 2005 - Development - lists.drupal.org

[drupal-devel] [bug] Block regions not initiated for newly enabled theme, so theme unusable
by drumm 26 Aug '05

26 Aug '05

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: drumm -Status: patch (code needs review) +Status: patch (code needs work) Updating the status. drumm 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'") ."\nOK\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". t("Success") ."\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.' . "\nFAILED\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". t('Failed') ."\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.') . "\nOK\n", array('%theme' => $theme))); ?> Do it like this: <?php + $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n". t('Success') ."\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'") ."\nOK\n"); + } + else { + $ret[] = array('0', check_plain("UPDATE {blocks} SET theme = '$default_theme'") ."\nFAILED\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.' . "\nFAILED\n", array('%theme' => $theme))); + } + else { + $ret[] = array(1, t('Blocks initiated for %theme theme.' . "\nOK\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.

1 0

[drupal-devel] [bug] Block regions not initiated for newly enabled theme, so theme unusable
by drumm 26 Aug '05

26 Aug '05

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: drumm Status: patch (code needs review) 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'") ."\nOK\n"); + } + else { + $ret[] = array('0', check_plain("UPDATE {blocks} SET theme = '$default_theme'") ."\nFAILED\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.' . "\nFAILED\n", array('%theme' => $theme))); + } + else { + $ret[] = array(1, t('Blocks initiated for %theme theme.' . "\nOK\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. drumm 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'") ."\nOK\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". t("Success") ."\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.' . "\nFAILED\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". t('Failed') ."\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.') . "\nOK\n", array('%theme' => $theme))); ?> Do it like this: <?php + $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n". t('Success') ."\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.

1 0

[drupal-devel] [bug] Block regions not initiated for newly enabled theme, so theme unusable
by chx 26 Aug '05

26 Aug '05

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: chx Status: patch (code needs review) 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. chx 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'") ."\nOK\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". t("Success") ."\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.' . "\nFAILED\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". t('Failed') ."\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.') . "\nOK\n", array('%theme' => $theme))); ?> Do it like this: <?php + $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n". t('Success') ."\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".

1 0

[drupal-devel] [bug] Block regions not initiated for newly enabled theme, so theme unusable
by nedjo 26 Aug '05

26 Aug '05

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: nedjo Status: patch (code needs review) I think there's some confusion here. The portion of my patch in question *is* to update.inc--not "the running part of Drupal". nedjo 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'") ."\nOK\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". t("Success") ."\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.' . "\nFAILED\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". t('Failed') ."\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.') . "\nOK\n", array('%theme' => $theme))); ?> Do it like this: <?php + $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n". t('Success') ."\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.

1 0

[drupal-devel] [bug] Block regions not initiated for newly enabled theme, so theme unusable
by chx 26 Aug '05

26 Aug '05

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: chx Status: patch (code needs review) 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. chx 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'") ."\nOK\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". t("Success") ."\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.' . "\nFAILED\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". t('Failed') ."\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.') . "\nOK\n", array('%theme' => $theme))); ?> Do it like this: <?php + $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n". t('Success') ."\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.

1 0

[drupal-devel] [bug] PG_QUERY ERROR - LOCK TABLES - IMPLEMENTATION FAULT.
by Cvbge 25 Aug '05

25 Aug '05

Issue status update for http://drupal.org/node/22911 Post a follow up: http://drupal.org/project/comments/add/22911 Project: Drupal Version: cvs Component: database system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: kinai Updated by: Cvbge -Status: fixed +Status: patch (ready to be committed) Please rollback your patch and either a) use syntax LOCK TABLE name (dropping S and WRITE, if you say that mysql support "LOCK") b) apply my patch. Postgres does not support WRITE part (as well as S part). Also locking won't work without starting transaction. You can do LOCK TABLE name only, but this lock is too excessive and won't allow even SELECTs on locked table. Cvbge Previous comments: ------------------------------------------------------------------------ Sun, 15 May 2005 14:34:48 +0000 : kinai I get this error : Warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error on or near "cache" at character 13 in /var/www/drupal/includes/database.pgsql.inc on line 45 Fatal error: ERROR: syntax error on or near "cache" at character 13 query: LOCK TABLES cache WRITE in /var/www/drupal/includes/database.pgsql.inc on line 62 Someone could solve this problem ? Thanks kinaï ------------------------------------------------------------------------ Sun, 15 May 2005 15:19:26 +0000 : kinai The SQL Command : LOCK TABLES like used in bootstrap.inc is only specified for MySQL, not for PostgreSQL. The UNLOCK TABLES SQL command is not implemented in PostgreSQL. Someone could solve this problem ? Thanks, Kinaï ------------------------------------------------------------------------ Sat, 28 May 2005 23:34:03 +0000 : bd_csmc this issue still exists, i've verified it on a fresh cvs install w/ pgsql 8 ------------------------------------------------------------------------ Sat, 28 May 2005 23:39:52 +0000 : bd_csmc i fixed the problem in my installation, by changing the syntax to: LOCK TABLE {cache} it's table, not tables. and i dropped the 'write' bit. furthermore, pgsql does not support the UNLOCK command, so i had to comment the unlock tables command. unless there are more lock/unlock commands elsewhere in the code, this works fine for me. according to pgsql docs, UNLOCK is automatically performed at transaction end, which is why there is no such command. ------------------------------------------------------------------------ Fri, 19 Aug 2005 11:29:25 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks.diff (2.91 KB) The patch adds two functions to lock and unlock tables. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:57:43 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks_0.diff (2.8 KB) Updated, without unnecessary changes. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:07:18 +0000 : chx Cvbge says he tested pgsql, I applied to a mysql one, saved system settings and looks OK. ------------------------------------------------------------------------ Tue, 23 Aug 2005 14:31:53 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks_1.diff (2.81 KB) Worked, yet not worked ... This version does not use <?php db_query("{$table}" ) ?> syntax. I promise I won't do that again! ------------------------------------------------------------------------ Thu, 25 Aug 2005 21:10:07 +0000 : Dries MySQL is happy with the singular 'LOCK TABLE' (rather than 'LOCK TABLES') which should work with both MySQL and PostgreSQL. Committed that changes to CVS.

1 0

[drupal-devel] [bug] PG_QUERY ERROR - LOCK TABLES - IMPLEMENTATION FAULT.
by Dries 25 Aug '05

25 Aug '05

Issue status update for http://drupal.org/node/22911 Post a follow up: http://drupal.org/project/comments/add/22911 Project: Drupal Version: cvs Component: database system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: kinai Updated by: Dries Status: patch (ready to be committed) MySQL is happy with the singular 'LOCK TABLE' (rather than 'LOCK TABLES') which should work with both MySQL and PostgreSQL. Committed that changes to CVS. Dries Previous comments: ------------------------------------------------------------------------ Sun, 15 May 2005 14:34:48 +0000 : kinai I get this error : Warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error on or near "cache" at character 13 in /var/www/drupal/includes/database.pgsql.inc on line 45 Fatal error: ERROR: syntax error on or near "cache" at character 13 query: LOCK TABLES cache WRITE in /var/www/drupal/includes/database.pgsql.inc on line 62 Someone could solve this problem ? Thanks kinaï ------------------------------------------------------------------------ Sun, 15 May 2005 15:19:26 +0000 : kinai The SQL Command : LOCK TABLES like used in bootstrap.inc is only specified for MySQL, not for PostgreSQL. The UNLOCK TABLES SQL command is not implemented in PostgreSQL. Someone could solve this problem ? Thanks, Kinaï ------------------------------------------------------------------------ Sat, 28 May 2005 23:34:03 +0000 : bd_csmc this issue still exists, i've verified it on a fresh cvs install w/ pgsql 8 ------------------------------------------------------------------------ Sat, 28 May 2005 23:39:52 +0000 : bd_csmc i fixed the problem in my installation, by changing the syntax to: LOCK TABLE {cache} it's table, not tables. and i dropped the 'write' bit. furthermore, pgsql does not support the UNLOCK command, so i had to comment the unlock tables command. unless there are more lock/unlock commands elsewhere in the code, this works fine for me. according to pgsql docs, UNLOCK is automatically performed at transaction end, which is why there is no such command. ------------------------------------------------------------------------ Fri, 19 Aug 2005 11:29:25 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks.diff (2.91 KB) The patch adds two functions to lock and unlock tables. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:57:43 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks_0.diff (2.8 KB) Updated, without unnecessary changes. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:07:18 +0000 : chx Cvbge says he tested pgsql, I applied to a mysql one, saved system settings and looks OK. ------------------------------------------------------------------------ Tue, 23 Aug 2005 14:31:53 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks_1.diff (2.81 KB) Worked, yet not worked ... This version does not use <?php db_query("{$table}" ) ?> syntax. I promise I won't do that again!

1 0

[drupal-devel] [task] Remove old manual update info
by drumm 25 Aug '05

25 Aug '05

Issue status update for http://drupal.org/node/29597 Post a follow up: http://drupal.org/project/comments/add/29597 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: drumm Reported by: drumm Updated by: drumm Status: patch (code needs review) Attachment: http://drupal.org/files/issues/update.php_1.diff (3.65 KB) We can get rid of all this since the associated pre-4.5 updates aren't there. drumm

4 5

[drupal-devel] [feature] Browse by month
by Morbus Iff 25 Aug '05

25 Aug '05

Issue status update for http://drupal.org/node/9414 Post a follow up: http://drupal.org/project/comments/add/9414 Project: Drupal Version: cvs Component: archive.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: ahoeben Updated by: Morbus Iff Status: patch (code needs review) Attachment: http://drupal.org/files/issues/_p_9414_archive.patch (5.09 KB) Attached updated patch for HEAD. I'll be fiddling with it some more too. Morbus Iff Previous comments: ------------------------------------------------------------------------ Tue, 20 Jul 2004 12:00:51 +0000 : ahoeben Attachment: http://drupal.org/files/issues/archive_AH040720.patch (5.22 KB) I made a couple of changes to archive.module to facilitate browsing the archives by date. * Opening an archive URL without specifying a day will display the full month, eg www.example.com/archive/2004/07 * Selecting the 0th day of the month in the archive page drop down selects the full month * Using the previous/next raquo's in the calender block now uses full month browsing The attached patch is against 4.4.2 and is untested against CVS. ------------------------------------------------------------------------ Wed, 11 Aug 2004 12:15:46 +0000 : Anonymous Patch don't work with archive.module (drupal 4.4.2). Hunk #1 FAILED at 1. Hunk #2 FAILED at 26. Hunk #3 FAILED at 49. Hunk #4 FAILED at 64. Hunk #5 FAILED at 85. Hunk #6 FAILED at 131. Hunk #7 FAILED at 218. Hunk #8 FAILED at 236. Hunk #9 FAILED at 245. 9 out of 9 hunks FAILED ------------------------------------------------------------------------ Sat, 07 May 2005 23:02:45 +0000 : ahoeben Attachment: http://drupal.org/files/issues/archive_AH050508.patch (4.78 KB) Blast from the past... Not sure what I did wrong to have my original patch fail, but here is a new patch applying the same changes to Drupal 4.6 ------------------------------------------------------------------------ Sat, 07 May 2005 23:16:48 +0000 : Cvbge Hello, thanks for your patch, that's a feature I wanted :) But please change day "0" to "ANY" or similar. ------------------------------------------------------------------------ Sun, 08 May 2005 10:19:21 +0000 : ahoeben Attachment: http://drupal.org/files/issues/archive_AH050508b.patch (5.15 KB) New patch. Changed '0' in day dropdown to 'All days'. You should be able to change that string using a locale (but I have no experience with locales in Drupal). Also fixed a tiny bug that would cause the year dropdown to stop being useful on jan 1st 2006. ------------------------------------------------------------------------ Tue, 10 May 2005 13:50:13 +0000 : ahoeben Patch applies to cvs version as well...

1 0

[drupal-devel] [bug] Block regions not initiated for newly enabled theme, so theme unusable
by nedjo 25 Aug '05

25 Aug '05

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: nedjo Status: patch (code needs review) "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. nedjo 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'") ."\nOK\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". t("Success") ."\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.' . "\nFAILED\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". t('Failed') ."\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.') . "\nOK\n", array('%theme' => $theme))); ?> Do it like this: <?php + $ret[] = array(1, t('Blocks initiated for theme %theme.', array('%theme' => theme('placeholder', $theme)) . "\n". t('Success') ."\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)..

1 0