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.
Comment | File | Size | Author |
---|---|---|---|
#11 | clean_region_naming_1.patch | 17.02 KB | dvessel |
clean_region_naming.patch | 17.02 KB | dvessel | |
Comments
Comment #1
dvessel CreditAttribution: dvessel commentedForgot 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.
Comment #2
merlinofchaos CreditAttribution: merlinofchaos commentedThis 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!
Comment #3
Senpai CreditAttribution: Senpai commentedI 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?
Comment #4
Senpai CreditAttribution: Senpai commentedBTW, the notice was:
Comment #5
merlinofchaos CreditAttribution: merlinofchaos commentedWe have a No Notice policy so that needs work. It should be an easy fix and can then be RTBC'd tho
Comment #6
Senpai CreditAttribution: Senpai commentedOn 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.
Comment #7
merlinofchaos CreditAttribution: merlinofchaos commentedThis 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'].
Comment #8
Senpai CreditAttribution: Senpai commentedRetested 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!
Comment #9
dvessel CreditAttribution: dvessel commentedThat 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.
Comment #10
Dries CreditAttribution: Dries commentedThis patch needs to be re-rolled as it no longer applies correctly.
Comment #11
dvessel CreditAttribution: dvessel commentedHEAD is moving fast today. :)
Comment #12
RobRoy CreditAttribution: RobRoy commented$variables['layout'] = ($variables['layout'] == 'left') ? 'both' : 'right';
should be
$variables['layout'] = ($variables['layout'] == 'left' ? 'both' : 'right');
Comment #13
merlinofchaos CreditAttribution: merlinofchaos commentedRobRoy: The two pieces of code you put are functionally idential. Moving the parens has no effect, and your version is the less readable.
Comment #14
dman CreditAttribution: dman commentedThankyou.
I had another head-slap moment again this week when trying to understand why the template (gutenberg) was printing out the
$header
just fine, but never displaying anything in the$footer
....
$footer_message
indeed! Thanks for clearing this up.It'll cause a bit of pain to all themes during upgrade, but it's worth it.
Comment #15
Dries CreditAttribution: Dries commentedI've committed this patch, but we'll want to check if any of the documentation or upgrade instructions need to be updated.
Comment #16
dvessel CreditAttribution: dvessel commentedUpdated guide updated with the changes. :)
http://drupal.org/node/132442#region-variables
Comment #17
(not verified) CreditAttribution: commented