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.

CommentFileSizeAuthor
#9 block_render_cleanup.patch4.24 KBFrando
#6 block_render_cleanup.patch4.25 KBFrando
#5 block_render_cleanup.patch5.49 KBmoshe weitzman
block_render_cleanup.patch4.05 KBmoshe weitzman

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

moshe weitzman commented

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

catch’s picture

catch
he/him
Primary language English
commented

subscribe.

Frando’s picture

Frando commented

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!

+        $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.

+  // 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:
+ '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.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Needs review
StatusFileSize
new block_render_cleanup.patch5.49 KB

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.

Frando’s picture

Frando commented
StatusFileSize
new block_render_cleanup.patch4.25 KB

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.

moshe weitzman’s picture

moshe weitzman commented

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.

Frando’s picture

Frando commented
Status: Needs work » Needs review
StatusFileSize
new block_render_cleanup.patch4.24 KB

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.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

Agreed. Code and tests are hppy.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

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

moshe weitzman’s picture

moshe weitzman commented
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.