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

CommentFileSizeAuthor
#205 form-custom-required-error-message.png81.13 KBvijaycs85
#203 742344-form-error-203.patch7.12 KBvijaycs85
#203 742344-diff-198-203.txt720 bytesvijaycs85
#198 interdiff.txt5.18 KBlord_of_freaks
#198 drupal7-allow_forms_to_set-742344-198.patch6.86 KBlord_of_freaks
#195 drupal7-allow_forms_to_set-742344-195.patch1.67 KBlord_of_freaks
#193 drupal7-allow_forms_to_set_custom_validation_error_messages_on_required_fields-742344.patch1.62 KBlord_of_freaks
#184 drupal8-formRequiredEmptyValidate-742344-184.patch9.25 KBmgifford
#180 drupal8-formRequiredEmptyValidate-742344-180.patch11.52 KBmgifford
#175 742344before1.png48.64 KBandymartha
#175 742344after1.png51.18 KBandymartha
#175 742344after2.png64.06 KBandymartha
#173 drupal8-formRequiredEmptyValidate-742344-173.patch11.51 KBbowersox
#171 drupal8.form-required-empty-validate.707484.171.patch2.3 KBbowersox
#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
#151 drupal8.form-required-empty-validate.148.patch1.96 KBmicnap
#149 drupal8.form-required-empty-validate.148.patch1.96 KBmicnap
#147 drupal8.form-required-empty-validate.128.patch1.96 KBmicnap
#137 drupal-form-required-fields-742344-137.patch4.67 KBmgifford
#131 drupal8-form_required_empty-execution_order-742344-131.patch1.53 KBlucascaro
#128 drupal8.form-required-empty-validate.128.patch1.96 KBsun
#122 drupal-form-required-fields-742344-122-tests+fix.patch7.8 KBDevin Carlson
#113 drupal-form-required-fields-742344-113-tests+fix.patch8.65 KBlucascaro
#113 interdiff.txt597 byteslucascaro
#106 drupal-form-required-fields-742344-106-testsonly.patch6.78 KBtarmstrong
#106 drupal-form-required-fields-742344-106-tests+fix.patch9.41 KBtarmstrong
#103 drupal-form-required-fields-742344-103-testsonly.patch5.72 KBtarmstrong
#103 drupal-form-required-fields-742344-103-tests+fix.patch8.35 KBtarmstrong
#99 drupal-form-required-fields-742344-99-testsonly.patch5.73 KBpillarsdotnet
#99 drupal-form-required-fields-742344-99-tests+fix.patch8.42 KBpillarsdotnet
#97 drupal-form-required-fields-742344-97.patch8.54 KBpillarsdotnet
#94 drupal-form-required-custom-742344-94.patch7.77 KBwamilton
#91 drupal-form-required-custom-742344-91.patch8.74 KBpillarsdotnet
#85 drupal-form-required-custom-742344-85.patch8.13 KBpillarsdotnet
#58 drupal.form-required-custom.58.patch7.82 KBsun
#52 drupal.form-required-custom.52.patch7.13 KBsun
#51 drupal.form-required-custom.50.tests-only.patch5.62 KBsun
#44 drupal.form-required-custom.44.patch1.95 KBsun
#41 drupal.form-required-custom.41.patch13.72 KBsun
#35 drupal.form-required-custom.35.patch13.36 KBsun
#27 drupal.form-required-custom.27.patch10.48 KBsun
#21 drupal.form-required-custom.21.patch10.46 KBsun
#20 drupal.form-required-custom.20.patch10.45 KBsun
#18 drupal.form-required-custom.18.patch10.85 KBsun
#13 drupal.form-required-custom.separate.13.patch8.83 KBsun
#13 drupal.form-required-custom.re-use.13.patch8.69 KBsun
#11 drupal.form-required-custom.11.patch5.82 KBsun
#8 drupal.form-required-custom.8.patch5.83 KBsun
#4 form.inc_.customrequired.cvshead.patch1.04 KBneilnz
form.inc_.customrequired.d7.patch1 KBneilnz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

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

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

Everett Zufelt’s picture

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.

neilnz’s picture

Status: Needs work » Needs review
FileSize
1.04 KB

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

Everett Zufelt’s picture

mgifford’s picture

subscribe

neilnz’s picture

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

sun’s picture

Title: Provide support for custom form element #required messages per field » Support custom #required form error messages per element
Category: feature » task
FileSize
5.83 KB

Alternatively, we could simply re-use #required?

  $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."'),
  );
Damien Tournoud’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
5.82 KB

Fixed that typo.

sun’s picture

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.

sun’s picture

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

Both options attached.

jbrown’s picture

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

jbrown’s picture

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.

sun’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
10.85 KB

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.

sun’s picture

