Posted by nevets on May 11, 2009 at 4:11pm
3 followers
| Project: | Region Visibility |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
I like the idea behind the module though I would like to propose a change to make it more efficient.
Currently the code uses region_conf_preprocess_page() to hide the regions. This means the blocks have already been processed. To avoid processing the blocks I would like to propose implementing region_conf_blocks() which overrides theme_blocks() and only processes blocks in a region if the region is visible. The change also mean implementing region_conf_theme().
The attached code reflects these changes.
| Attachment | Size |
|---|---|
| region_conf.module.txt | 7.48 KB |
Comments
#1
I like the idea of catching the blocks before they've even been rendered, but I don't think this implementation would actually work. AFAIK, theme functions are only caught by themes, not modules. You'd have to do some theme registry altering in order to get it to use your function.
Also, region_conf.module and region_visibility.module are going to merge.
#2
Actually the code is tested and works, modules can catch theme functions if they implement hook_theme.
#3
I hope you keep your code, it has a nicer architecture.
#4
Ah, thanks nevets, didn't notice the hook_theme call right before it. I'll look into this some more when I get a chance.
#5
Ok, if you get a chance nevets, can you check out this patch? I'm still on the fence if themes should be allowed to override theme_blocks() if we're doing it in there. So if we use your method a theme would be able to override theme_blocks, essentially making this module useless. If we use theme_registry_alter to set the function, no themes would be allowed to override it.
I decided to go the route of using theme_registry_alter instead of hook_theme, so that a theme can't override the function. hook_theme also introduced some other ugliness, by adding two template_preprocess functions to $theme_registry['blocks'], and seems a bit hackish to me, as it's essentially doing what theme_registry_alter is there for in the first place.
I think I'm going to go ahead and commit this patch to the 6.x-1.x-dev branch, but I attached the patch here in case it doesn't get packaged up before you have time to check it out.
I'd appreciate your feedback.
Thanks!
#6
#7
I'm going to mark this as fixed, please re-open if you think it needs tweaking.
#8
Automatically closed -- issue fixed for 2 weeks with no activity.