In FormAPI, the #value property does double-duty. Sometimes, it represents processable input values from a $_POST submission. Other times (specifically, with 'item' and 'markup' element types), it represents raw HTML markup that should be printed straight to the page. This leads to ambiguity, potential security snafus when themers override normal form rendering, and requires some tricky dancing in drupal_render().

The following patch, originally suggested by Steven Wittens in Barcelona, changes 'item' and 'markup' element types to use the #markup property for direct-output HTML rather than the #value property. This impacts anything that was pushed through drupal_render() as well, including node bodies. Upload module, book module, poll module, forum module, and others are included.

CommentFileSizeAuthor
#27 docs-252013-27.patch6.77 KBpwolanin
#23 type-markup-252013-23.patch65.16 KBpwolanin
#18 type-markup-252013-18.patch63.23 KBpwolanin
#14 type-markup-252013-14.patch62.3 KBpwolanin
#9 form_markup_0.patch62.21 KBSusurrus
form_markup.patch60.01 KBeaton
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eaton’s picture

eaton CreditAttribution: eaton commented
Title: markup and item should use #markup, not #value » drupal_render() should print #markup, not #value

Changing the title to better reflect the nature of the patch.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

+1 for the clarification of the API
-1 for the API change

I'm not sure that gain of 6 lines of code in drupal_render really worth changing all core and contributed modules out there.

eaton’s picture

eaton CreditAttribution: eaton commented

I'm not sure that gain of 6 lines of code in drupal_render really worth changing all core and contributed modules out there.

Three thoughts.

  1. The change is exceedingly minor -- changing the name of one ambiguous property to a less ambiguous one. Only modules that use 'markup' and 'item' elements are affected.
  2. The 'payoff' is only partially related to drupal_render(). Decreasing the chances of a confused themer directly outputting #value is another significant gain.
  3. drupal_render() is slowly becoming one of our central 'heavy lifting' functions, and the six lines in question made its inner workings much more difficult to understand. Making the API simpler, even incrementally, is a significant improvement IMO.

If there is significant opposition, I won't fight for this patch, but I do feel strongly that some of our little-known 'inner loop' functions like drupal_render() and _form_builder() need to be slowly untangled, to improve their readability and simplify the logic. This is one of the steps in that direction.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I don't think the API change is a big deal here, like Eaton says it's very minor.

My only issue would be that #prefix and #suffix are also used for markup - which gives us stuff like these three lines copied out of the patch file:

           '#markup' => t('Depends on: !dependencies', array('!dependencies' => implode(', ', $dependencies))),
           '#prefix' => '<div class="admin-dependencies">',
           '#suffix' => '</div>',

That gives us three different things all providing 'markup', one of which is called #markup, in this case making up a whole form element. Which just looks weird.

eaton’s picture

eaton CreditAttribution: eaton commented

That gives us three different things all providing 'markup', one of which is called #markup, in this case making up a whole form element. Which just looks weird.

That's the way it currently works. We just call one of them '#value', which coincidentally means something different when used with input-enabled elements.

There's a whole ball of snakes to be untangled with how the FAPI structure is tied up with the markup that's generated. It makes a lot of things simpler, but there are very subtle behaviors like the need for #prefix and #suffix, as you noted. prefix and suffix, for their part, are wrapped around the themed version of the element itself. So, theme_markup() might wrap the text in a span. #prefix and #suffix would appear outside of, rather than inside of, that span.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

That's the way it currently works. We just call one of them '#value', which coincidentally means something different when used with input-enabled elements.

This is true, and it's an improvement on #value, it just highlights the quirk. I tried to think of alternatives to #prefix, and only came up with #prefix-markup, which is worse. So I don't think their existence should any way hold this patch up. It'd be worth another issue though maybe.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

I was trying to understand this dual use of #value in drupal_render(), so glad to see a clarification of this code.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

I like the idea, and I think this would help more the rendering patch forward.

However, the patch fails to apply in a number of places.

Susurrus’s picture

Susurrus CreditAttribution: Susurrus commented
Status: Needs work » Needs review
FileSize
form_markup_0.patch62.21 KB

Here's an updated version of the patch that should apply cleanly. I haven't had a chance to run all SimpleTests yet, as they take forever to run, but I will start that running now and report back. There's nothing that actually tests the FAPI directly I don't think, but it would probably be nice to have something like that to simplify testing any changes made to it.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Thanks for re-rolling it - however, the code in drupal_render doesn't look right. #prefix and #suffix are unset before they can be used.

Susurrus’s picture

Susurrus CreditAttribution: Susurrus commented

@pwolanin: #prefix and #suffix are unset for only a specific condition. This is actually how drupal_render works now. At another point in drupal_render(), you can see they are being used:

    $prefix = isset($elements['#prefix']) ? $elements['#prefix'] : '';
    $suffix = isset($elements['#suffix']) ? $elements['#suffix'] : '';
    return $prefix . $content . $suffix;
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Susurrus - ok, let me look at the actual patched code, rather than just the patch so I can see the whole function.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

I still get some failed hunks (also seems to have windows line endings). Also, please diff with -p or -F^f.

11 out of 14 hunks FAILED -- saving rejects to file includes/locale.inc.rej
(Stripping trailing CRs from patch.)

Hunk #1 FAILED at 49.
1 out of 2 hunks FAILED -- saving rejects to file modules/blog/blog.module.rej

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
type-markup-252013-14.patch62.3 KB

here's a revised patch. Caught several places where the conversion was missed. Also, it seems the unset of #prefix and #suffix as it remained was breaking things like the node preview. So, maybe Eaton can explain the intention there- I don't really get it. I took it out for now and node previews work again.

Dries’s picture

Dries CreditAttribution: Dries commented

I think this change looks good, and it should probably be committed.

I can't run the tests right now, but maybe someone else can. If there are no regressions, I think this is good to go in.

I've been thinking about how we can make this more fail-safe but haven't any good ideas right now. Maybe someone else still has.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

edit: ignore this one.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Needs work

I get Blog functionality: 181 passes, 6 fails, 0 exceptions

Otherwise all fine.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
type-markup-252013-18.patch63.23 KB

hmm, I thought the blog one was a known fail (we really need to get those resolved this week).

Ok, I found where I missed one line in blog.module - a 'user_profile_item'.

With this change, all the blog module tests pass.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs work » Needs review

Testing caught a regression! Awesome!

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Needs work

2 actually - page preview and this blog one.

found another: with the patch upload module test has 4 fails, without it has 3. However, manually clicking is ok. Anyone with insight into this?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I get a couple of fails with upload/user pictures on one of my boxes, not consistent. I don't think this patch made any difference to that though.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review

ok, then CNR at least

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
type-markup-252013-23.patch65.16 KB

re-roll for the fact that image.gd.inc has moved.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Locale module test has a couple failures now, but due to: http://drupal.org/node/280628

eaton’s picture

eaton CreditAttribution: eaton commented

I need to run through and get a full D7 test environment set up before I can verify that the tests are passing properly, but after reviewing the meat of the patch itself (the changes to drupal_render()), I give it a bit two thumbs up. It's an important change that makes explaining some of our security rules a good deal easier, IMO.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

I've reviewed, tested and committed this patch. Thanks Eaton, pwolanin, Susurrus et al.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
docs-252013-27.patch6.77 KB

updated: http://drupal.org/node/224333 and core docs: http://drupal.org/cvs?commit=128130 per attached patch

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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