| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | forms system |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | accessibility, API addition, needs backport to D7, Needs committer feedback, Needs Documentation, Novice, Usability |
Issue Summary
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
- Support a
#required_errorproperty 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." - Additionally set a
#required_but_emptyflag 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:
#required_error#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:
- 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.
- 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.
- 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)
- 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
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| form.inc_.customrequired.d7.patch | 1 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch form.inc_.customrequired.d7.patch. | View details | Re-test |
Comments
#1
I see D7 in the patch so I guess this a drupal 7 patch...
#2
@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.
#3
The last submitted patch, form.inc_.customrequired.d7.patch, failed testing.
#4
@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.
#5
@neilnz
Does the patch in #447816: WCAG violation: Relying on a color by itself to indicate a field validation error address any of your issues?
#6
subscribe
#7
@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.
#8
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."'),
);
?>
#9
Both a feature request and an API change... That said, this is useful, and the patch is tiny.
#10
The last submitted patch, drupal.form-required-custom.8.patch, failed testing.
#11
Fixed that typo.
#12
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.
#13
I really don't care, as long as we fix this bug.
Both options attached.
#14
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
#15
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.
#16
The last submitted patch, drupal.form-required-custom.re-use.13.patch, failed testing.
#17
Patch can be re-rolled quickly. I need and want to know which variant though. @Damien? @chx?
#18
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.
#19
The last submitted patch, drupal.form-required-custom.18.patch, failed testing.
#20
#21
Re-rolled against HEAD + simplified.
This looks ready to fly for me.
#22
No API change here.
#23
nice feature :D
#24
sub
#25
#21: drupal.form-required-custom.21.patch queued for re-testing.
#26
The last submitted patch, drupal.form-required-custom.21.patch, failed testing.
#27
Re-rolled against HEAD.
#28
#29
#27: drupal.form-required-custom.27.patch queued for re-testing.
#30
The last submitted patch, drupal.form-required-custom.27.patch, failed testing.
#31
#27: drupal.form-required-custom.27.patch queued for re-testing.
#32
The last submitted patch, drupal.form-required-custom.27.patch, failed testing.
#33
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).
#34
Is it possible that we can bring this functionality into http://drupal.org/project/accessible
#35
Re-rolled against HEAD. Better title, proper category.
#36
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).
#37
Needs to be re-rolled against HEAD.
#38
#35: drupal.form-required-custom.35.patch queued for re-testing.
#39
Again? Ok.. Just retesting it. I had applied it just fine an hour ago.
#40
The last submitted patch, drupal.form-required-custom.35.patch, failed testing.
#41
Re-rolled against HEAD.
#42
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.
#43
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.
#44
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.
#45
Well that looks much nicer, if it works. Doesn't change core's default UI nor developer-facing Form API.
#46
And it passed. kk, could use a line of reasoning in _form_validate() + also a test that proves that the patch actually fixes something ;)
#48
@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?
#49
http://api.drupal.org/api/function/contact_site_form_validate/7
http://api.drupal.org/api/function/profile_field_form_validate/7
http://api.drupal.org/api/function/profile_user_form_validate/7
http://api.drupal.org/api/function/user_admin_role_validate/7
#50
sub
#51
#52
Cleaner, better, rock solid.
#53
@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 checkingisset($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.
#54
That seems like a valuable ux improvement for little risk. +1
#55
Tagging for the question in #53.
#56
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.)
#57
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.
#58
Attached patch performs the 3 line change and thus, optionally supports to set #required_error on a form element.
#59
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.
#60
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.
#61
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.
#62
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.
#63
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?
#64
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.
#65
@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.
#66
@EvanDonovan
Already did (yesterday), see #11 / #12.
#67
Sorry, didn't see the latest posts.
#68
#58: drupal.form-required-custom.58.patch queued for re-testing.
#69
#58: drupal.form-required-custom.58.patch queued for re-testing.
#70
The last submitted patch, drupal.form-required-custom.58.patch, failed testing.
#71
#58: drupal.form-required-custom.58.patch queued for re-testing.
#72
#58: drupal.form-required-custom.58.patch queued for re-testing.
#73
Based on my patch, I think this wasn't possible in D6 either, so let's improve this for D8.
#74
#58: drupal.form-required-custom.58.patch queued for re-testing.
#75
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_emptyproperty (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.
#76
This looks backportable to me, only an API addition.
#77
Added issue summary.
#78
#79
+++ 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']))?#80
+++ 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.
#81
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_emptyin an#element_validatefunction 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...
#82
subscribing for #1239910: META: tracking other issues about validation
#83
patch from comment #58 applies to drupal 7.x-dev and passes all the test
#84
#58: drupal.form-required-custom.58.patch queued for re-testing.
#85
Re-rolled patch from #58 to correct fuzz. No code changes.
#86
sub
#87
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?
#88
+ '#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.
#89
I think
#required_is_emptyis a confusingly named flag. At first glance I'd imagine it wasempty($element['#required']).A more readable name might be
#required_and_emptyor#required_but_emptyto indicate that really it is for elements which are both #required and left empty.#90
I don't understand why we need
#form_test_required_errorwhen the purpose of the test is to test#form_is_empty? As far as I can tell,#form_test_required_errorhas 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.#91
@bfroehle:
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:
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.
#92
CNR for bot because of the change from
#required_is_emptyto#required_but_empty.Also updated issue summary and added an API change node.
#93
This will need a reroll for the core directory change, etc. Tagging novice to reroll the patch.
#94
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.