The variables $sidebar_left, $sidebar_right and $footer_message don't exactly match what the regions are set to be. This was a pre 4.7 convention, right? Before the block system that exists today.

All this does is change those region variables to $left, $right and $footer. Setting the regions through the .info is a lot more straight forward this way. Why should region[left] end up being $sidebar_left? I touched on every instance of this. All themes should be fine. I tested them all.

The global $sidebar_indicator was removed and *all* regions get its unique zebra and counting variable specific to the region.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dvessel’s picture

Forgot to mention, it fixes the block counter in "template_preprocess_block()". No static was set. It also uses the new "template_preprocess()" function that was moved recently.

merlinofchaos’s picture

Title: Fix region variables naming. » Remove ancient cruft from page.tpl.php region naming
Category: task » bug

This leftover cruft from pre 4.7 definitely should be removed. While this is a minor API change, I think this is a case where removing cruft really simplifies things; mostly what is going on here is the removal of code that simply doesn't need to be there, with the added bonus side effect of treating all regions the same.

Because it is bad API and very crufty, I consider this a bug, if a fairly minor one. It is a bug because it confuses the crap out of people who set up their regions and expect $left to work as documented.

I think this is very good. I'd like one other party to run a full test, but other than that I honestly feel this should be RTBC. Anyone who fully tests this and can confidently say this is working, you have my blessing to RTBC this!

Senpai’s picture

Assigned: Unassigned » Senpai
Status: Needs review » Reviewed & tested by the community

I tested the clean_region_naming.patch just now, and I get a PHP notice on the Marvin theme, but other than that, all looks well!

I'd RTBC it if you're not too worried about one little notice message on a theme?

Senpai’s picture

BTW, the notice was:

notice: Trying to get property of non-object in /includes/theme.inc on line 1729
merlinofchaos’s picture

Status: Reviewed & tested by the community » Needs work

We have a No Notice policy so that needs work. It should be an easy fix and can then be RTBC'd tho

Senpai’s picture

On the patched file, line 1729 is:

1728 // If on an individual node page, add the node type.
1729 if (isset($variables['node']) && $variables['node']->type) {
1730 $body_classes[] = 'node-type-'. form_clean_id($variables['node']->type);
1731 }

And we really do have a ' no notices' policy? http://drupal.org/node/104416
I'm confused.

merlinofchaos’s picture

This is new for Drupal 6. Drupal 6 is supposed to be NOTICE free. It probably just needs an is_object thrown in there to guarantee the safety of $variables['node'].

Senpai’s picture

Status: Needs work » Reviewed & tested by the community

Retested the patched module against a newly updated HEAD, and with devel_node_access.module shut off, I get no errors, warnings, or notices. All content is where its supposed to be, and blocks can be moved around at random, no matter which theme is active.

RTBC!

dvessel’s picture

That was just a small error from Senpai. That line should not be giving errors. I was going over it with Senpai in IRC and all is well now.

Thanks for checking it out.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

This patch needs to be re-rolled as it no longer applies correctly.

dvessel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.02 KB

HEAD is moving fast today. :)

RobRoy’s picture

Status: Reviewed & tested by the community » Needs work

$variables['layout'] = ($variables['layout'] == 'left') ? 'both' : 'right';

should be

$variables['layout'] = ($variables['layout'] == 'left' ? 'both' : 'right');

merlinofchaos’s picture

Status: Needs work » Reviewed & tested by the community

RobRoy: The two pieces of code you put are functionally idential. Moving the parens has no effect, and your version is the less readable.

dman’s picture

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I've committed this patch, but we'll want to check if any of the documentation or upgrade instructions need to be updated.

dvessel’s picture

Status: Needs work » Fixed

Updated guide updated with the changes. :)

http://drupal.org/node/132442#region-variables

Anonymous’s picture

Status: Fixed » Closed (fixed)