Incorrect use of theme_form_element() [#1071200]

The function devel_node_access_block_view() calls theme_form_element() three times, instead of theme('form_element').

Additionally, it is preferable to build renderable arrays, than to generate HTML by calling theme functions when generating content for hook_block_view(), but I understand this is ported over from Drupal 6 and people aren't used to the new method yet. I still do this myself.

Comment	File	Size	Author
#8	devel_7--1_1071200_8.patch	9.55 KB	danielb
#8
#4	devel_7--1_1071200_4.patch	9.01 KB	danielb
#4
#4	devel_6--1_1071200_4.patch	3.33 KB	danielb
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

danielb CreditAttribution: danielb commented 24 February 2011 at 00:58

Though each use of theme_form_element in this function is simply used to output one sentence of text in a 'description' container.

Comment #2

salvis

he/his

CreditAttribution: salvis commented 24 February 2011 at 01:23

Please post a patch.

Comment #3

danielb CreditAttribution: danielb commented 24 February 2011 at 02:30

cvs update -dP -r DRUPAL-7--1 removes all the files from the project :/
I will try to patch against the downloaded release.

edit: nevermind, found it in Head

Comment #4

danielb CreditAttribution: danielb commented 24 February 2011 at 03:06

File	Size
devel_6--1_1071200_4.patch	3.33 KB

devel_7--1_1071200_4.patch	9.01 KB

Here's my patch. I dunno, it's not perfect, there are still theme function being called to compose the tables and lists, but I don't know if passing arrays in to those would work.

You should probably review this thoroughly as I'm still new to Drupal 7. I'm unsure about the use of '#prefix' and '#suffix' for wrapper divs.

I've also attached a much simpler patch for 6.x.

There are likely other places in this project where improvements could be made to the building of output in D7.

Comment #5

salvis

he/his

CreditAttribution: salvis commented 24 February 2011 at 10:43

Status:

Active

» Needs work

The D6 version had an extraneous parenthesis. Fixed and committed. Thanks!

For D7 I agree that returning renderable arrays is preferable.

The use of '#prefix' and '#postfix' is uneven in core, but if we go for renderable arrays, then it's logical to also go for '#prefix' and '#postfix' wherever possible, because the idea is the same. Please do this and then set the status to 'needs review'.

Otherwise the patch looks good.

Comment #6

danielb CreditAttribution: danielb commented 25 February 2011 at 22:36

The use of '#prefix' and '#postfix' is uneven in core, but if we go for renderable arrays, then it's logical to also go for '#prefix' and '#postfix' wherever possible, because the idea is the same. Please do this and then set the status to 'needs review'.

I don't understand what you're saying, and whether you've said '#postfix' instead of '#suffix' by accident? I've already used #prefix and #suffix in a few places where there was need for a wrapper around something to be themed. It isn't used in places where there is already a block of #markup, I don't see the need.

Comment #7

salvis

he/his

CreditAttribution: salvis commented 26 February 2011 at 07:37

Yes, I meant '#suffix'...

The reason for returning renderable arrays is to make it easier for other modules to get to the content of what you're returning and possibly to change it, without having to resort to regexes to analyze the HTML. If we accept this as a valid concern, then we put enclosing <div>s into '#prefix' and '#suffix', too, to make it easier for other modules to get to the content of what we're returning.

Comment #8

danielb CreditAttribution: danielb commented 28 February 2011 at 01:53

Status:

Needs work

» Needs review

File	Size
devel_7--1_1071200_8.patch	9.55 KB

Good point here's an updated version. I'm actually uncertain about how to view the output for some of these (like the bit where I added the variable $account_items) so consider this needs a review.