Status: Needs work » Needs review
Issue tags: +API addition
FileSize
10.45 KB
sun’s picture

Title: Support custom #required form error messages per element » Custom #required form error messages per element
FileSize
10.46 KB

Re-rolled against HEAD + simplified.

This looks ready to fly for me.

sun’s picture

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.

tobiasb’s picture

Status: Needs review » Reviewed & tested by the community

nice feature :D

bowersox’s picture

sub

sun’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
10.48 KB

Re-rolled against HEAD.

sun’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Status: Reviewed & tested by the community » Needs work

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

tobiasb’s picture

Status: Needs work » Needs review

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

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

sun’s picture

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

mgifford’s picture

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

sun’s picture

Title: #required_error for custom form element error messages » Various 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
FileSize
13.36 KB

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

mgifford’s picture

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

sun’s picture

Status: Reviewed & tested by the community » Needs work

Needs to be re-rolled against HEAD.

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
13.72 KB

Re-rolled against HEAD.

mgifford’s picture

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.

webchick’s picture

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.

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
1.95 KB

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.

webchick’s picture

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

sun’s picture

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

mgifford’s picture

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

casey’s picture

sub

sun’s picture

sun’s picture

Cleaner, better, rock solid.

sun’s picture

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

moshe weitzman’s picture

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

sun’s picture

Issue tags: +Needs committer feedback

Tagging for the question in #53.

EvanDonovan’s picture

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

mgifford’s picture

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.

sun’s picture

Category: support » bug
FileSize
7.82 KB

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

mgifford’s picture

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.

bowersox’s picture

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.

webchick’s picture

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.

mlncn’s picture

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.

EvanDonovan’s picture

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?

bojanz’s picture

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.

EvanDonovan’s picture

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

bojanz’s picture

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

EvanDonovan’s picture

Sorry, didn't see the latest posts.

robertom’s picture

Issue tags: -Usability, -Accessibility, -Needs committer feedback, -API addition
sun’s picture

Status: Reviewed & tested by the community » Needs work

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

sun’s picture

Status: Needs work » Needs review
pillarsdotnet’s picture

Issue tags: +Usability, +Accessibility, +Needs committer feedback, +API addition
sun’s picture

Category: bug » task

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

mgifford’s picture

quicksketch’s picture

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.

catch’s picture

Issue tags: +Needs backport to D7

This looks backportable to me, only an API addition.

xjm’s picture

Added issue summary.

xjm’s picture

Title: Various forms try to set a custom validation error for #required elements, but that is not possible » Allow forms to set custom validation error messages on required fields
tstoeckler’s picture

+++ 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']))?

sun’s picture

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

tstoeckler’s picture

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

tstoeckler’s picture

Issue summary: View changes

Added issue summary.

xjm’s picture

Issue summary: View changes

Clearer indication that it is an API addition only.

attiks’s picture

Jelle_S’s picture

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

pillarsdotnet’s picture

pillarsdotnet’s picture

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

naeluh’s picture

sub

mgifford’s picture

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?

sun’s picture

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

bfroehle’s picture

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.

Dries’s picture

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.

pillarsdotnet’s picture

Issue summary: View changes

Even clearer.

pillarsdotnet’s picture

Status: Needs review » Needs work
FileSize
8.74 KB

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

pillarsdotnet’s picture

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.

xjm’s picture

Assigned: sun » Unassigned
Issue tags: +Novice

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

wamilton’s picture

Status: Needs work » Needs review
FileSize
7.77 KB

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.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Usability, +Needs documentation, +Novice, +Accessibility, +Needs committer feedback, +API addition, +Needs backport to D7

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

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
8.54 KB

Re-rolled to correct for fuzz.

Status: Needs review » Needs work

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

pillarsdotnet’s picture

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.

mgifford’s picture

Issue tags: +a11ySprint

Tagging for sprint

tarmstrong’s picture

Assigned: Unassigned » tarmstrong
tarmstrong’s picture

tarmstrong’s picture

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.

tarmstrong’s picture

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

tarmstrong’s picture

Status: Needs work » Needs review
mgifford’s picture

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

tarmstrong’s picture

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

penyaskito’s picture

moshe weitzman’s picture

chanderbhushan’s picture

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'));
}
}

lucascaro’s picture

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.

chanderbhushan’s picture

@lucascaro thanks

mgifford’s picture

mgifford’s picture

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.

lucascaro’s picture

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.

sun’s picture

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.

chanderbhushan’s picture

Yes i think lucascaro is right.

webchick’s picture

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.

webchick’s picture

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

Ahem.

Devin Carlson’s picture

Status: Patch (to be ported) » Needs review
FileSize
7.8 KB

Backport of #113.

mgifford’s picture

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

lucascaro’s picture

