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.
Comment | File | Size | Author |
---|---|---|---|
#17 | theme_wrappers.patch | 11.64 KB | Frando |
#13 | theme_wrappers_array_2.patch | 12.63 KB | David_Rothstein |
#10 | theme_wrappers_array.patch | 10.58 KB | David_Rothstein |
#9 | theme_wrapper.patch | 8.32 KB | Frando |
#1 | fix_form_radios_checkboxes_theming.patch | 1.67 KB | David_Rothstein |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
#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.#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 :)Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedActually 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" :)
Comment #3
Frando CreditAttribution: Frando commentedWell, 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.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedProbably 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.
Comment #5
Dries CreditAttribution: Dries commentedI'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.
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedAs 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:
If I'm right, that's about as simple a change as you can get :)
Comment #7
Dries CreditAttribution: Dries commentedI suggest we nuke the is_array() test then. Thanks for investigating, David.
Comment #8
Frando CreditAttribution: Frando commentedYeah, let's kill the is_array() check, make #theme_wrapper always an array and rename it to #theme_wrappers. +1 from me.
Comment #9
Frando CreditAttribution: Frando commentedOK, 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.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedOops, 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 :)
Comment #12
eaton CreditAttribution: eaton commentedAfter 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
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
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.
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 :)
Comment #14
Frando CreditAttribution: Frando commentedGreat, 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.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedI'm happy with this patch as well. The array might help us simplify the render structure for a block.
Comment #17
Frando CreditAttribution: Frando commentedI 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.
Comment #18
webchickCommitted to HEAD. Thanks!