Fresh install of 7.x-1.x-dev dated 4/13. Just core modules, nothing else.

On all forms elements with type "radios", the div with class form-radios is missing from the markup sent to the browser, and theme_radios() is never called. An example of the problem can be seen at admin/content/node-settings. In Drupal 6 the markup for the two radio buttons on that page looks like this:

<div class="form-item">
  <label>Preview post: </label>
  <div class="form-radios">
    <div class="form-item" id="edit-node-preview-0-wrapper">
      <label class="option" for="edit-node-preview-0"><input type="radio" id="edit-node-preview-0" name="node_preview" value="0"  checked="checked"  class="form-radio" /> Optional</label>
    </div>
    <div class="form-item" id="edit-node-preview-1-wrapper">
      <label class="option" for="edit-node-preview-1"><input type="radio" id="edit-node-preview-1" name="node_preview" value="1"   class="form-radio" /> Required</label>
    </div>
  </div>
  <div class="description">Must users preview posts before submitting?</div>
</div>

While in Drupal 7 the same markup looks like this:

<div class="form-item">
  <label>Preview post: </label>
  <div class="form-item" id="edit-node-preview-0-wrapper">
    <label class="option" for="edit-node-preview-0"><input type="radio" id="edit-node-preview-0" name="node_preview" value="0"  checked="checked"  class="form-radio" /> Optional</label>
  </div>
  <div class="form-item" id="edit-node-preview-1-wrapper">
    <label class="option" for="edit-node-preview-1"><input type="radio" id="edit-node-preview-1" name="node_preview" value="1"   class="form-radio" /> Required</label>
  </div>
  <div class="description">Must users preview posts before submitting?</div>
</div>

The difference is that the D7 markup lacks a div with class .form-radios (<div class="form-radios"></div>) that is present in D6. In trying to track this down, I found that theme_radios() (with an s) is never called. However, theme_radio() is called twice as expected.

As a consequence, styling radios using the .form-radios selector doesn't work (e.g. system.css defines several styles for .form-radios), nor does any JavaScript looking for those elements.

I didn't find any mention of this problem in the forums or the release notes, and it doesn't seem like an intentional change. Perhaps someone who is better versed on the innards of the FormAPI in D7 has some idea what's happening here.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Status: Active » Needs work
FileSize
1.67 KB

I came across this too. It looks like the bug was introduced as part of #355236: Refactor drupal_render theming - docs, where the theme functions that would normally run for radios and checkboxes seems to get replaced by a different function, when actually the intention is to string the two functions together and run them both...

The attached patch seems to fix the problem for me, but there are two caveats:

  • I fixed this by allowing the new #theme_wrapper element to be an array, but it looks like this idea was discussed and rejected at some point during the discussion of #355236: Refactor drupal_render theming - docs. Someone who understands this part of Drupal better should definitely weigh in here.
  • The fix here is quick but ugly. If making #theme_wrapper an array is in fact the correct solution, then we want to force it to be an array everywhere rather than adding the crazy special-case code I added in this patch... I was just too lazy to go do that given that I'm not even sure this is the correct solution :)
David_Rothstein’s picture

Status: Needs work » Needs review

Actually let's set it to "needs review" so the testbot can run, and also because the idea itself needs review. As mentioned above, though, it's really "needs work" :)

Frando’s picture

Well, in the first iterations of the drupal_render() theming patch, as you said, I had #theme_wrapper as an array to allow multiple wrapper theme functions to run one after the other.

I removed that after Eaton said it would be too complex. I am kinda torn, because yes, #theme_wrapper allowing multiple functions to be run would make the syntax a little more complex and a little different from #theme, which would remain just one theme function name, but on the other side, I still see the benefits of allowing multiple #theme_wrappers. For example, in this issue, but also potentially in other cases where more markup is shared between different elements.

If I were to decide, I think I'd do #theme_wrapper as an array to solve this. Or, I'd allow #theme_wrapper to be either a string containing one theme function name or an array of multiple theme functions to be run.

David_Rothstein’s picture

Probably if it's allowed to be an array, it should always be an array - otherwise it would tend to lead to awkward code as shown in my above patch. Plus, it seems to be the way Drupal does it for other things; #submit and #validate are arrays even though commonly the array will only contain one element...

Maybe we could force it to be an array but rename it to #theme_wrappers? Hopefully the plural would help people remember that the syntax is different than #theme.

Dries’s picture

