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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Active » Needs review
FileSize
1.31 KB
dww’s picture

Except I forgot to declare the function to the theme registry in drupal_common_theme(). I actually tested this now and confirmed it works. ;)

chx’s picture

Status: Needs review » Needs work

You tested, great but what about putting that into code? We do not add anything to core these days without simpletests.

dww’s picture

Status: Needs work » Needs review
FileSize
7.87 KB

Updated 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:

Array to string conversion	Notice	bootstrap.inc	1157	drupal_validate_utf8()	
preg_match() expects parameter 2 to be string, array given	Warning	bootstrap.inc	1163	drupal_validate_utf8()	
Array to string conversion	Notice	bootstrap.inc	1157	drupal_validate_utf8()	
preg_match() expects parameter 2 to be string, array given	Warning	bootstrap.inc	1163	drupal_validate_utf8()	
Array to string conversion	Notice	bootstrap.inc	1157	drupal_validate_utf8()	
preg_match() expects parameter 2 to be string, array given	Warning	bootstrap.inc	1163	drupal_validate_utf8()	

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. ;)

Status: Needs review » Needs work

The last submitted patch failed testing.

dww’s picture

Status: Needs work » Postponed
Jacine’s picture

Issue tags: +Needs themer review

This 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 ;)

Jacine’s picture

FileSize
8.36 KB

Here'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. :)

dww’s picture

Status: Postponed » Needs work

#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.

chx’s picture

A 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.

dww’s picture

Status: Needs work » Needs review
FileSize
5.59 KB

Ok, 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.

dww’s picture

At 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.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yeah that test is funny but can be fixed post freezed. Agreed.

Jacine’s picture

FTR, 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?

dww’s picture

@Jacine: Sorry for being terse, lemme try to explain:

In a form API structured array, you can already say things like:

array(
  '#type' => 'select',
  '#attributes' => array('class' => 'something-cool'),
  '#options' => array(0 => t('No'), 1 => t('Yes'),
  ...
);

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

Jacine’s picture

FileSize
1.19 KB

Hi 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 ;)

dww’s picture

@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?

Jacine’s picture

Yes, I can see what you mean now. Sorry, that wasn't my intention.

Don't give Moshe a hard time, ok? LOL ;)

chx’s picture

To clarify , #12 is RTBC and #16 should be moved to a separate issue (and likely deleted from here).

dww’s picture

Rerolled post theme() apocalypse.

dww’s picture

Issue tags: +API clean-up

Oh, and tagging for API clean-up...

webchick’s picture

Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Bleh. :( 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.

dww’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation

Status: Fixed » Closed (fixed)

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

JohnAlbin’s picture

Assigned: dww » Unassigned
Status: Closed (fixed) » Needs review
FileSize
448 bytes

The 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'.

JohnAlbin’s picture

Category: task » bug

To be clear, this is a bug.

casey’s picture

Status: Needs review » Reviewed & tested by the community

Agreed.

Eric_A’s picture

Status: Reviewed & tested by the community » Needs work

(EDIT: Taking it back quickly.)

Eric_A’s picture

sun’s picture

Status: Needs work » Reviewed & tested by the community
+++ modules/simpletest/tests/form.test	9 Oct 2009 20:16:49 -0000
@@ -69,19 +72,34 @@ class FormsTestCase extends DrupalWebTes
+          // Form elements of type 'radios' throw all sorts of PHP notices
+          // when you try to render them like this, so we ignore those for
+          // testing the required marker.
+          // @todo Fix this work-around (http://drupal.org/node/588438).
+          $form_output = ($type == 'radios') ? '' : drupal_render($form);

Ah-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.

sun’s picture

sun’s picture

Version: 7.x-dev » 8.x-dev

Although 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).

dww’s picture

Title: Move required form element marker into its own theme function » form_required_marker() doesn't properly render the form element
Version: 8.x-dev » 7.x-dev

I'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

Eric_A’s picture

Title: form_required_marker() doesn't properly render the form element » form_required_marker() isn't passed the form element (theme_hook definition is wrong)
Component: forms system » theme system
Issue tags: -Needs themer review, -API clean-up
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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