Updated: #175
From Bowersox on #171
Status Summary
In comment 120, @webchick committed the D8 patch. This added the new API, which is good. Folks started working on the D7 backport. But @David_Rothstein pointed out that the committed patch had broken form validation in D8. David suggested that we fix the order that form validation happens in comments 126 and 127. But @sun suggested an alternative work-around in 128 and 140. Currently this is messed up in D8 and we need to implement a solution. The D7 backport discussion may resume later.

New Patch
I support the @David_Rothstein approach, which means doing the required field validation before the #element-validate validation. The new patch attached basically re-orders the logic to restore the order. This could use a code review and a test to make sure the example David points out gets the right error message. andymartha provided screenshots that the patch applies cleanly with error messages tailored to the required field.

Problem/Motivation

Accessibility guidelines recommend specific, meaningful error messages when required form elements are empty. However, FAPI currently does not support custom error messages for required elements.

Proposed resolution

  1. Support a #required_error property for form elements. If set, this message will be used when a required form element is empty, instead of the default "Field x is required."
  2. Additionally set a #required_but_empty flag automatically during validation when the required field is empty. This is an internal property and is not set manually. It allows #element_validate handlers to set a custom required error message, but without havingto re-implement the complex logic to figure out whether the field value is empty.

Remaining tasks

Commit feature and document these properties. The feature is a small API improvement that is applicable to both D8 and D7.

User interface changes

None.

API changes

Minor, backwards-compatible addition only. No existing code will be affected.

Addition of two FAPI element properties:

  1. #required_error
  2. #required_but_empty

Original report by neilnz

Hi,

In a couple of cases working on forms that need to meet government accessibility guidelines, I've been asked by a client to implement specific help text that is shown when a form field is not completed that replaces the generic "[fieldname] field is required". An example might be changing "Password field is required" to "Please enter your password" on the login form.

I know that the error message template can be overridden on a general level to say something different (like "Please fill in [fieldname]"), but it's not easily possible to set it for one specific instance of a field at present.

There are four current solutions to this problem, two of which require core hacks:

  1. My core patch to form.inc, as attached, which adds a new #required_message element to the form definition which is used to override the default message for that form field. This can be applied using hook_form_alter() or used directly in a custom form definition.
  2. Another approach is a different patch to form.inc which wraps the first t() around the error with a second t(), so the message with substituted field name can be overridden by a module like string overrides. Possibly this could be supported more generically by the t() function by scanning for a second translation after the substitution is performed.
  3. Use hook_form_alter() to set #required to false and add a new #validate callback for the element, which sets an appropriate message (obviously can do this natively on a custom form)
  4. Use hook_form_alter() to add a validate callback that uses form_set_error(NULL, '', true) to get and clear errors, parse the errors, replace a particular error message from core, and then store the resulting error array back again (if any).

I guess regexp at the theme/preprocess layer could be used as well, but that's a bit ugly.

At the moment I'm using option 3, which works alright except it removes the asterisk from the field, so this needs to be hacked back in at the theme layer.

The potential for my patch is to make this support a bit more widespread, eg. in a field config form, the user can tick the "Required" option for the field, but also gets an optional textbox to replace the standard error for that field.

I'm submitting this mostly as an RFC to see if it's worthwhile. If so, I'm happy to finish off the support by patching the form API docs etc as well.

Patch is against HEAD.

Thanks

