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
- 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." - 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:
#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
Comment | File | Size | Author |
---|---|---|---|
#205 | form-custom-required-error-message.png | 81.13 KB | vijaycs85 |
#203 | 742344-form-error-203.patch | 7.12 KB | vijaycs85 |
#203 | 742344-diff-198-203.txt | 720 bytes | vijaycs85 |
#198 | interdiff.txt | 5.18 KB | lord_of_freaks |
#198 | drupal7-allow_forms_to_set-742344-198.patch | 6.86 KB | lord_of_freaks |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedI see D7 in the patch so I guess this a drupal 7 patch...
Comment #2
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #4
neilnz CreditAttribution: neilnz commented@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.
Comment #5
Everett Zufelt CreditAttribution: Everett Zufelt commented@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?
Comment #6
mgiffordsubscribe
Comment #7
neilnz CreditAttribution: neilnz commented@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.
Comment #8
sunAlternatively, we could simply re-use #required?
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedBoth a feature request and an API change... That said, this is useful, and the patch is tiny.
Comment #11
sunFixed that typo.
Comment #12
sunTaking 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.
Comment #13
sunI really don't care, as long as we fix this bug.
Both options attached.
Comment #14
jbrown CreditAttribution: jbrown commentedI 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
Comment #15
jbrown CreditAttribution: jbrown commentedI 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.
Comment #17
sunPatch can be re-rolled quickly. I need and want to know which variant though. @Damien? @chx?
Comment #18
sunLet's use a separate property. That's cleaner. This patch should pass. Had to fix another Form API bug to make the tests work.
Comment #20
sunComment #21
sunRe-rolled against HEAD + simplified.
This looks ready to fly for me.
Comment #22
sunNo API change here.
Comment #23
tobiasbnice feature :D
Comment #24
bowersox CreditAttribution: bowersox commentedsub
Comment #25
sun#21: drupal.form-required-custom.21.patch queued for re-testing.
Comment #27
sunRe-rolled against HEAD.
Comment #28
sunComment #29
sun#27: drupal.form-required-custom.27.patch queued for re-testing.
Comment #31
tobiasb#27: drupal.form-required-custom.27.patch queued for re-testing.
Comment #33
sunAlthough badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).
Comment #34
mgiffordIs it possible that we can bring this functionality into http://drupal.org/project/accessible
Comment #35
sunRe-rolled against HEAD. Better title, proper category.
Comment #36
mgiffordThis 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).
Comment #37
sunNeeds to be re-rolled against HEAD.
Comment #38
mgifford#35: drupal.form-required-custom.35.patch queued for re-testing.
Comment #39
mgiffordAgain? Ok.. Just retesting it. I had applied it just fine an hour ago.
Comment #41
sunRe-rolled against HEAD.
Comment #42
mgiffordthanks sun. Think we're good to go here. This patch applies nicely against the CVS & seems to be working just fine in my sandbox.
Comment #43
webchickSorry, 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.
Comment #44
sunNope, 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.
Comment #45
webchickWell that looks much nicer, if it works. Doesn't change core's default UI nor developer-facing Form API.
Comment #46
sunAnd it passed. kk, could use a line of reasoning in _form_validate() + also a test that proves that the patch actually fixes something ;)
Comment #48
mgifford@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?
Comment #49
sunhttp://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
Comment #50
casey CreditAttribution: casey commentedsub
Comment #51
sunComment #52
sunCleaner, better, rock solid.
Comment #53
sun@webchick:
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.
Comment #54
moshe weitzman CreditAttribution: moshe weitzman commentedThat seems like a valuable ux improvement for little risk. +1
Comment #55
sunTagging for the question in #53.
Comment #56
EvanDonovan CreditAttribution: EvanDonovan commentedI'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.)
Comment #57
mgiffordOk, so to test this I would
Add something like this to a form that I want to make manditory:
And then add in a customized function:
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.
Comment #58
sunAttached patch performs the 3 line change and thus, optionally supports to set #required_error on a form element.
Comment #59
mgiffordLooks 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.
Comment #60
bowersox CreditAttribution: bowersox commentedGood 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.
Comment #61
webchickSorry, 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.
Comment #62
mlncn CreditAttribution: mlncn commentedGiven #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.
Comment #63
EvanDonovan CreditAttribution: EvanDonovan commentedIs #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?
Comment #64
bojanz CreditAttribution: bojanz commentedLet'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.
Comment #65
EvanDonovan CreditAttribution: EvanDonovan commented@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.
Comment #66
bojanz CreditAttribution: bojanz commented@EvanDonovan
Already did (yesterday), see #11 / #12.
Comment #67
EvanDonovan CreditAttribution: EvanDonovan commentedSorry, didn't see the latest posts.
Comment #68
robertom CreditAttribution: robertom commented#58: drupal.form-required-custom.58.patch queued for re-testing.
Comment #69
sun#58: drupal.form-required-custom.58.patch queued for re-testing.
Comment #71
sun#58: drupal.form-required-custom.58.patch queued for re-testing.
Comment #72
pillarsdotnet CreditAttribution: pillarsdotnet commented#58: drupal.form-required-custom.58.patch queued for re-testing.
Comment #73
sunBased on my patch, I think this wasn't possible in D6 either, so let's improve this for D8.
Comment #74
mgifford#58: drupal.form-required-custom.58.patch queued for re-testing.
Comment #75
quicksketchFantastic 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.
Comment #76
catchThis looks backportable to me, only an API addition.
Comment #77
xjmAdded issue summary.
Comment #78
xjmComment #79
tstoecklerThis is really an esoteric question :-), hence leaving RTBC, but could this not simply be
if (!empty($element['#required_is_empty']))
?Comment #80
sunThere 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.
Comment #81
tstoecklerAhhhh....
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...
Comment #81.0
tstoecklerAdded issue summary.
Comment #81.1
xjmClearer indication that it is an API addition only.
Comment #82
attiks CreditAttribution: attiks commentedsubscribing for #1239910: META: tracking other issues about validation
Comment #83
Jelle_Spatch from comment #58 applies to drupal 7.x-dev and passes all the test
Comment #84
pillarsdotnet CreditAttribution: pillarsdotnet commented#58: drupal.form-required-custom.58.patch queued for re-testing.
Comment #85
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled patch from #58 to correct fuzz. No code changes.
Comment #86
naeluh CreditAttribution: naeluh commentedsub
Comment #87
mgiffordI 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?
Comment #88
sunFirst example using the #required_error property.
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.
Comment #89
bfroehle CreditAttribution: bfroehle commentedI think
#required_is_empty
is a confusingly named flag. At first glance I'd imagine it wasempty($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.Comment #90
Dries CreditAttribution: Dries commentedI 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.Comment #90.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedEven clearer.
Comment #91
pillarsdotnet CreditAttribution: pillarsdotnet commented@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:@sun:
In the diff to
includes/form.inc
:I think you meant to say "encouraged" rather than "discouraged". Reworded as follows:
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.
Comment #92
pillarsdotnet CreditAttribution: pillarsdotnet commentedCNR for bot because of the change from
#required_is_empty
to#required_but_empty
.Also updated issue summary and added an API change node.
Comment #93
xjmThis will need a reroll for the core directory change, etc. Tagging novice to reroll the patch.
Comment #94
wamilton CreditAttribution: wamilton commentedRe-rolled for nothing but dir changes and one hunk was looking for trailing whitespace that had already been fixed. Using sed continues to rock.
Comment #95
mgifford#94: drupal-form-required-custom-742344-94.patch queued for re-testing.
Comment #97
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled to correct for fuzz.
Comment #99
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrected, I think.
testsonly should fail.
tests+fix should pass.
Comment #101
mgiffordTagging for sprint
Comment #102
tarmstrong CreditAttribution: tarmstrong commentedComment #103
tarmstrong CreditAttribution: tarmstrong commentedRe-rolling #99.
Comment #104
tarmstrong CreditAttribution: tarmstrong commentedComment #106
tarmstrong CreditAttribution: tarmstrong commentedOkay, let's try this again. Adding code to the tests to recognize the custom error messages.
Comment #107
tarmstrong CreditAttribution: tarmstrong commentedComment #108
mgiffordSo the second one is what we should be testing locally to see if we can get enough folks to mark it RTBC?
Comment #109
tarmstrong CreditAttribution: tarmstrong commentedYes, the first is tests only, and should fail; the second includes the patch.
Comment #110
penyaskito#106: drupal-form-required-fields-742344-106-tests+fix.patch queued for re-testing.
Comment #111
moshe weitzman CreditAttribution: moshe weitzman commented#106: drupal-form-required-fields-742344-106-tests+fix.patch queued for re-testing.
Comment #112
chanderbhushan CreditAttribution: chanderbhushan commentedyou 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'));
}
}
Comment #113
lucascaro CreditAttribution: lucascaro commentedThis 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.
Comment #114
chanderbhushan CreditAttribution: chanderbhushan commented@lucascaro thanks
Comment #115
mgiffordJust posted this related issue #1797438: HTML5 validation is preventing form submit and not fully accessible
Comment #116
mgiffordThis 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.
Comment #117
lucascaro CreditAttribution: lucascaro commentedIMO 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.
Comment #118
sunYes, 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.
Comment #119
chanderbhushan CreditAttribution: chanderbhushan commentedYes i think lucascaro is right.
Comment #120
webchickThis 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.
Comment #121
webchickAhem.
Comment #122
Devin Carlson CreditAttribution: Devin Carlson commentedBackport of #113.
Comment #123
mgiffordThis is great news. I applied @Devin's D7 patch to two instances too and they worked great.
Comment #124
lucascaro CreditAttribution: lucascaro commentedLooks like a straightforward backport, thanks.
Comment #125
webchickThis 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
Comment #126
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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:
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:
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.)
Comment #127
David_Rothstein CreditAttribution: David_Rothstein commentedTentatively 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.)
Comment #128
sunI 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").Comment #130
mgiffordtagging.
Comment #131
lucascaro CreditAttribution: lucascaro commentedHere'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.)
Comment #132
mgifford@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' *
Comment #134
lucascaro CreditAttribution: lucascaro commented@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.
Comment #135
mgifford#131: drupal8-form_required_empty-execution_order-742344-131.patch queued for re-testing.
Comment #137
mgiffordA reroll combining 122 & 131. Mostly trying to move this ahead.
Comment #138
mgiffordoops, go bot go.
Comment #140
sun#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.Comment #141
mgiffordI'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.
Comment #142
mgifford#128: drupal8.form-required-empty-validate.128.patch queued for re-testing.
Comment #143
mgifford#128: drupal8.form-required-empty-validate.128.patch queued for re-testing.
Comment #144
markosaurus CreditAttribution: markosaurus commentedJust 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
Comment #145
mgifford@markosaurus - Want to try to put together patches from #122 & #131 to see if we can get something the bot likes?
Comment #146
micnap CreditAttribution: micnap commentedI'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
Comment #147
micnap CreditAttribution: micnap commentedI'm leaning towards the "#128 patch needs revision" instead of the "combine #122 and #131 patches" for now.
Comment #149
micnap CreditAttribution: micnap commentedUgh. Wrong file name.
Comment #151
micnap CreditAttribution: micnap commentedComment #152
micnap CreditAttribution: micnap commented#149: drupal8.form-required-empty-validate.148.patch queued for re-testing.
Comment #154
micnap CreditAttribution: micnap commentedOne more time.
Comment #155
micnap CreditAttribution: micnap commentedComment #156
mgiffordFollowing @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:Which is the expected result after the patch.
Is this enough for it to be RTBC?
Comment #157
mgiffordHere's an interdiff between @sun's patch way back up the list & the latest one from @micnap.
Comment #158
mgifford#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #159
mgifford#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #160
nod_#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #161
mgiffordOk, marking it RTBC.
Comment #162
alexpottWe 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...
Comment #163
mgiffordHey @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.
Comment #164
lucascaro CreditAttribution: lucascaro commentedI 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)
Comment #165
mgifford#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #166
PanchoIn #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.
Comment #167
mgifford#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #168
PanchoThis 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.
Comment #169
mgifford#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #170
larowlan#154: drupal8.form-required-empty-validate.154.patch queued for re-testing.
Comment #171
bowersox CreditAttribution: bowersox commentedThis 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.
Comment #173
bowersox CreditAttribution: bowersox commentedThis 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.
Comment #174
mgiffordTrying to implement from the strained example provided in comment 126:
Note: this is a different url than the comment in 126
Note: I couldn't add a space to produce an error. Closest I could do was with "png, gif, jpg, '', jpeg"
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
custom/custom.module
But it's too late and I should be sleeping. Maybe it will make more sense later...
Comment #175
andymartha CreditAttribution: andymartha commentedAfter 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..."
After saving only a space in the field in a form, there is a different error message, "Allowed file extensions is required".
However, entering "wrong" file extensions still correctly results in "The list of allowed extensions is not valid..."
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.
Comment #175.0
andymartha CreditAttribution: andymartha commentedChanged "#required_is_empty" to "#required_but_empty"
Comment #176
bowersox CreditAttribution: bowersox commentedThat 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."
Comment #177
mgifford#173: drupal8-formRequiredEmptyValidate-742344-173.patch queued for re-testing.
Comment #178
mgifford#173: drupal8-formRequiredEmptyValidate-742344-173.patch queued for re-testing.
Comment #180
mgiffordReroll... Mostly fuzz except for core/modules/system/lib/Drupal/system/Tests/Form/ValidationTest.php
Comment #181.0
(not verified) CreditAttribution: commentedUpdated issue summary after patch review at mwds
Comment #182
jibran180: drupal8-formRequiredEmptyValidate-742344-180.patch queued for re-testing.
Comment #184
mgiffordform_error() had $form_state added to it. Otherwise it's just a re-roll.
Comment #186
tim.plunkettAs far I understand, this is a bug in D7 as well, but was downgraded in #127 for a reason.
Comment #187
mgiffordI'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....
Comment #189
mgiffordThis is a significant issue with D8. It's not a regression, but it is still a major limitation for D8.
Comment #190
mgiffordComment #191
tim.plunkettThis 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.
Comment #192
malcomio CreditAttribution: malcomio commentedFor D7, there is a workaround:
Don't set the form field to be required, but add the required marker to the title.
I'm trying to think of any reason why this won't work, but it seems to fit the bill so far.
Comment #193
lord_of_freaks CreditAttribution: lord_of_freaks commentedHi All
Here is a patch for drupal 7 ... at least to get the error messages from the
#required_error
propertyComment #194
mgiffordWant this tested by the bot.
Comment #195
lord_of_freaks CreditAttribution: lord_of_freaks commentedHi 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 finishedComment #196
lord_of_freaks CreditAttribution: lord_of_freaks commentedHi all
Any news on this? next steps? can I do something to make it progress???
Regards
Comment #197
mgiffordPatch 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.
Comment #198
lord_of_freaks CreditAttribution: lord_of_freaks commentedHi 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
Comment #199
justindodge CreditAttribution: justindodge commentedI 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!
Comment #202
dcam CreditAttribution: dcam commentedComment #203
vijaycs85Updating documentation for 'required_but_empty'. Should be RTBC still, changing status for testbot.
Comment #204
vijaycs85back to RTBC as per #202
Comment #205
vijaycs85Adding manual test screenshot...
Comment #208
lord_of_freaks CreditAttribution: lord_of_freaks commentedComment #211
lord_of_freaks CreditAttribution: lord_of_freaks commentedComment #214
dcam CreditAttribution: dcam commentedComment #217
dcam CreditAttribution: dcam commentedComment #218
David_Rothstein CreditAttribution: David_Rothstein commentedSo, 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.
Comment #219
lord_of_freaks CreditAttribution: lord_of_freaks commentedI'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 thet
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
Comment #222
mgiffordThink we have to deal with this in 8.1
Comment #223
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedWell, 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.
Comment #224
mgiffordThat would be great if we can get it in later in the 8.0.x releases.
Comment #225
mgiffordComment #236
cosmicdreams CreditAttribution: cosmicdreams at Nerdery commentedHey 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).
Comment #237
sic CreditAttribution: sic commentedbump
Comment #239
nielsaers CreditAttribution: nielsaers commentedIt 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.
Comment #243
mgiffordWould be nice to be able to chose any of these options - https://adamsilver.io/blog/how-to-highlight-required-and-optional-form-f...
Comment #245
steinmb CreditAttribution: steinmb at University Of Bergen commentedGoing through older form API issues and found this. D9.x got https://www.drupal.org/commitlog/commit/2/08ff47b5fd1e9ff1039dba129ac1c4...
What is left in here? Are we done?