Regions - Lost the Navigation Block while Building a Theme

this should probably be tested on a -dev tarball of D6 to see if it can be reproduced. http://drupal.org/node/97368

See http://drupal.org/node/201165, I have to check if the patch still applies.

The structure isn't right.

if (!empty($old_blocks[$module][$delta]['region']) && !array_key_exists($old_blocks[$module][$delta]['region'], $regions)) {
  drupal_set_message(t('Block %info has been disabled because its assigned to an invalid region: %region', array('%info' => $old_blocks[$module][$delta]['info'], '%region' => $old_blocks[$module][$delta]['region'])), 'error');
  $old_blocks[$module][$delta]['status'] = 0;
  $old_blocks[$module][$delta]['region'] = BLOCK_REGION_NONE;
}
else {
  $old_blocks[$module][$delta]['region'] = $old_blocks[$module][$delta]['status'] ? $old_blocks[$module][$delta]['region'] : BLOCK_REGION_NONE;
}

$old_blocks[$module][$delta]['region'] will always have a value so that first condition will only check on the array_key_exists().

How about moving $old_blocks[$module][$delta]['status'] in its place and remove the ternary conditional.

@dvessel: $old_blocks[$module][$delta]['region'] on my tests this is empty where region is <none>

Your right. I really shouldn't be reviewing anything late a night.

I like the new behavior. In 5, the disabled regions would be pushed into the default region with system_default_region() but the way it's cached doesn't allow that in 6. This is reasonable so marking it ready.

Works for me.

- Erm, why is this an 'error' level message to the user? Does this bug cover the case when a region disappears and the user visits the blocks admin page?
- The English in the code comments could use some polish: "if region name not exists" does not sounds right to me.

Changed the dsm() to 'warning', and tidied up comment English, don't credit me if this gets committed. Not tested this yet.

@catch: Thanks for your corrections.
@Gábor: This bug covers the case you mentioned on #9: 'blocks admin page'.

There was also another issue opened by hampshire: http://drupal.org/node/201165

$old_blocks[$module][$delta]['region'] on my tests this is empty <= empty or not set? Also, why array_key_exists instead of isset can the value of $regions[$old_blocks[$module][$delta]['region']] be NULL? that'd be the only reason to use the very slow array_key_exists instead of isset.

@chx: $old_blocks[$module][$delta]['region'] on my tests this is empty, isset returns TRUE.

I've changed the slow array_keys_exists for an isset on the regions list.

Well if it is set then why do we need to check for emptiness? Surely $regions[''] is not set. I have asked that so that we are not putting a notice in here.

It's not working for me because every already disabled module it has a ' ' value for its region name.

OK then #13 is the current to be reviewed.

BTW, in this message in #13, "its" should be "it is" (or at the very least, "it's").

+            drupal_set_message(t('Block %info has been disabled because its assigned to an invalid region: %region', array('%info' => $old_blocks[$module][$delta]['info'], '%region' => $old_blocks[$module][$delta]['region'])), 'warning');

Or, how about:

+            drupal_set_message(t('The block %info was assigned to the invalid region %region and has been disabled.', array('%info' => $old_blocks[$module][$delta]['info'], '%region' => $old_blocks[$module][$delta]['region'])), 'warning');

Thanks Keith.
I'm attaching two files with two versions of #13 based on #17.

OK I'd go for the wording in block_region_check_e1.patch

Tested this with a contrib theme, works fine, so marking RTBC.

Committed that one, thanks.