Status: Needs review » Reviewed & tested by the community

Looks like a straightforward backport, thanks.

webchick’s picture

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

David_Rothstein’s picture

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:

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

David_Rothstein’s picture

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

sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs documentation, -Novice, -Needs committer feedback
FileSize
1.96 KB

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.

mgifford’s picture

tagging.

lucascaro’s picture

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

mgifford’s picture

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.

lucascaro’s picture

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

mgifford’s picture

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

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.

mgifford’s picture

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

mgifford’s picture

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.

sun’s picture

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.

mgifford’s picture

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.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -Accessibility, -API addition, -a11ySprint
mgifford’s picture

markosaurus’s picture

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

mgifford’s picture

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

micnap’s picture

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

micnap’s picture

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.

micnap’s picture

Status: Needs work » Needs review
FileSize
1.96 KB

Ugh. Wrong file name.

Status: Needs review » Needs work

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

micnap’s picture

micnap’s picture

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.

micnap’s picture

One more time.

micnap’s picture

Status: Needs work » Needs review
mgifford’s picture

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.

mgifford’s picture

FileSize
1.18 KB

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

mgifford’s picture

mgifford’s picture

nod_’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Ok, marking it RTBC.

alexpott’s picture

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

mgifford’s picture

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.

lucascaro’s picture

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)

mgifford’s picture

Status: Needs work » Needs review
Pancho’s picture

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.

mgifford’s picture

Pancho’s picture

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.

mgifford’s picture

larowlan’s picture

bowersox’s picture

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.

bowersox’s picture

Status: Needs work » Needs review
FileSize
11.51 KB

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.

mgifford’s picture

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

andymartha’s picture

FileSize
64.06 KB
51.18 KB
48.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.

andymartha’s picture

Issue summary: View changes

Changed "#required_is_empty" to "#required_but_empty"

bowersox’s picture

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

mgifford’s picture

mgifford’s picture

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.

mgifford’s picture

Status: Needs work » Needs review
FileSize
11.52 KB

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary after patch review at mwds

jibran’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

mgifford’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.25 KB

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.

tim.plunkett’s picture

Priority: Critical » Normal

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

mgifford’s picture

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

mgifford’s picture

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.

mgifford’s picture

tim.plunkett’s picture

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.

malcomio’s picture

For D7, there is a workaround:

Don't set the form field to be required, but add the required marker to the title.

  $form['postcode_or_town'] = array(
    '#type' => 'textfield',
    '#title' => t('Postcode/town') . ' ' . theme('form_required_marker', array()),
    '#size' => 20,
    '#default_value' => empty($form_state['values']['postcode_or_town']) ? '' : $form_state['values']['postcode_or_town'],
  );

I'm trying to think of any reason why this won't work, but it seems to fit the bill so far.

lord_of_freaks’s picture

Hi All

Here is a patch for drupal 7 ... at least to get the error messages from the #required_error property

mgifford’s picture

Status: Patch (to be ported) » Needs review

Want this tested by the bot.

lord_of_freaks’s picture

Hi again

Added also the suggested behavior for #required_but_empty adding the property in _form_validate once the validation logic for the #required property is finished

lord_of_freaks’s picture

Hi all

Any news on this? next steps? can I do something to make it progress???

Regards

mgifford’s picture

Patch still applies nicely on SimplyTest.me.

Mostly it's a matter of testing that the functionality works as needed.

I would benefit from having a quick list of steps replicate this problem so that we can verify it is fixed.

The patch looks simple enough.

lord_of_freaks’s picture

Hi mgifford

I uploaded a new version of the patch providing the test cases for FORM API elements with the new properties #required_error and #required_but_empty

Also is attached and interdiff patch but it should not be necessary because the patch in itself didn´t change at all

Thanks for your efforts reviewing it!!!!
Best regards

justindodge’s picture

Status: Needs review » Reviewed & tested by the community

I had an opportunity to test the patch in #198 in D 7.34 with a real-world scenario that required this functionality.