Files: 
CommentFileSizeAuthor
#184 drupal8-formRequiredEmptyValidate-742344-184.patch9.25 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] 59,743 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#180 drupal8-formRequiredEmptyValidate-742344-180.patch11.52 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-formRequiredEmptyValidate-742344-180.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#175 742344before1.png48.64 KBandymartha
#175 742344after1.png51.18 KBandymartha
#175 742344after2.png64.06 KBandymartha
#173 drupal8-formRequiredEmptyValidate-742344-173.patch11.51 KBbowersox
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-formRequiredEmptyValidate-742344-173.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#171 drupal8.form-required-empty-validate.707484.171.patch2.3 KBbowersox
FAILED: [[SimpleTest]]: [MySQL] 57,405 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#157 interdiff-128-154.txt1.18 KBmgifford
#156 Screen Shot 2013-03-27 at 10.31.48 AM.png109.97 KBmgifford
#154 drupal8.form-required-empty-validate.154.patch2.03 KBmicnap
PASSED: [[SimpleTest]]: [MySQL] 56,938 pass(es).
[ View ]
#151 drupal8.form-required-empty-validate.148.patch1.96 KBmicnap
FAILED: [[SimpleTest]]: [MySQL] 53,392 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#149 drupal8.form-required-empty-validate.148.patch1.96 KBmicnap
FAILED: [[SimpleTest]]: [MySQL] 53,350 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#147 drupal8.form-required-empty-validate.128.patch1.96 KBmicnap
FAILED: [[SimpleTest]]: [MySQL] 53,271 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#137 drupal-form-required-fields-742344-137.patch4.67 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#131 drupal8-form_required_empty-execution_order-742344-131.patch1.53 KBlucascaro
FAILED: [[SimpleTest]]: [MySQL] 49,715 pass(es), 7 fail(s), and 3 exception(s).
[ View ]
#128 drupal8.form-required-empty-validate.128.patch1.96 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,771 pass(es), 41 fail(s), and 0 exception(s).
[ View ]
#122 drupal-form-required-fields-742344-122-tests+fix.patch7.8 KBDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 39,503 pass(es).
[ View ]
#113 drupal-form-required-fields-742344-113-tests+fix.patch8.65 KBlucascaro
PASSED: [[SimpleTest]]: [MySQL] 41,329 pass(es).
[ View ]
#113 interdiff.txt597 byteslucascaro
#106 drupal-form-required-fields-742344-106-testsonly.patch6.78 KBtarmstrong
FAILED: [[SimpleTest]]: [MySQL] 37,026 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#106 drupal-form-required-fields-742344-106-tests+fix.patch9.41 KBtarmstrong
PASSED: [[SimpleTest]]: [MySQL] 40,649 pass(es).
[ View ]
#103 drupal-form-required-fields-742344-103-testsonly.patch5.72 KBtarmstrong
FAILED: [[SimpleTest]]: [MySQL] 37,027 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#103 drupal-form-required-fields-742344-103-tests+fix.patch8.35 KBtarmstrong
FAILED: [[SimpleTest]]: [MySQL] 37,051 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#99 drupal-form-required-fields-742344-99-testsonly.patch5.73 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 35,635 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#99 drupal-form-required-fields-742344-99-tests+fix.patch8.42 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] 35,662 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#97 drupal-form-required-fields-742344-97.patch8.54 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/simpletest/tests/form_test.module.
[ View ]
#94 drupal-form-required-custom-742344-94.patch7.77 KBwamilton
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-form-required-custom-742344-94.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#91 drupal-form-required-custom-742344-91.patch8.74 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,073 pass(es).
[ View ]
#85 drupal-form-required-custom-742344-85.patch8.13 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,656 pass(es).
[ View ]
#58 drupal.form-required-custom.58.patch7.82 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.58.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#52 drupal.form-required-custom.52.patch7.13 KBsun
PASSED: [[SimpleTest]]: [MySQL] 26,486 pass(es).
[ View ]
#51 drupal.form-required-custom.50.tests-only.patch5.62 KBsun
FAILED: [[SimpleTest]]: [MySQL] 26,478 pass(es), 20 fail(s), and 0 exception(es).
[ View ]
#44 drupal.form-required-custom.44.patch1.95 KBsun
PASSED: [[SimpleTest]]: [MySQL] 26,019 pass(es).
[ View ]
#41 drupal.form-required-custom.41.patch13.72 KBsun
PASSED: [[SimpleTest]]: [MySQL] 25,976 pass(es).
[ View ]
#35 drupal.form-required-custom.35.patch13.36 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.35.patch.
[ View ]
#27 drupal.form-required-custom.27.patch10.48 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.27.patch.
[ View ]
#21 drupal.form-required-custom.21.patch10.46 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.21.patch.
[ View ]
#20 drupal.form-required-custom.20.patch10.45 KBsun
PASSED: [[SimpleTest]]: [MySQL] 23,329 pass(es).
[ View ]
#18 drupal.form-required-custom.18.patch10.85 KBsun
FAILED: [[SimpleTest]]: [MySQL] 23,282 pass(es), 21 fail(s), and 27 exception(es).
[ View ]
#13 drupal.form-required-custom.separate.13.patch8.83 KBsun
FAILED: [[SimpleTest]]: [MySQL] 22,757 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#13 drupal.form-required-custom.re-use.13.patch8.69 KBsun
FAILED: [[SimpleTest]]: [MySQL] 22,762 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#11 drupal.form-required-custom.11.patch5.82 KBsun
PASSED: [[SimpleTest]]: [MySQL] 22,746 pass(es).
[ View ]
#8 drupal.form-required-custom.8.patch5.83 KBsun
FAILED: [[SimpleTest]]: [MySQL] 22,742 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#4 form.inc_.customrequired.cvshead.patch1.04 KBneilnz
PASSED: [[SimpleTest]]: [MySQL] 18,729 pass(es).
[ View ]
form.inc_.customrequired.d7.patch1 KBneilnz
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc_.customrequired.d7.patch.
[ View ]

Comments

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

I see D7 in the patch so I guess this a drupal 7 patch...

Priority:Minor» Normal
Issue tags:+accessibility

@neilnz

Glad to see that you raised this issue. Can you point us to the accessibility standard and specific criteria that you are trying to satisfy, it might be helpful for brainstorming alternative methods of meeting the criteria.

Would a appropriately composed field description be sufficient to satisfy this criteria?

Are you developing the site with D7 or D6? It is unlikely that this change will make it into D6 or D7 at this time, but it is definitely worth us discussing.

Status:Needs review» Needs work

The last submitted patch, form.inc_.customrequired.d7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 18,729 pass(es).
[ View ]

@aspilicious - That's sort-of true. Currently (AFAIK?) HEAD is still D7, but D7 is closed for feature requests, therefore it's a proposal for D8.

@Everett - For Drupal + NZGWS in general there's some notes at http://huntdesign.co.nz/drupalsouth/nz-govt-web-standards-and-drupal. Recently we switched from proprietary guidelines to ones based on extended WCAG 2.0. The particular part that's waved in front of me is http://www.webstandards.govt.nz/standards-summary/ part 3.3.1, for which the original definition from WCAG is http://www.w3.org/TR/2008/REC-WCAG20-20081211/#minimize-error-identified

It's a bit of a fuzzy test, but on audit I've been told that Drupal needs to provide an anchor link to jump to the errant element, and should provide more meaningful errors (in some cases) than the defaults. The anchor link problem still remains (since the form errors aren't listed in a form that contains HTML ID information about their target), so might be something to solve in Drupal as well.

[edit] Oh sorry, forgot to finish answering you @Everett - I'm working with D6 here, and we already use descriptions on elements to provide assistance. In most circumstances the description is sufficient to explain why a field is required etc, but it's more the problem that on top of that, a standardised error message doesn't necessarily fit all field instances.

PS: Reattaching the patch in CVS diff format to appease the testbot.

subscribe

@Everett - I'm sure I already replied to this. Oh well.

Yes that does look interesting. It solves the jump-to-element problem I guess.

It also looks like it's affecting the same area of code as my patch, so they might conflict a bit.

But my patch still offers different functionality to that one anyway.

Title:Provide support for custom form element #required messages per fieldSupport custom #required form error messages per element
Category:feature» task
StatusFileSize
new5.83 KB
FAILED: [[SimpleTest]]: [MySQL] 22,742 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Alternatively, we could simply re-use #required?

<?php
  $form
['regular'] = array(
   
'#title' => t('Regular'),
   
'#required' => TRUE,
   
'#description' => t('Outputs "Regular field is required."'),
  );
 
$form['custom'] = array(
   
'#title' => t('Custom'),
   
'#required' => 'You need to fill in a value for !title field.',
   
'#description' => t('Outputs "You need to fill in a value for Custom field."'),
  );
?>

Category:task» feature
Issue tags:+API change

Both a feature request and an API change... That said, this is useful, and the patch is tiny.

Status:Needs review» Needs work

The last submitted patch, drupal.form-required-custom.8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.82 KB
PASSED: [[SimpleTest]]: [MySQL] 22,746 pass(es).
[ View ]

Fixed that typo.

Taking into account #875722: Custom #required messages broken, I'd consider this anything but a feature request ;)

Anyway, I think this is almost done already. We just need to decide on the way forward, whether

A) simply re-using the existing #required property (assigning a error message string instead of TRUE, which uses the default error string), or

