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.

Files: 
CommentFileSizeAuthor
#9 block_render_cleanup.patch4.24 KBFrando
Passed: 11089 passes, 0 fails, 0 exceptions
[ View ]
#6 block_render_cleanup.patch4.25 KBFrando
Failed: 11892 passes, 14 fails, 0 exceptions
[ View ]
#5 block_render_cleanup.patch5.49 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
block_render_cleanup.patch4.05 KBmoshe weitzman
Failed: 11792 passes, 14 fails, 0 exceptions
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch failed testing.

Seems like I lost some block titles when they are rendered. Grumble. Will fix.

subscribe.

Haven'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!

<?php
+        $page[$region]['block_description'] = array(
+         
'#markup' => $description,
+         
'#weight' => 10,
?>

Let's give it a weight of 11 so that it is always below all blocks.
<?php
// Create the $content variable that templates expect.
$variables['block']->content = $variables['elements']['#children'];
?>

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:

<?php
+      '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.

Status:Needs work» Needs review
StatusFileSize
new5.49 KB
Failed: Failed to apply patch.
[ View ]

Incorporated 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.

StatusFileSize
new4.25 KB
Failed: 11892 passes, 14 fails, 0 exceptions
[ View ]

Great! 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!

Status:Needs review» Needs work

The last submitted patch failed testing.

I 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.

Status:Needs work» Needs review
StatusFileSize
new4.24 KB
Passed: 11089 passes, 0 fails, 0 exceptions
[ View ]

OK, 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.

Status:Needs review» Reviewed & tested by the community

Agreed. Code and tests are hppy.

Status:Reviewed & tested by the community» Needs work

Committed to CVS HEAD. Marking 'code needs work' until we update the upgrade instructions. Thanks.

Status:Needs work» Fixed

I 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.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.