Everything went swimmingly:
1. Form elements that used the default required message (did not have #required_error set) continued to produce this default and were not adversely/unexpectedly affected.
2. Required form elements using the new #required_error property successfully displayed the error message that I had specified upon form submission when submitting empty values.
3. I examined the form array in a custom validate handler and found that the #required_but_empty property was set appropriately after having been flagged by the code in core upon submission.

Perfect, 5 stars, would buy again.

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 198: drupal7-allow_forms_to_set-742344-198.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs reroll
vijaycs85’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
720 bytes
7.12 KB

Updating documentation for 'required_but_empty'. Should be RTBC still, changing status for testbot.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

back to RTBC as per #202

vijaycs85’s picture

Adding manual test screenshot...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 203: 742344-form-error-203.patch, failed testing.

lord_of_freaks’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 203: 742344-form-error-203.patch, failed testing.

lord_of_freaks’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 203: 742344-form-error-203.patch, failed testing.

dcam queued 203: 742344-form-error-203.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 203: 742344-form-error-203.patch, failed testing.

dcam queued 203: 742344-form-error-203.patch for re-testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Reviewed & tested by the community » Needs work

So, this issue is a little confusing, but the history is basically that it was never fully implemented correctly in Drupal 8. #126 is a regression that requires some kind of followup. I double-checked now that you can still reproduce #126 in Drupal 8 today (only difference is the URL to visit for the in-core example, admin/structure/types/manage/article/fields/node.article.field_image).

The Drupal 7 patch looks more like I would expect and doesn't have this problem, but I haven't reviewed it carefully.

#191 suggested moving the Drupal 8 followup to a separate issue, which is fine, but I don't think anyone did that yet (and there were a lot of patches for that here already)...

I am really hesitant to add this feature to Drupal 7 until it's been finalized in Drupal 8; otherwise I think we'll wind up with some confusing inconsistencies between the two versions, especially since fixing this in Drupal 8 might require (small) API changes.

So, I am pushing this back to Drupal 8 for now. If someone wants to move the followup to a separate issue and put this back to RTBC for Drupal 7, that's fine. But I think we'll need to wait until there's more progress on the Drupal 8 fix to actually commit this to Drupal 7.

lord_of_freaks’s picture

I'm not sure sure about the Drupal 8 version of the issue (I only worked for the D7 one) but it doesn't look like right to me to subordinate absolutely every single feature to be developed and "released" in D8 to be part of D7, specially those that are well defined as this one.

We are talking about allowing to customise per element the message for the core #required property (specially import for the usage of the t function) and to cache in the element whether or not this element passed that first validation. IMHO it SHOULD NOT be such a big challenge and SHOULD be part of the core and obviously PORTED TO/INHERITED BY D8 not the other way around.

The issue is 5 years old (March 15, 2010 at 1:28am) and has 200+ comments plus a bunch of patches ... are most of this delay comes due to we are trying to backport something that was not already done in an API (Drupal 8) that wasn't done yet, that is changing, and so forth and so on ... so it looks like an impossible to me and to be honest a waste of time and effort to contribute anything to D7 given will be delayed/ignored.

My opinion is to split the development for the feature in two branches, this one for D7 (as in origin was) marking it RTBC as suggested by #218 (if the community is happy enough with the latest patch) and keep developing the D8 version in another issue based on the requirement given in #0

The last submitted patch, 203: 742344-form-error-203.patch, failed testing.

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Think we have to deal with this in 8.1

David_Rothstein’s picture

Version: 8.1.x-dev » 8.0.x-dev

Well, the current behavior is a bug, and there were some suggestions above for fixing it without anything resembling an API change for Drupal 8 (e.g. @sun's suggestion in #128)... I think the ideal fix would be different, but after a number of years it may be getting late to do anything else for Drupal 8.

It could still be a separate issue though.

mgifford’s picture

That would be great if we can get it in later in the 8.0.x releases.

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 08ff47b on 8.3.x
    Issue #742344 by sun, pillarsdotnet, tarmstrong, neilnz, wamilton,...

  • webchick committed 08ff47b on 8.3.x
    Issue #742344 by sun, pillarsdotnet, tarmstrong, neilnz, wamilton,...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 08ff47b on 8.4.x
    Issue #742344 by sun, pillarsdotnet, tarmstrong, neilnz, wamilton,...

  • webchick committed 08ff47b on 8.4.x
    Issue #742344 by sun, pillarsdotnet, tarmstrong, neilnz, wamilton,...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cosmicdreams’s picture

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

Hey gang:

I was searching for this functionality today and found this issue. Looks like this was committed to Drupal 8.3 and 8.4. Given the history of this conversation I suppose the next thing that was talked about was finding a way to add this feature to Drupal 7 (if that's what you want to do now).

sic’s picture

bump

  • webchick committed 08ff47b on 9.1.x
    Issue #742344 by sun, pillarsdotnet, tarmstrong, neilnz, wamilton,...
nielsaers’s picture

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

It actually doesn't work when the field required through #states.

This is because the FormValidator.php never gets into checking for an empty value because $elements['#required'] is empty.

I haven't been able to look into a fix, but wanted to share this for anyone passing by.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.1.10 (June 4, 2021) and Drupal 9.2.10 (November 24, 2021) were the last bugfix releases of those minor version series. Drupal 9 bug reports should be targeted for the 9.3.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mgifford’s picture

Would be nice to be able to chose any of these options - https://adamsilver.io/blog/how-to-highlight-required-and-optional-form-f...

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.