B) adding a new optional #required_message property as in earlier patches in this issue.

Tests could be merged and quickly adapted from aforementioned issue.

Regardless of A or B, we should change the string from !name to !title as already done in my patch, since "!name" basically implies that we'd replace it with #name, not #title, which is not the case.

Assigned:Unassigned» sun
StatusFileSize
new8.69 KB
FAILED: [[SimpleTest]]: [MySQL] 22,762 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
new8.83 KB
FAILED: [[SimpleTest]]: [MySQL] 22,757 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

I really don't care, as long as we fix this bug.

Both options attached.

I think you can just set #validated => TRUE on a #required element and then provide your own required testing / error.

This is what I did in this patch: http://drupal.org/node/266488#comment-3277916

I can't decide whether I like A or B better.

Something we should do in Drupal 8 is combine all non-customized #required errors into a single message, e.g.

Fields username and password are required.

Status:Needs review» Needs work

The last submitted patch, drupal.form-required-custom.re-use.13.patch, failed testing.

Patch can be re-rolled quickly. I need and want to know which variant though. @Damien? @chx?

Status:Needs work» Needs review
StatusFileSize
new10.85 KB
FAILED: [[SimpleTest]]: [MySQL] 23,282 pass(es), 21 fail(s), and 27 exception(es).
[ View ]

Let's use a separate property. That's cleaner. This patch should pass. Had to fix another Form API bug to make the tests work.

Status:Needs review» Needs work

The last submitted patch, drupal.form-required-custom.18.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+API addition
StatusFileSize
new10.45 KB
PASSED: [[SimpleTest]]: [MySQL] 23,329 pass(es).
[ View ]

Title:Support custom #required form error messages per elementCustom #required form error messages per element
StatusFileSize
new10.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.21.patch.
[ View ]

Re-rolled against HEAD + simplified.

This looks ready to fly for me.

Title:Custom #required form error messages per element#required_error for custom form element error messages
Issue tags:-API change+Usability

No API change here.

Status:Needs review» Reviewed & tested by the community

nice feature :D

sub

Status:Reviewed & tested by the community» Needs work
Issue tags:+Usability, +accessibility, +API addition

The last submitted patch, drupal.form-required-custom.21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.27.patch.
[ View ]

Re-rolled against HEAD.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal.form-required-custom.27.patch, failed testing.

Status:Needs work» Needs review

#27: drupal.form-required-custom.27.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +accessibility, +API addition

The last submitted patch, drupal.form-required-custom.27.patch, failed testing.

Version:7.x-dev» 8.x-dev
Assigned:sun» Unassigned

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

Is it possible that we can bring this functionality into http://drupal.org/project/accessible

Title:#required_error for custom form element error messagesVarious forms try to set a custom validation error for #required elements, but that is not possible
Version:8.x-dev» 7.x-dev
Assigned:Unassigned» sun
Category:feature» bug
Status:Needs work» Needs review
StatusFileSize
new13.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.35.patch.
[ View ]

Re-rolled against HEAD. Better title, proper category.

Status:Needs review» Reviewed & tested by the community

This looks good to me @sun. Thanks for adjusting the settings and re-rolling the patch.

I didn't grab any screenshots, but I navigated the site looking for differences & didn't find any (which is the desired result).

Status:Reviewed & tested by the community» Needs work

Needs to be re-rolled against HEAD.

Status:Needs work» Needs review

#35: drupal.form-required-custom.35.patch queued for re-testing.

Again? Ok.. Just retesting it. I had applied it just fine an hour ago.

Status:Needs review» Needs work

The last submitted patch, drupal.form-required-custom.35.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.72 KB
PASSED: [[SimpleTest]]: [MySQL] 25,976 pass(es).
[ View ]

Re-rolled against HEAD.

Status:Needs review» Reviewed & tested by the community

thanks sun. Think we're good to go here. This patch applies nicely against the CVS & seems to be working just fine in my sandbox.

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

Sorry, but this is too late. Looks like a nice improvement for D8, though. And yes, I realize it doesn't break APIs, but it does change APIs (you either can now enter either a boolean or a string for #required or a else the addition of a new #required_message property) and invalidates documentation.

Seems to me something like hook_element_info_alter() could add a process function to handle this in contrib in the accessibility project in the meantime.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.95 KB
PASSED: [[SimpleTest]]: [MySQL] 26,019 pass(es).
[ View ]

Seems to me something like hook_element_info_alter() could add a process function to handle this in contrib in the accessibility project in the meantime.

Nope, and that's the point of this patch. _form_validate() preemptively checks whether a form element is #required, and if it is, and it has no value, then it sets a form error on the element's #parents. Since form_set_error() works like it works, it only allows one error, the first error, to be set on an element. Therefore, any attempt to set a custom form validation error for any #required field is simply not taken into account, because _form_validate() has already set an error on the element.

This is an API bug in Form API. Not sure why we're moving bugs to D8... :P ;)

However, all of that being said, you're simply genius! :-D Why introduce something new if the API has to be fixed in reality? :)

...it's perfectly possible that this patch will fail, since there are various form validation and form element validation handlers throughout core that *try* to set a custom required error message, but since that is not possible, tests may blatantly assume the default error message.

Well that looks much nicer, if it works. Doesn't change core's default UI nor developer-facing Form API.

And it passed. kk, could use a line of reasoning in _form_validate() + also a test that proves that the patch actually fixes something ;)

@sun - thanks for arguing that this be a D7 issue.

I applied the patch here http://drupal7.dev.openconcept.ca/user

It seems to work well. What needs to be done to make this patch RTBC & fix this bug in core?

sub

