Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I have no idea if this would be committed at this point in D7's life-cycle, or if it's even a good idea in general... ;) However, I was just working on a form table UI, and I wanted the table headers for required fields in the table to be marked as required fields, even though the elements in the table rows don't have form labels. So, I just had to copy and paste the code from out of core's theme_form_element(). Seems like it'd be nice if this little bit was its own theme function so that it could be reliably reused by other customers. Stay tuned for a patch.
Comment | File | Size | Author |
---|---|---|---|
#25 | theme-form-required-marker-582584-25.patch | 448 bytes | JohnAlbin |
#20 | 582584-20.theme_form_required_marker.patch | 5.77 KB | dww |
#16 | attributes.diff | 1.19 KB | Jacine |
#12 | 582584-12.theme_form_required_marker.patch | 5.66 KB | dww |
#11 | 582584-11.theme_form_required_marker.patch | 5.59 KB | dww |
Comments
Comment #1
dwwComment #2
dwwExcept I forgot to declare the function to the theme registry in drupal_common_theme(). I actually tested this now and confirmed it works. ;)
Comment #3
chx CreditAttribution: chx commentedYou tested, great but what about putting that into code? We do not add anything to core these days without simpletests.
Comment #4
dwwUpdated FormsTestCase::testRequiredFields() to handle this case. For each sample form element, it now processes the form twice, once with the element required and once without. It uses preg_match() to find the marker inside the
<label>
, right next to the closing</label>
.However, for some reason I don't yet understand, calling drupal_render() on the form when it's of #type 'radios' generates a bunch of PHP notices:
Trying to modify this test to use drupal_build_form() instead of drupal_process_form() is a pain since that'd require form builder functions. I don't understand why radios are "special" in this way. Maybe that's a FAPI bug. ;)
Comment #6
dwwBlocked on cleaning up #582956: FormsTestCase::testRequiredFields() is broken in various ways first...
Comment #7
JacineThis is a good idea. I think it would need the $element attribute though, so the themer can have context in case the markup needs to be different per element type or whatever. If not, it might be better to leave it as it is. Tagging "needs theme review" so it's on our radar ;)
Comment #8
JacineHere's a modified version of patch that passes through $element and also uses drupal_attributes(). I know you've got it postponed, but I figured I'd post before I forget. :)
Comment #9
dww#582956: FormsTestCase::testRequiredFields() is broken in various ways is in, so this can move forward again. I'll try to reroll later this afternoon to merge in Jacine's changes and see if I can get the tests working properly now.
Comment #10
chx CreditAttribution: chx commentedA child called attributes? Whom are you trying to confuse? We let that in and sun opens a DrupalWTF issue before you can rejoice that your patch got in.
Comment #11
dwwOk, I'm still passing in $element on the off chance someone wants that, and I'm okay using drupal_attributes(), but the way it was done in the patch in #8 is very bizarre. No one had any control over this markup before, now you get some. If you want to further complicate this, please do so in a new patch for D8 -- I'd actually like to see this get in D7... ;)
Note, the tests are still slightly fubar. When you try to render one of these dummy "empty" radios form elements, you get PHP notices. That's basically because of the unholy way these FAPI tests work at all. For now, I'm just ignoring the problematic case. It's not a bug in FAPI nor this patch, it's a problem with the tests themselves, but it's too big a task to completely rewrite the FAPI tests just for this trivial patch.
Comment #12
dwwAt chx's request, I submitted #588438: FormTest::testRequiredFields() fails to drupal_render() elements of #type 'radios' and added a @todo code comment pointing to it.
Comment #13
chx CreditAttribution: chx commentedYeah that test is funny but can be fixed post freezed. Agreed.
Comment #14
JacineFTR, I wasn't trying to "complicate" anything. I don't get what is "bizarre", but maybe that's because I am a themer who will actually use this function. Anyway, I don't have any objection as long as $element is there. I see it's being passed it, but it's not in the theme hook arguments. Maybe it's not needed there?
Comment #15
dww@Jacine: Sorry for being terse, lemme try to explain:
In a form API structured array, you can already say things like:
In that case, the form element rendered would include class="something-cool". Your patch introduced the notion of some special child subelement called 'attributes' (no '#'). So, it's a "WTF?!" because in this one weird case, you can define both '#attributes' (for attributes on the form element itself) and 'attributes' (for attributes on a part of the rendered form element). That's not very friendly for themers *or* developers.
Generally speaking, anything that starts with an '#' sign is an attribute of the form element itself, and anything else is a nested form element. For example, that's how fieldsets work -- the outer array defines the fieldset properties with things like '#type', '#title', '#collapsible', etc, and then everything else (e.g. 'setting1', etc) without the '#' sign, are child form elements, not other properties of the parent element.
Does that all make sense?
Cheers,
-Derek
Comment #16
JacineHi Derek,
Ah yes didn't realize that was the source of the WTF. Thanks for taking the time to respond, I appreciate it ;)
Maybe I should have explained more of where I was coming from. Since #493746: Enhance drupal_attributes() for multiple valued values and #400292: Allow preprocess functions for theme hooks implemented as functions went in, I've wanted to take on cleaning up all these theme functions (even though it's probably over my head), so that they are in arrays and the functions themselves allow a place to plug-in these attributes (to the wrapper markup, not the element itself). I sat with w/Moshe at Drupalcon in Paris and he helped me with a patch for theme_form_element() as an example of somewhere to start, and we used $element['attributes']['class'] to replace all the $class and
implode(' ', $class)
nonsense (see attachment). So, basically since this is a new function, I figured that would be a better way to do it.Maybe it's still not correct, but hopefully it's less bizarre ;)
Comment #17
dww@Jacine: ok, fair enough. I'll be sure to give Moshe a hard time about this when I next get the chance. ;) Anyway, to put it mildly, there are "problems" with doing it that way, but this isn't the place to hash those out. I understand the desire to allow people to alter the attributes on the nested theme function via the form array definition, but that's what I mean by "complication." That's a new feature, and we're sort of past the new feature stage for D7. This patch is a simple reorganization (you might even call it a bug fix) to make it possible to control the markup for this marker at all. Introducing a whole new FAPI array definition convention for these nested theme functions is out of scope for this issue, and perhaps out of scope for D7 at all at this point. See what I mean?
Comment #18
JacineYes, I can see what you mean now. Sorry, that wasn't my intention.
Don't give Moshe a hard time, ok? LOL ;)
Comment #19
chx CreditAttribution: chx commentedTo clarify , #12 is RTBC and #16 should be moved to a separate issue (and likely deleted from here).
Comment #20
dwwRerolled post theme() apocalypse.
Comment #21
dwwOh, and tagging for API clean-up...
Comment #22
webchickBleh. :( I really don't like committing this with that @todo in it. But trying to get my API change queue cleared up, and agree that it's not really for this issue to solve brain-dead tests...
Committed to HEAD. This API change needs to be documented in the theme upgrade guide.
Comment #23
dwwThanks for the commit. Docs added:
http://drupal.org/node/254940/revisions/view/761826/764726
Comment #25
JohnAlbinThe patch in #20 was rolled before #600974: theme() fails for theme functions taking one argument in renderable arrays landed, but commited after it. So the theme_hook definition of form_required_marker is wrong and the renderable element isn't actually passed to the theme function. But since the default theme function doesn't actually use that $element, no one noticed.
This patch just fixes the drupal_common_theme() definition of form_required_marker to use 'render element' instead of 'arguments'.
Comment #26
JohnAlbinTo be clear, this is a bug.
Comment #27
casey CreditAttribution: casey commentedAgreed.
Comment #28
Eric_A CreditAttribution: Eric_A commented(EDIT: Taking it back quickly.)
Comment #29
Eric_A CreditAttribution: Eric_A commentedComment #30
sunAh-ha! Now I finally learned where this got introduced! (sorry, didn't even think once to click that link... do you know that comments should explain actual problems instead of linking to arbitrary d.o issues? :P)
Patch should still apply.
Powered by Dreditor.
Comment #31
sun#25: theme-form-required-marker-582584-25.patch queued for re-testing.
Comment #32
sunAlthough badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).
Comment #33
dwwI'm confused. I didn't think D8 was even open for development yet. There's a bug in D7 introduced by two patches going in at roughly the same time that conflicted with each other, as per #25. The original point of this issue is already in D7. It's just broken. So we should fix it.
Re: "do you know that comments should explain actual problems instead of linking to arbitrary d.o issues?"
I don't agree. A d.o issue represents a full discussion on a bug. CS teaches us the value of a level of indirection. Instead of trying to duplicate the info in a paragraph worth of comment in the code, we can just link to the full discussion. I see nothing wrong with this practice, although this issue isn't the place to have that debate. ;)
Cheers,
-Derek
Comment #34
Eric_A CreditAttribution: Eric_A commentedComment #35
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.