Simplify block rendering [#478744]

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
#9
#6	block_render_cleanup.patch	4.25 KB	Frando
#6
#5	block_render_cleanup.patch	5.49 KB	moshe weitzman
#5
	block_render_cleanup.patch	4.05 KB	moshe weitzman

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

1 June 2009 at 15:10

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #2

moshe weitzman CreditAttribution: moshe weitzman commented 1 June 2009 at 18:27

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

Comment #3

catch

he/him

English

CreditAttribution: catch commented 1 June 2009 at 20:15

subscribe.

Comment #4

Frando CreditAttribution: Frando commented 3 June 2009 at 10:14

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.

Comment #5

moshe weitzman CreditAttribution: moshe weitzman commented 6 June 2009 at 20:43

Status:

Needs work

» Needs review

File	Size
block_render_cleanup.patch	5.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.

Comment #6

Frando CreditAttribution: Frando commented 7 June 2009 at 17:01

File	Size
block_render_cleanup.patch	4.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!

Comment #7

7 June 2009 at 17:40

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #8

moshe weitzman CreditAttribution: moshe weitzman commented 7 June 2009 at 20:35

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.

Comment #9

Frando CreditAttribution: Frando commented 11 June 2009 at 20:13

Status:

Needs work

» Needs review

File	Size
block_render_cleanup.patch	4.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.

Comment #10

moshe weitzman CreditAttribution: moshe weitzman commented 12 June 2009 at 00:30

Status:

Needs review

» Reviewed & tested by the community

Agreed. Code and tests are hppy.

Comment #11

Dries CreditAttribution: Dries commented 12 June 2009 at 09:03

Status:

Reviewed & tested by the community

» Needs work

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

Comment #12

moshe weitzman CreditAttribution: moshe weitzman commented 12 June 2009 at 15:10

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.