StatusFileSize
new5.62 KB
FAILED: [[SimpleTest]]: [MySQL] 26,478 pass(es), 20 fail(s), and 0 exception(es).
[ View ]

StatusFileSize
new7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 26,486 pass(es).
[ View ]

Cleaner, better, rock solid.

@webchick:

+++ includes/form.inc 23 Oct 2010 14:15:21 -0000
@@ -1252,6 +1247,24 @@ function _form_validate(&$elements, &$fo
+      if (isset($elements['#title'])) {
+        form_error($elements, $t('!name field is required.', array('!name' => $elements['#title'])));
+      }
+++ modules/simpletest/tests/form_test.module 23 Oct 2010 14:10:42 -0000
@@ -304,6 +310,48 @@ function form_test_validate_form_validat
+  $form['textfield'] = array(
+    '#type' => 'textfield',
+    '#title' => 'Name',
+    '#required' => TRUE,
+    '#required_error' => t('Please enter a name.'),
+    '#element_validate' => array('form_test_validate_required_form_element_validate'),
...
+function form_test_validate_required_form_element_validate($element, &$form_state) {
+  // Set a custom validation error on the #required element.
+  if (!empty($element['#required_is_empty'])) {
+    form_error($element, $element['#required_error']);
+  }

As visible in the _form_validate() code, it would be a 3 line change to optionally support a custom #required_error message, by checking isset($elements['#required_error']) before checking isset($elements['#title']). I.e., 3 lines that have a very large effect and benefit. With that additional check, most forms wouldn't have to implement a custom #element_validate handler to throw a custom error message, but instead just set it in #required_error.

I do not see a reason why we couldn't add that condition, as it's a very tiny API addition only. Would be happy to re-roll and extend the tests accordingly, but need to know upfront whether that addition will be rejected.

Powered by Dreditor.

That seems like a valuable ux improvement for little risk. +1

Tagging for the question in #53.

I'm nowhere near adept enough to review the actual code of the patch, but this would be a great help for accessibility, and would be great to get it. If it doesn't get in, though, can #447816: WCAG violation: Relying on a color by itself to indicate a field validation error at least get in since that was RTBC a long while ago. (It is red now because of @sun's review.)

Category:bug» support

Ok, so to test this I would

Add something like this to a form that I want to make manditory:

    '#required' => TRUE,
    '#required_error' => t('Please select something.'),
    '#element_validate' => array('form_test_validate_required_form_element_validate'),

And then add in a customized function:

function form_test_validate_required_form_element_validate($element, &$form_state) {
  // Set a custom validation error on the #required element.
  if (!empty($element['#required_is_empty'])) {
    form_error($element, $element['#required_error']);
  }
}

The code looks fine. My site continues to work fine. I'm just not sure about how to add in a test case so I have a custom required error show up.

I'd like to be able to mark this RTBC.

Category:support» bug
StatusFileSize
new7.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.form-required-custom.58.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Attached patch performs the 3 line change and thus, optionally supports to set #required_error on a form element.

Status:Needs review» Reviewed & tested by the community

Looks good @sun. I applied this to my sandbox and tested the standard errors.

Your test now runs through a custom error #form_test_required_error, so I'm willing to mark this RTBC now as that was what was holding me back before.

We'll need to get that documented when this gets into core.

Good idea to create #required_is_empty. That will also be helpful in this patch: #447816: WCAG violation: Relying on a color by itself to indicate a field validation error.

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

Sorry, this doesn't represent a critical regression and API changes are off the table now. There are workarounds, though they're hacky, so that'll get us by for D7.

Version:8.x-dev» 7.x-dev
Priority:Normal» Major

Given #811542: Regression: Required radios throw illegal choice error when none selected which is a (critical?) regression, and that Sun's patch in #58 is the best solution for it and is tiny, the vast majority is comments and tests, i think it should be committed against D7.

Is #811542: Regression: Required radios throw illegal choice error when none selected actually a regression, though - I agree that it is weird, and sun's logic at #8 for that issue makes sense. However, I think that this problem existed in D6, as well.

Also, @mlncn, did you test the patch?

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

Let's keep this issue at 8.x

The only fix we actually need for the radios is the array_key_exists() part + the "default_values" => NULL part. It's two lines of code.

We are days away from a release, let's keep it simple.

@bojanz: Care to post that comment (or even better, a patch) at the #811542: Regression: Required radios throw illegal choice error when none selected issue? I would do so, but I don't understand FAPI well enough to know precisely what is needed.

@EvanDonovan
Already did (yesterday), see #11 / #12.

Sorry, didn't see the latest posts.

#58: drupal.form-required-custom.58.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal.form-required-custom.58.patch, failed testing.

Status:Needs work» Needs review

#58: drupal.form-required-custom.58.patch queued for re-testing.

Category:bug» task

Based on my patch, I think this wasn't possible in D6 either, so let's improve this for D8.

#58: drupal.form-required-custom.58.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Fantastic patch. I've had dozens of requests for this in the Webform queue. Still applies now. I've tested it and confirmed it works as advertised manually (though obviously sun's patch ensure it works through testing). This patch is a trivial change. I was skeptical of the #required_is_empty property (which is internal to core and advanced #element_validate functions, never set manually), until I read all the documentation above it. Basically it's an optional enhancement, but without it every module would have to manually check if it's an array value, a string, or some other empty value. Makes sense to me and I'd appreciate the improvement on top of fixing the larger problem of just setting a custom required message.

Still passes tests, applies cleanly, works when tested manually, and I love the change. This should have been RTBC months ago.

Issue tags:+needs backport to D7

This looks backportable to me, only an API addition.

Added issue summary.

Title:Various forms try to set a custom validation error for #required elements, but that is not possibleAllow forms to set custom validation error messages on required fields

+++ includes/form.inc 20 Nov 2010 00:59:09 -0000
@@ -1271,6 +1266,27 @@ function _form_validate(&$elements, &$fo
+    if (isset($is_empty_value) && ($is_empty_multiple || $is_empty_string || $is_empty_value)) {

This is really an esoteric question :-), hence leaving RTBC, but could this not simply be if (!empty($element['#required_is_empty']))?

+++ includes/form.inc 20 Nov 2010 00:59:09 -0000
@@ -1271,6 +1266,27 @@ function _form_validate(&$elements, &$fo
+    // Ensure that a #required form error is thrown, regardless of whether
+    // #element_validate handlers changed any properties. If $is_empty_value
+    // is defined, then above #required validation code ran, so the other
+    // variables are also known to be defined and we can test them again.
+    if (isset($is_empty_value) && ($is_empty_multiple || $is_empty_string || $is_empty_value)) {

There is this extremely verbose comment right above the line in question, which attempts to explain why we're not simply checking for #required_is_empty. Perhaps it's not clear enough?

4 days to next Drupal core point release.

Ahhhh....
I was so happy I understood the second sentence, I overlooked the first one.
Rereading it, the comment is pretty clear, I guess.
Don't really know what the use-case of unsetting #required_is_empty in an #element_validate function is (and wouldn't you expect to be able to eliminate the validation error then?), but that is reeeeeally (!!!) edge-case-y, so I don't really care.

Either way, this is sooo RTBC...

Issue summary:View changes

Added issue summary.

Issue summary:View changes

Clearer indication that it is an API addition only.

patch from comment #58 applies to drupal 7.x-dev and passes all the test

#58: drupal.form-required-custom.58.patch queued for re-testing.

StatusFileSize
new8.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,656 pass(es).
[ View ]

Re-rolled patch from #58 to correct fuzz. No code changes.

sub

I can see it being useful to for the docs to have a good example of how to use this. Applying the patch is easy, but do we have an example we can use in the testing with a custom validation messages. Say, we wanted the error messages to come up in Pirate or Klingon for instance, how would we insert that?

+    '#required' => TRUE,
+    '#required_error' => t('Please enter a name.'),

First example using the #required_error property.

+    '#required' => TRUE,
+    '#form_test_required_error' => t('Please choose at least one option.'),
+    '#element_validate' => array('form_test_validate_required_form_element_validate'),
...
+function form_test_validate_required_form_element_validate($element, &$form_state) {
+  // Set a custom validation error on the #required element.
+  if (!empty($element['#required_is_empty']) && isset($element['#form_test_required_error'])) {
+    form_error($element, $element['#form_test_required_error']);
+  }
+}

Second example using an #element_validate handler that sets a custom form error message. For testing purposes, it uses a predefined string value of a property on the element, but obviously, you can set a custom error message in Klingon here. (Note however, that both Pirate talk and Klingon are languages, so these are invalid examples, since you should use localized strings for them instead.)

12 days to next Drupal core point release.

I think #required_is_empty is a confusingly named flag. At first glance I'd imagine it was empty($element['#required']).

A more readable name might be #required_and_empty or #required_but_empty to indicate that really it is for elements which are both #required and left empty.

Status:Reviewed & tested by the community» Needs work

I don't understand why we need #form_test_required_error when the purpose of the test is to test #form_is_empty? As far as I can tell, #form_test_required_error has nothing to do with this patch.

Can't we make the test more specific and make it clear that we're testing #form_is_empty? It would make for a better, more readable and more targetted test in my opinion.

Issue summary:View changes

Even clearer.

Status:Needs review» Needs work
StatusFileSize
new8.74 KB
PASSED: [[SimpleTest]]: [MySQL] 33,073 pass(es).
[ View ]

@bfroehle:

A more readable name might be ... #required_but_empty to indicate that really it is for elements which are both #required and left empty.

I agree; so modified. Also, this flag needs to be documented somewhere besides in an inline comment. At the very least, an API change node should be written. Tagging "Needs Documentation" for now.

@Dries:

I don't understand why we need #form_test_required_error when the purpose of the test is to test #form_is_empty?

What test are you looking at? In the test that contains #form_test_required_error, the purpose is:

  /**
   * Tests #required with custom validation errors.
   *
   * @see form_test_validate_required_form()
   */
  function testCustomRequiredError() {

@sun:

In the diff to includes/form.inc:

// Although discouraged, a #title is not mandatory for form elements. In
// case there is no #title, we cannot set a form error message.
// Instead of setting no #title, form constructors are encouraged to set
// #title_display to 'invisible' to improve accessibility.

I think you meant to say "encouraged" rather than "discouraged". Reworded as follows:

// A #title is not mandatory for form elements, but without it we cannot
// set a form error message. So when a visible title is undesirable, form
// constructors are encouraged to set #title anyway, and then set
// #title_display to 'invisible'. This improves accessibility.

I hope that similar text will be added to the Forms API reference and/or the Form generation overview.

Adding it to Develop for Drupal / User interface standards / Interface patterns / Form elements, even though that page is marked Drupal 6.x and Needs updating.

Status:Needs work» Needs review
Issue tags:+Needs Documentation

CNR for bot because of the change from #required_is_empty to #required_but_empty.

Also updated issue summary and added an API change node.

Assigned:sun» Unassigned
Issue tags:+Novice

This will need a reroll for the core directory change, etc. Tagging novice to reroll the patch.

Status:Needs work» Needs review
StatusFileSize
new7.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-form-required-custom-742344-94.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled for nothing but dir changes and one hunk was looking for trailing whitespace that had already been fixed. Using sed continues to rock.

The last submitted patch, drupal-form-required-custom-742344-94.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.54 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/simpletest/tests/form_test.module.
[ View ]

Re-rolled to correct for fuzz.

Status:Needs review» Needs work

The last submitted patch, drupal-form-required-fields-742344-97.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.42 KB
FAILED: [[SimpleTest]]: [MySQL] 35,662 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new5.73 KB
FAILED: [[SimpleTest]]: [MySQL] 35,635 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Corrected, I think.

testsonly should fail.

tests+fix should pass.

Status:Needs review» Needs work

The last submitted patch, drupal-form-required-fields-742344-99-tests+fix.patch, failed testing.

Issue tags:+a11ySprint

Tagging for sprint

Assigned:Unassigned» tarmstrong

StatusFileSize
new8.35 KB
FAILED: [[SimpleTest]]: [MySQL] 37,051 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new5.72 KB
FAILED: [[SimpleTest]]: [MySQL] 37,027 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Re-rolling #99.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-form-required-fields-742344-103-tests+fix.patch, failed testing.

StatusFileSize
new9.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,649 pass(es).
[ View ]
new6.78 KB
FAILED: [[SimpleTest]]: [MySQL] 37,026 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Okay, let's try this again. Adding code to the tests to recognize the custom error messages.

Status:Needs work» Needs review

So the second one is what we should be testing locally to see if we can get enough folks to mark it RTBC?

Yes, the first is tests only, and should fail; the second includes the patch.

you can set your custom forms validation error through this hook
mymodule_form_validate($form, &$form_state) {
exmmple:--

if (!$form_state['values']['cid']) {
form_set_error('cid', t('Your message'));
}
}

StatusFileSize
new597 bytes
new8.65 KB
PASSED: [[SimpleTest]]: [MySQL] 41,329 pass(es).
[ View ]

This is a great functionality to incorporate in the form API.

If we needed to define a custom #element_validate function anyways, then @chanderbhushan would be right.

But, @chanderbhushan, what they're adding with this patch is the ability to specify #required_error in a field and have the form API automatically use that text as the error message.

I'm attaching a patch that makes this clearer by removing the custom element_validate from the first field.

@lucascaro thanks

Status:Needs review» Needs work

This has been marked RTBC 6 times in it's history. I think the outstanding elements have been addressed.

@chanderbhushan, @tarmstrong & @lucascaro - do you guys think it's ready? If so let's mark it RTBC one last time.

Oh ya, patch applies nicely. I don't see any problems in my D8 instance.

IMO the test case explains pretty well what it's doing and does what the issue text says.

@Dries was concerned about #form_test_required_error but that's simply a custom parameter for testing that adding custom validation with custom error messages still works. It may not be essential for testing this feature but it doesn't hurt at all and makes the test better IMO.

So if we all agree on that last detail this should be RTBC.

Assigned:tarmstrong» Unassigned
Status:Needs work» Reviewed & tested by the community

Yes, this still looks good to me.

It also seems pretty clear to me that @Dries got confused about the custom #property set by the test, which is merely passed forward to the custom #element_validate handler in form_test module, but not consumed by Form API.

Thanks all for keeping this alive.

Yes i think lucascaro is right.

Status:Reviewed & tested by the community» Patch (to be ported)

This seems like a reasonable fix. There might be some overlap with #1785436: Submission of #required elements without #title and empty value does not display any error? Not sure.

Anyway, committed and pushed to 8.x. Down to 7.x for backport.

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

Ahem.

Status:Patch (to be ported)» Needs review
StatusFileSize
new7.8 KB
PASSED: [[SimpleTest]]: [MySQL] 39,503 pass(es).
[ View ]

Backport of #113.

This is great news. I applied @Devin's D7 patch to two instances too and they worked great.

Status:Needs review» Reviewed & tested by the community

Looks like a straightforward backport, thanks.

Assigned:Unassigned» David_Rothstein

This seems fine to me, but probably best for David to eyeball it first. He has the uncanny ability to forsee instability problems with seemingly harmless patches. :D

Assigned:David_Rothstein» Unassigned
Status:Reviewed & tested by the community» Needs work

This looks like a great improvement and I'd really like to see this get in, but the current patch will break things, won't it? In particular, by moving the #required validation after the #element_validate validation, this means that in the case of an empty required field, standard validation errors will now start taking priority (and will be displayed instead of the #required ones). But that doesn't really make sense.

I had trouble finding a good example in core (because most core #element_validate functions do their own rudimentary "required field" checks to begin with), but here's a strained example:

  1. On a fresh installation, go admin/structure/types/manage/article/fields/field_image.
  2. Put an empty space in the "Allowed file extensions" field (don't leave it completely blank, but do put an empty space, which the core #required validation is supposed to treat as empty).
  3. Submit the form. Before the patch, the error message says "Allowed file extensions field is required." After the patch, it says "The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space."

In contrib modules there are definitely examples that are less strained and more serious. I could point to some, but really all you need is pseudo-code like this:

<?php
function MYMODULE_form($form, &$form_state) {
...
 
$form['myfield'] = array(
   
'#title' => 'First name',
   
'#type' => 'textfield',
   
'#required' => TRUE,
   
'#element_validate' => array('MYMODULE_myfield_validate'),
  );
...
}
function
MYMODULE_myfield_validate($element, &$form_state) {
  if (
$element['#value'] != 'David') {
   
form_error($element, t('People with the name of %name are not allowed to submit this form.', array('%name' => $element['#value'])));
  }
}
?>

Then submit the form with no first name filled out; before the patch you get an error message saying the field is required, but after the patch you get a nonsensical error message.

I think for Drupal 7 an acceptable patch would be one that just added the #required_error property, but left the #required validation in the same part of the code it currently is. (That seems like it would hit the 95% use case anyway. I can't think of too many reasons why you'd want to wait until #element_validate to set this custom message anyway, when you can just do it in the original $form array instead.)

Version:7.x-dev» 8.x-dev
Priority:Major» Normal

Tentatively this should go back to Drupal 8 as well.... Because I guess one way to say it is that in addition to introducing the #required_but_empty property, the code reordering included in this patch also introduced a requirement that all #element_validate functions actually check that property. But that wasn't really discussed (and in any case, the #element_validate functions in core weren't actually changed to do it).

I'm also bumping this down to normal priority. (It was marked "major" in #62 based on the idea that it might help fix a critical bug, but that bug has already been fixed since then.)

Status:Needs work» Needs review
Issue tags:-Needs Documentation, -Novice, -Needs committer feedback
StatusFileSize
new1.96 KB
FAILED: [[SimpleTest]]: [MySQL] 50,771 pass(es), 41 fail(s), and 0 exception(s).
[ View ]

I grepped for '#element_validate' in core and adjusted the callbacks that may not sufficiently check for an empty required value.

The empty($element['#required_but_empty']) condition reads a bit strange, and I think that is why I originally went with #required_is_empty (i.e., "is" vs. "but").

Status:Needs review» Needs work

The last submitted patch, drupal8.form-required-empty-validate.128.patch, failed testing.

tagging.

StatusFileSize
new1.53 KB
FAILED: [[SimpleTest]]: [MySQL] 49,715 pass(es), 7 fail(s), and 3 exception(s).
[ View ]

Here's a patch that adds tests that demonstrate #126 in D8: (it fails showing Unexpected error message: People with the name of are not allowed to submit this form.)

Status:Needs work» Needs review

@lucascaro I'm pushing this to needs review, as I think that's what your goal was of adding the additional tests.

That's an odd error message. Couldn't find it with grep -ir 'People with' *

Status:Needs review» Needs work

The last submitted patch, drupal8-form_required_empty-execution_order-742344-131.patch, failed testing.

@mgifford I just implemented the exact example from #126 as a test, that message comes from a custom field validator that shouldn't be running since the field is empty.

Status:Needs review» Needs work
Issue tags:+Usability, +accessibility, +API addition, +needs backport to D7, +a11ySprint

The last submitted patch, drupal8-form_required_empty-execution_order-742344-131.patch, failed testing.

StatusFileSize
new4.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

A reroll combining 122 & 131. Mostly trying to move this ahead.

Status:Needs work» Needs review

oops, go bot go.

Status:Needs review» Needs work

The last submitted patch, drupal-form-required-fields-742344-137.patch, failed testing.

Issue tags:-needs backport to D7

#137 contains changes that do not seem relevant for this issue.

At this point, I think we should just fix up whatever #element_validate handlers might need an adjustment — i.e., #128 — and do not backport this change.

I.e., there's a slight change in #element_validate handlers in D8 in that they are able to override even the default #required error message.

Closely related: #67769: Add trim to all text field submissions will further optimize this, and eliminate the need for #element_validate handlers to perform manual trim()-ing of #value.

I'm fine with jumping back to 128. I had tried to re-roll the patch, keep up with core and combine 122 & 131.

Can you try to re-roll this @sun? Would be great to have this in Core.

Status:Needs work» Needs review
Issue tags:-Usability, -accessibility, -API addition, -a11ySprint

Just thought I'd weigh in with my two-cents at this point.

I keep coming up against this in a commercial setting so first of all, this would be invaluable to me to be able to set custom FAPI error messages.

To that end, I'm happy to help where I can. Not sure whatg that will involve, but I'm hoping to get more depply involved with Drupal over the nextg year so might be able to help with testing/patches etc.

Thanks,

Mark

@markosaurus - Want to try to put together patches from #122 & #131 to see if we can get something the bot likes?

I'm sooo confused! What's actually needed to move this forward? Sun seems to be saying in #140 that just the #element_validate handlers in D8 need to be updated. And that nothing will be done on this in D7. But then mgifford proposes that the patch from #122 (which is for D7) and #131 (which is for D8) be combined. I was going to work on this but not sure what needs to be done. Help!

Thanks,
Mickey

StatusFileSize
new1.96 KB
FAILED: [[SimpleTest]]: [MySQL] 53,271 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

I'm leaning towards the "#128 patch needs revision" instead of the "combine #122 and #131 patches" for now.

Status:Needs review» Needs work

The last submitted patch, drupal8.form-required-empty-validate.128.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.96 KB
FAILED: [[SimpleTest]]: [MySQL] 53,350 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

Ugh. Wrong file name.

Status:Needs review» Needs work

The last submitted patch, drupal8.form-required-empty-validate.148.patch, failed testing.

StatusFileSize
new1.96 KB
FAILED: [[SimpleTest]]: [MySQL] 53,392 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Status:Needs work» Needs review
Issue tags:-Usability, -accessibility, -API addition, -a11ySprint

Status:Needs review» Needs work
Issue tags:+Usability, +accessibility, +API addition, +a11ySprint

The last submitted patch, drupal8.form-required-empty-validate.148.patch, failed testing.

StatusFileSize
new2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 56,938 pass(es).
[ View ]

One more time.

Status:Needs work» Needs review

StatusFileSize
new109.97 KB

Following @David_Rothstein note in #126, I went /admin/structure/types/manage/article/fields/field_image, used this list of allowed file extensions: png, gif, jpg, , jpeg and after hitting submit got this value:

Error message
The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space.

Which is the expected result after the patch.

Is this enough for it to be RTBC?

Screen Shot of applied patch on Image settings for Article.

StatusFileSize
new1.18 KB

Here's an interdiff between @sun's patch way back up the list & the latest one from @micnap.

Status:Needs review» Reviewed & tested by the community

Ok, marking it RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

We shouldn't be adding new features without tests. You should use the form_test module to do this... See http://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modul...

Hey @alexpott - the main issue was actually pushed to core in #120, but there was a follow-up bug that was identified somewhere between #127 & #134.

So although I like tests (I'm no good at writing them) and in this case I don't think it's a new feature. It's a bug fix.

I think we still need the tests that show the bug, break before the patch and pass with the patch applied (see http://drupal.org/contributor-tasks/write-tests)

Status:Needs work» Needs review

Issue tags:+needs backport to D7

In #122, there has already been a D7-backport which was quite agreed upon until David found additional flaws, making us move back to D8 for a followup. I'm just re-adding the tag, so we don't forget after commit.

Category:task» bug
Priority:Normal» Critical

This obviously is a regression (see #126), therefore by definition should be a critical bug? Switch back to normal task for the D7 backport after the followup has landed. For that we still need tests though.

StatusFileSize
new2.3 KB
FAILED: [[SimpleTest]]: [MySQL] 57,405 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

This issue got messy. :) Here's a summary of where we stand plus a patch for the @David_Rothstein approach to fix D8.

Status Summary

In comment 120, @webchick committed the D8 patch. This added the new API, which is good. Folks started working on the D7 backport. But @David_Rothstein pointed out that the committed patch had broken form validation in D8. David suggested that we fix the order that form validation happens in comments 126 and 127. But @sun suggested an alternative work-around in 128 and 140. Currently this is messed up in D8 and we need to implement a solution. The D7 backport discussion may resume later.

New Patch

I support the @David_Rothstein approach, which means doing the required field validation before the #element-validate validation. The new patch attached basically re-orders the logic to restore the order. This could use a code review and a test to make sure the example David points out gets the right error message.

Status:Needs review» Needs work

The last submitted patch, drupal8.form-required-empty-validate.707484.171.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-formRequiredEmptyValidate-742344-173.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This patch fixes the tests to align with the desired behavior.

Nothing else has changed. The approach and summary in comment 171 still apply. This needs review and needs someone to check the example David pointed out in comment 126.

Trying to implement from the strained example provided in comment 126:

  1. On a fresh installation, go admin/structure/types/manage/article/fields/node.article.field_image
  2. Note: this is a different url than the comment in 126

  3. Put an empty space in the "Allowed file extensions" field (don't leave it completely blank, but do put an empty space, which the core #required validation is supposed to treat as empty).
  4. Note: I couldn't add a space to produce an error. Closest I could do was with "png, gif, jpg, '', jpeg"

  5. Submit the form. Before the patch, the error message says "Allowed file extensions field is required." After the patch, it says "The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space."
  6. Note: Without patch the error message now says - "The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space."

I also had trouble enabling the module with:

custom/custom.info.yml

name: Custom
description: 'Custom testing.'
core: 8.x
package: Development
version: VERSION

custom/custom.module

<?php
/**
* Implements hook_menu().
*/
function fat_menu() {
  $items = array();
  $items['custom-form'] = array(
    'title' => 'Custom form',
    'description' => 'Simple form that generates error.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('custom_form'),
    'access arguments' => array('access content'),
  );
}
function custom_form($form, &$form_state) {
  $form = array();
  $form['myfield'] = array(
    '#title' => 'First name',
    '#type' => 'textfield',
    '#required' => TRUE,
    '#element_validate' => array('MYMODULE_myfield_validate'),
  );
  $form['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Send'),
    '#weight' => 100
  );
  return $form;
}
function custom_myfield_validate($element, &$form_state) {
  if ($element['#value'] != 'David') {
    form_error($element, t('People with the name of %name are not allowed to submit this form.', array('%name' => $element['#value'])));
  }
}

But it's too late and I should be sleeping. Maybe it will make more sense later...

StatusFileSize
new64.06 KB
new51.18 KB
new48.64 KB

After applying the patch drupal8-formRequiredEmptyValidate-742344-173.patch by bowersox in #173 to a Drupal 8 fresh install 8/16 , I confirm that the error message changes when the field is missing.

Following the steps in 174
Before the patch, saving only a space in the field in a form results in the same error message as saving the wrong type -- in this case, "The list of allowed extensions is not valid..."
742344before1.png
After saving only a space in the field in a form, there is a different error message, "Allowed file extensions is required".
742344after1.png
However, entering "wrong" file extensions still correctly results in "The list of allowed extensions is not valid..."
742344after2.png

I use could help with whether this patch fixes the issue, though. Will take the comments mentioned in #171 and through out and update the issue summary now.

Issue summary:View changes

Changed "#required_is_empty" to "#required_but_empty"

That behavior looks perfect. It sounds like the patch works as desired.

Here's how to verify that we get the desired behavior:

1. On a fresh installation, go admin/structure/types/manage/article/fields/field_image.
2. Put an empty space in the "Allowed file extensions" field (don't leave it completely blank, but do put an empty space, which the core #required validation is supposed to treat as empty).
3. Submit the form. After this patch is applied, the error message should say "Allowed file extensions field is required." Without this patch, Drupal 8 head is currently saying "The list of allowed extensions is not valid, be sure to exclude leading dots and to separate extensions with a comma or space."

Status:Needs review» Needs work
Issue tags:+Usability, +Needs tests, +accessibility, +API addition, +needs backport to D7, +a11ySprint

The last submitted patch, drupal8-formRequiredEmptyValidate-742344-173.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8-formRequiredEmptyValidate-742344-180.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll... Mostly fuzz except for core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php

Status:Needs review» Needs work
Issue tags:-Usability, -Needs tests, -accessibility, -API addition, -needs backport to D7, -a11ySprint

The last submitted patch, drupal8-formRequiredEmptyValidate-742344-180.patch, failed testing.

Issue summary:View changes

Updated issue summary after patch review at mwds

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 180: drupal8-formRequiredEmptyValidate-742344-180.patch, failed testing.

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new9.25 KB
FAILED: [[SimpleTest]]: [MySQL] 59,743 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

form_error() had $form_state added to it. Otherwise it's just a re-roll.

Status:Needs review» Needs work

The last submitted patch, 184: drupal8-formRequiredEmptyValidate-742344-184.patch, failed testing.

Priority:Critical» Normal

As far I understand, this is a bug in D7 as well, but was downgraded in #127 for a reason.

I'm concerned with this being dropped down to Normal. I can understand your reason from dropping it from Critical @tim.plunkett - I don't see a clear regression as @Pancho mentioned in #168.

This issue was started in March, 2010 in an effort to better meet WCAG accessibility guidelines (3.3.1, 3.3.2, & 3.3.3).

I am hoping we'll address many of those concerns in #1493324: Inline form errors for accessibility and UX but we still should be able to allow admins to have customizable messages. It could be that our solution doesn't meet the requirements for some departments.

It has been marked RTBC 5 times in 2010 (by tobiasb, sun, & mgifford (me)), once in 2011 (quicksketch) & 2 times in 2012 (sun, & mgifford (me)). This does make sense in terms of the release cycle.

This has been committed in D8, and then as Brandon summarized "But @David_Rothstein pointed out that the committed patch had broken form validation in D8. David suggested that we fix the order that form validation happens in comments 126 and 127. But @sun suggested an alternative work-around in 128 and 140. Currently this is messed up in D8 and we need to implement a solution".

This has to be considered at least a Major priority....

Priority:Normal» Major
Issue tags:+Needs reroll

This is a significant issue with D8. It's not a regression, but it is still a major limitation for D8.

Version:8.x-dev» 7.x-dev
Priority:Major» Normal
Status:Needs work» Patch (to be ported)

This was committed October 2012, and marked for backport. Any further non-critical D8 follow-ups should have been moved to another issue.

Feel free to do that now.