Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This patch implements #theme_wrapper to theme an individual block. That let us simplify the rendering code and even remove a drupal_render() call in template_preprocess_block().
The innovation above let me fix an annoyance in hook_page_alter() where the main page content is buried under $page['content']['blocks']['system-main']['#block']['content']. Particularly ugly is the #block there. This patch moves all block content up to $page['content]['system-main'] (for example). Thats significantly simpler.
Comment | File | Size | Author |
---|---|---|---|
#9 | block_render_cleanup.patch | 4.24 KB | Frando |
#6 | block_render_cleanup.patch | 4.25 KB | Frando |
#5 | block_render_cleanup.patch | 5.49 KB | moshe weitzman |
block_render_cleanup.patch | 4.05 KB | moshe weitzman | |
Comments
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedSeems like I lost some block titles when they are rendered. Grumble. Will fix.
Comment #3
catchsubscribe.
Comment #4
Frando CreditAttribution: Frando commentedHaven't tried the patch, but here's a code review:
I like the overall approach. Much much cleaner than what's in core right now. This is definitely needed. Great!
Let's give it a weight of 11 so that it is always below all blocks.
Hm, don't like that too much. This overloads the $block->content property and gives it multiple meanings based on position in the code (either unrendered or rendered content). I'd just do $variables['content'] = $variables['elements']['#children'] and update the block templates.
Completely unrelated to this patch, but something to think about:
+ 'arguments' => array('elements' => NULL),
(in hook_theme, that is). We see more and more and more of these, for every theme function that is only used via #theme. That's quite redundant to declare each time. Maybe make it the default? But as I said, unrelated to this patch.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedIncorporated Frando's feedback and fixed block title issue by paying attention to nesting of elements in $page.
FYI, it is easy to browse $page for any Drupal page by enabling 'Show $page array' with devel module.
Comment #6
Frando CreditAttribution: Frando commentedGreat! I like the patch a lot.
It broke, though, for drupal_static conversion of block.module. Attached reroll makes it apply.
You kept the block content in a 'content' key (by moving the block content into
$block->content['content']
in_block_render_blocks
). I removed that - a block's content is now directly in$block->content
. This means that we now have$page['content']['system_main']['nodes']
instead of$page['content']['system_main']['content']['nodes']
, which is even nicer and simpler IMO. No visible changes at all, and I can't think of any sideeffects.This is RTBC in my opinion. If you agree with my tiny change, go ahead an RTBC it after the testbot has passed!
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedI think you have broken block titles just like I did earlier. I had to add a level of nesting (i.e. content) in order for there to be no clashes between #theme_wrapper that is in the form that is returned by the search block and login block.
Comment #9
Frando CreditAttribution: Frando commentedOK, to get rid of the 'content' key in the blocks we need to support multiple theme wrappers. There's an issue for that already: #433992: Radios missing class form-radios, theme_radios() never called. Should that get in we can revisit this.
But for now, let's go with the previous version! It is already a huge improvement over what we have in HEAD currently. The attached patch is #5, rerolled to make it apply. Let's get this in.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedAgreed. Code and tests are hppy.
Comment #11
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Marking 'code needs work' until we update the upgrade instructions. Thanks.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedI added a mention that blocks should return renderable arrays in addition to menu callbacks doing same. i think thats all thats needed for upgrade docs.