I'm not a fan of the is_array() check in this patch. I think we should kill those and come up with a more elegant API. David's suggestion in #4 sounds nice, but I don't remember what complexity it would add under the hood. I'm happy to revisit that discussion if you can point me to it.

If there is an elegant fix that does not require is_array() checking, we can proceed with that, and revisit the other issue in parallel.

David_Rothstein’s picture

As far as I can tell from reading the other issue, the only argument against it being an array was related to consistency with #theme -- see http://drupal.org/node/355236#comment-1231073. I'm not aware of any complexity that it would add; as far as I can tell, the only code that would actually have to change "under the hood" is this part of the above patch:

-    $elements['#children'] = theme($elements['#theme_wrapper'], $elements);
+    foreach ($elements['#theme_wrapper'] as $theme_wrapper) {
+      $elements['#children'] = theme($theme_wrapper, $elements);
+    }

If I'm right, that's about as simple a change as you can get :)

Dries’s picture

I suggest we nuke the is_array() test then. Thanks for investigating, David.

Frando’s picture

Yeah, let's kill the is_array() check, make #theme_wrapper always an array and rename it to #theme_wrappers. +1 from me.

Frando’s picture

FileSize
8.32 KB

OK, so here's a patch:
#theme_wrapper is now always an array. No is_array checking needed, as Dries proposed. All places where #theme_wrapper occurs are changed to use arrays. I also updated the documentation to reflect the changes.

David_Rothstein’s picture

FileSize
10.58 KB

Oops, darn it! I actually wrote a patch for this over the weekend while I was on a long train ride, and then, um, forgot to actually post the patch when I got off the train... :(

Well, I guess it's good for reviewing though -- comparing my patch to yours, they're almost exactly the same, which is a good thing! I'm going ahead and posting mine anyway, though, since mine also got the renaming from #theme_wrapper to #theme_wrappers in there, so it's a bit more complete.

Also, my patch includes an isset() check before looping through #theme_wrappers, which I believe is a good idea since it's possible for that property not to be set (?).

Finally, I also merged some of your code comment changes into my patch, since I liked most of yours better :)

Status: Needs review » Needs work

The last submitted patch failed testing.

eaton’s picture

After taking a look at this, I think the use of the #theme_wrapper array is probably a legitimate one. the only difficulty is that it will bubble up our limited ability to control the *order* in which submit/validate/theme-wrapper functions are executed. That may not be a serious issue, but it's something to consider.

Does anyone think there's something to be said for renaming it just #wrapper rather than #theme_wrapper? That's almost certainly outside the scope of this particular issue, but the subtle behavioral differences between #theme (which only executes one function from a stack) and #theme_wrapper (which in this case would execute ALL functions in the array) could be a little disorienting

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
12.63 KB

This should most likely fix the failing tests - the failures happened because recent commits added more calls to #theme_wrapper that were not covered by the previous patch.

Does anyone think there's something to be said for renaming it just #wrapper rather than #theme_wrapper? That's almost certainly outside the scope of this particular issue...

Well, as long as we're renaming it anyway, it might be best to try to settle on a final name here. We can basically change it just by editing the patch file directly. I think I see where you're coming from, but it's still a theme-related property, so I'm not sure about taking 'theme' out of it.

but the subtle behavioral differences between #theme (which only executes one function from a stack) and #theme_wrapper (which in this case would execute ALL functions in the array) could be a little disorienting

That is part of the rationale for renaming it to #theme_wrappers in this patch. The idea is that #theme_wrappers is an array, each element of which is an individual theme wrapper. And each theme wrapper behaves similarly to #theme, just like before (i.e,, it can optionally be an array, only one element of which will be used).

OK, I guess it's still pretty subtle :)

Frando’s picture

Status: Needs review » Reviewed & tested by the community

Great, with eaton agreeing I think this is ready to go in. I like #theme_wrappers, as the wrappers are all theme functions and not regular functions or something else.

All tests pass, I proposed the things this patch adds already in an early version of the first drupal_render theming patch, David_Rothstein and me posted nearly the same patches independently and eaton and Dries also agree, so I think this is RTBC.

Please put it back on CNW if you think we should do more discussions on the name. As I said, IMO #theme_wrappers is perfect.

moshe weitzman’s picture

I'm happy with this patch as well. The array might help us simplify the render structure for a block.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

Frando’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.64 KB

I don't believe the testbot. Reroll attached, no changes. Testbot will set back to CNW if anything fails. Otherwise, this is ready to go in and as moshe said will let us further simplify the page array as well.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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