API page: https://api.drupal.org/api/drupal/core!includes!common.inc/function/drup...

The drupal_render documentation has instances of setting class to a string like:

'#attributes' => array('class' => 'foo'),

which should really be:

'#attributes' => array('class' => array( 'foo')),

Setting the class attribute to a string will work for some render arrays, but not for any that have pre_render functions that expect class to be an array. Any similar instances of this error in other Drupal 8 docs should also be corrected.

CommentFileSizeAuthor
#15 drupal8-class-attribute-2304399-15.patch5.59 KBundertext
#12 drupal8-class-attribute-2304399-12.patch5.72 KBundertext
#6 drupal8-class-attribute-2304399-6.patch928 bytespushpinderchauhan
#6 interdiff-2304399-2-6.txt505 bytespushpinderchauhan
#2 drupal8-class-attribute-2304399-2.patch824 bytespushpinderchauhan

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue tags: +Novice

Thanks, good idea to fix this! If you find other cases of documentation doing that, please file new issues. Let's get drupal_render() fixed on this issue.

By the way, the Drupal 7 version of the function doesn't have this example in it.

pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Active » Needs review
StatusFileSize
new drupal8-class-attribute-2304399-2.patch824 bytes

Please review.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Perfect! Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

The thing is that it does not have to be an array - should we be documenting this here?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

It doesn't *have* to be an array, but it usually is an array. I don't think it's a bad idea to have the examples changed to use arrays? We're not changing the documentation that talks about the class attribute, just some code samples that have to do with using the #theme_wrappers property anyway.

Of course, if we're doing that, I just noticed that one of the examples, just below where this patch ends, is still not an array:

  *         'container' => array(
  *           '#attributes' => array('class' => 'bar'),
pushpinderchauhan’s picture

pushpinderchauhan commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2304399-2-6.txt505 bytes
new drupal8-class-attribute-2304399-6.patch928 bytes

Did changes as mentioned in #5, please review updated patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

I think this is OK... alexpott: see #5 for why I think this should probably be done, but I'll let you make the decision.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: Unassigned » alexpott
samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented

If it helps, the functions in form.inc that expect class to be an array are:

  • _form_set_attributes
  • form_pre_render_conditional_form_element
  • form_process_actions
  • form_process_autocomplete
  • form_pre_render_button
  • form_pre_render_image_button
  • template_preprocess_textarea

I'm not sure if we should ever suggest that people set class to a string given the amount of functions that rely on it being an array.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: "class" attribute in drupal_render documentation should be an array » "class" attribute in drupal_render should be an array
Component: documentation » theme system

So let's also remove all instances where we don't set class to an array...

  • views_ui_preprocess_views_view()
  • TourTestController::tourTest1() - tip-5
  • RenderTest
  • RenderElementTypesTest

are the only places I can find where we do this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
undertext’s picture

undertext commented
StatusFileSize
new drupal8-class-attribute-2304399-12.patch5.72 KB
undertext’s picture

undertext commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: drupal8-class-attribute-2304399-12.patch, failed testing.

undertext’s picture

undertext commented
StatusFileSize
new drupal8-class-attribute-2304399-15.patch5.59 KB

Argh, unix-style line endings. Trying one more time

undertext’s picture

undertext commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

The patch looks good to me, and fixes all locations identified by alexpott in#10. Plus, testbot agrees that the tests still pass. Should be good to go, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 0d99a0c and pushed to 8.x. Thanks!

  • alexpott committed 0d99a0c on 8.x 
    Issue #2304399 by er.pushpinderrana, undertext | samuel.mortenson: Fixed...

Status: Fixed » Closed (fixed)

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

m1r1k’s picture

m1r1k commented
Issue tags: +#ams2014contest