Posted by AmrMostafa on July 31, 2008 at 10:53am
21 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D6, needs backport to D7, Novice, Usability |
Issue Summary
FAPI supports setting form_error(...) with an empty message. However, _form_set_class(...) doesn't support this, so the 'error' class is not added to such elements. One-liner patch attached.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| error_class_with_empty_message.patch | 587 bytes | Idle | Failed: 8990 passes, 1 fail, 0 exceptions | View details | Re-test |
Comments
#1
Is there are an acceptable use case for using form_set_error(...) with an empty message? I check all form_set_error()s in core and we always seem to pass in a message. I'd like to know to make sure we're fixing the real problem.
#2
I just checked all of today's core. In all cases but one, form_error is invoked with a constant non-empty string.
The only place where a variable (hence a potentially empty value) happens is in install.php. But in that case too the variable ($error) comes from a call and install.php only calls form_error if the variable is not empty. So there is no call to form_error with an empty message in core.
I also checked the whole DRUPAL-6--1 branch of contrib/modules and saw no use of form_error without a message.
So unless there is a newly-found need for this feature, I think the best fix would be to remove the default value for the message parameter, not add this class for the unused empty case.
#3
Forgot to set status. Reduced priority since the error does not actually occur.
#4
In that case it's a feature request, that I completely support. The patch applies (with a small offset), and can't do any harm, RTBC.
Note to Dries: we have no FAPI unit tests for now, and no use-case of that in core either. I'll create an issue so that we take care of this during the Testing Party.
#5
Agreed it's more a feature request than a bug. But, Damien, can you explain why you support the feature since we have no use case for it, and Dries says he makes sure such empty messages do not happen before committing ? Do you have new use case for it ?
#6
Setting back to 'code needs review' until we understand the use case.
#7
Sorry for not posting a use case earlier,
form.incalready supported it but_form_set_class()didn't, it was a bug in my own terms. The use case is that when files are uploaded with errors, file.inc'sfile_check_upload()usesdrupal_set_message()to show these errors. That of course doesn't let FAPI know about it. So as a way to work around that, I needed to set an error with empty message to the element (the message was alreadydrupal_set_message()ed byfile.inc).If it helps, I could add that I was doing that in a separate module which provides an improved FAPI type (using
hook_elements()) for handling file uploads. The relevant error were being set from<fapi-type>_value()hook.If that use case makes sense, then I would propose as a solution that
file_check_upload()could return errors instead of recording them withdrupal_set_message(). Perhaps with a flag$show_errorswhich would automaticallydrupal_set_message()the errors (but still return them).form.incshould also be more consistent in not allowing empty messages.#8
The last submitted patch failed testing.
#9
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#10
The last submitted patch failed testing.
#11
Why would we want errors without messages? IMO that's confusing for users at least.
#12
Please read #7 it explains the use case behind this.
All in all, if Drupal doesn't support empty error messages then it should do so in all the code, however, the code accepts empty messages in many points but in _form_set_class() the check, unintentionally IMHO, drops empty errors. In another words, the current behavior, I believe, is rather unintended and the original behavior was to allow such empty messages.
In #7, I demonstrate a use case, if it makes since for most people then we can fix this one line to have it working. If not, then we will need to do some code cleanup/tweak so empty errors are probably checked.
#13
I don't see why we shouldn't show an error message to tell people their file upload has gone wrong?
#14
We have unnecessarily diverted to another issue..
In answer to Xano: No one said we shouldn't, and that already happens by
file_save_upload(). However, it shows the error message by using adrupal_set_messagenot FAPI'sform_set_error. You can of course do a form_set_error('file_field_here', 'message') but the error triggered by file_save_upload() would end up on the user screen anyway (i.e. now 2 error messages). So my immature workaround was to form_set_error('file_field_here', ''); as to tell FAPI the element is erroneous.The title is not correct because FAPI already allows them but the 'error' class doesn't end up associated to the elements with empty error messages because of an unintended bug. Here is the current form_set_error(), read it carefully you will notice it accepts empty messages wholeheartedly:
<?phpfunction form_set_error($name = NULL, $message = '', $reset = FALSE) {
static $form = array();
if ($reset) {
$form = array();
}
if (isset($name) && !isset($form[$name])) {
$form[$name] = $message;
if ($message) {
drupal_set_message($message, 'error');
}
}
return $form;
}
?>
This is the current code as we speak, regardless of whether we are in support of empty error messages or not, in which case we need to open up another issue "Remove support for empty error messages".
The problem this issue is concerned with is that FAPI's
_form_set_classcheck on the errors is not strict enough to differentiate between when an element has an error with an empty message, and when the element doesn't have errors at all.A reroll! :)
#15
Thanks for clarifying. If the main function does indeed support it already, then the styling function should do that consistently.
In this case, the alternative to this patch is removing support for the feature entirely, and go with
<?phpfunction form_set_error($name = NULL, $message = '', $reset = FALSE) {
static $form = array();
if ($reset) {
$form = array();
}
if (isset($name) && !isset($form[$name]) && $message) {
$form[$name] = $message;
drupal_set_message($message, 'error');
}
return $form;
}
?>
Reject empty message calls entirely without modifying the array - or make sure that any modification to the array actually takes effect.
#16
I think this at least needs a comment to indicate why we're checking against NULL. Arancaytar - I'm not sure from your comment whether you think we should go for not allowing empty errors or not?
#17
Sorry for being unclear. I meant that rather than being a new feature to be introduced, this patch seems to be fixing a feature that was supposed to be already supported. I'm in favor of allowing empty errors, for flexibility.
I think that the !== NULL should be replaced by isset() for the reason you outline. isset() will return false for NULL values, and unlike the strict-type equality check (that is used very rarely) it is self-documenting in that everyone should be immediately able to tell what it does: It checks whether the variable is set, ie. whether an error has been set on this element, returning true even if the message is actually "".
#18
Just wanted to share another use case. I am writing a module as we speak that is comparing two or more fields to make sure each entry is unique. If two or more fields have the same value then I want to highlight the fields that have the same value, but only display the message "Fields must have unique values" once as displaying the message 2 or more time would be redundant. Therefore I need to be able to use:
<?phpform_set_error('testfield','Fields must have unique values');
form_set_error('testfield2');
form_set_error('testfield3');
//etc...
?>
rather then the workaround
<?phpform_set_error('testfield','Fields must have unique values');
form_set_error('testfield2',' '); //This adds blank but unnecessary spaces onto the message output
form_set_error('testfield3',' ');
//etc...
?>
A very simple work around to this is:
<?phpfunction form_set_error($name = NULL, $message = '', $reset = FALSE) {
static $form = array();
if ($reset) {
$form = array();
}
if (isset($name) && !isset($form[$name])) {
$form[$name] = !$message ? TRUE : $message;
if ($message) {
drupal_set_message($message, 'error');
}
}
return $form;
}
?>
This simply checks if there is a message value and if so assigns it to the $form[$name] variable if not it at least sets it to TRUE so it will pass through the isset() function as valid that the form_get_error(s) functions run it through and give the field it's error class.
#19
So I got into typing that one a little to quick and uploaded the D6 patch I made for myself earlier(since what I am doind right now involves D6) But all the same principles should still apply to the D7 function
<?phpfunction form_set_error($name = NULL, $message = '') {
$form = &drupal_static(__FUNCTION__, array());
if (isset($name) && !isset($form[$name])) {
$form[$name] = !$message ? TRUE : $message;
if ($message) {
drupal_set_message($message, 'error');
}
}
return $form;
}
?>
And here is the D7 patch.
**edit** I must have ate a big bowl of dumb for breakfast today. Didn't mean to change the Issue title either. **/edit**
#20
#21
#22
The last submitted patch failed testing.
#23
fixing the patch file.
#24
The last submitted patch failed testing.
#25
Subscribing. I hope to see support for setting errors on form fields without setting a message.
Example use case:
Say you have a form that accepts credit card data. You have a field for the expiration month, and another field for the expiration year. If the expiration date is in the past, you want to set an error saying "The credit card you entered has expired.". You only want to set one message, but you want both fields to be highlighted.
#26
Agreed with adamo.
My use case:
I have 3 text fields for phone numbers, social security numbers, etc. I would like to be able to set only one message. Any place that you have combo fields it would be nice. Just as if you have a parent hierarchy in #parents and they will all get set if you set the parent error.
#27
Wanted to update this with a patch that will hopefully pass testing. I don't know if it would be too late to include this but it's worth a shot.
#28
I am just curious if this patch(#27) could be looked at for inclusion in D7. I shared a use case in #18 and the D7 fix to this problem in #19. There are also some good use cases set forth in #25 and #26 that show some other good situations.
I can't think of any drawbacks to doing it this way. It allows for setting form elements to the error class with out having to display unnecessary blank error messages.
#29
Tagging this Usability, as I think this falls into that category and should be allowed for the post code freeze issues/
#30
same bug here: http://drupal.org/node/135867#comment-2455910
maybe we'll mark that a dup since this has a 7.x patch?
#31
this is surely a bug
#32
This looks like a straight bug. Let's fix it and backport.
#33
Here's a very similar patch, but avoids an extra conditional and avoids problems by making $form[$name] be consistently a string.
for example, http://api.drupal.org/api/function/install_run_task/7 runs an implode to generate a string.
#34
oops - left an extra line in.
#35
Actually - I kind of like the solution here better: http://drupal.org/node/135867
#36
Here's the patch.
#37
If we are there, why not
is_string($form[$key]) ? $form[$key] : 'error'?#38
@Damien - this value should be the form error - so it's always a string. The bug happens when you pass in an empty string - which is the default value.
Summary of the effect of the patch:
so without that 1-line change, I have to pass a separate non-empty error message for every element of the form I want to highlight with red as having an error
#39
If it takes 36+ replies to fix a one-liner, that's a good indication that we need tests for this. :P
It looks like those use cases in #18 could be translated into tests.
#40
Now with tests.
Tests give 4 fails in the absence of the functional change in the patch in #36
#41
The last submitted patch, form-error-289452-40.patch, failed testing.
#42
Re-test of form-error-289452-40.patch from comment #40 was requested by pwolanin.
#43
I'd say we should add a line of note to why we do do this, because it looks/is quite obscure. Basically we add a dummy error message here even if there was no error message, as far as I understand.
#44
Now with code comment. So, 2 lines of comments + 1 line of code change to the real codebase. All the rest relates to test cases.
#45
+ /**+ * Tests using the form in a usual way.
+ */
This is a bit cryptic, otherwise looks rtbc.
#46
code comment fixed.
#47
NIce.
#48
Great job. This needs to be backported to Drupal 6 as well.
+++ includes/form.inc@@ -1164,7 +1164,9 @@ function form_get_error($element) {
if (isset($form[$key])) {
- return $form[$key];
+ // If the value is empty, return a default error string. This is needed
+ // for _form_set_class() which tests whether the returned value is empty.
+ return $form[$key] ? $form[$key] : 'error';
}
$key = implode('][', $element['#parents']);
if (isset($form[$key])) {
What happens if this first if () condition fails? The second one is going to have the same problem and we don't notice it because the test doesn't try to set an error for an element that has a parent with #tree = TRUE. This really needs to be fixed from inside form_set_error().
<?phpif ($record) {
if ($message) {
drupal_set_message($message, 'error');
}
else {
$message = TRUE;
}
$form[$name] = $message;
}
?>
Powered by Dreditor.
#49
Added a multilevel element that uses form_error inside an element_validate callback. Confirmed the new test would have failed on the new element without change being moved to form_set_error().
#50
Same patch applied against D6 when this is fixed in D7.
#51
Ok, so this is basically back to earlier versions of the patch I think. At one point I used a string like
'error'instead of boolean TRUE to avoid having a mixed return type (which I think is ugly/bad API though there is plenty of it in core).#52
@pwolanin: Ah, I see the point about install_run_task(). I guess we could just put in a better generic message like
t('Error at @name.', array('@name' => $name));instead of mixing types or 'error'.#53
Revised patches that use the default string
t('Error in @name field.', array('@name' => $name));#54
looks fine - this string is likely never seen by a humna - but possible it might be during a batch operation.
#55
#53: 289452-form-set-empty-error-D7.patch queued for re-testing.
#56
#53: 289452-form-set-empty-error-D7.patch queued for re-testing.
#57
Test bot has has decided it doesn't want to run, but on second check this needs a reroll anyway.
#58
remove windows line endings
#59
The last submitted patch, 289452-form-set-empty-error-D7_58.patch, failed testing.
#60
Bot is being funny
#61
#58: 289452-form-set-empty-error-D7_58.patch queued for re-testing.
#62
Quick & dirty fix for this problem:
<?phpform_error($element, ' '); // used space
?>
Works for D6.
#63
Marked #1085874: Form error class is not applied if an empty message is sent to form_set_error as a duplicate of this issue and bumping to D8 to be fixed first.
#64
@marcingy: Thanks for your help.
But actually adding a formatted message when the provided one is empty is not a solution. We need to think that an empty message is not an error from the developper (if so, it is his problem and he should review his own code), but an intentional action.
The expected behavior is only to have the target element with the 'error' class, but not to create messages that may be duplicates and confuse the user. The less is sometimes the best.
I explain: figure out a table with entities mapping (for example external languages with Drupal ones). So you have in the left column the external languages names and in the right column a Drupal select field with all enabled languages.
You do not want to have any Drupal language mapped twice, so in the validation handler there is a check for that, which adds an error for all languages mapped more than once (starting from the second one). A global message alerts the user of what is wrong.
So adding a pre-formatted message would create duplicates and would really confuse the user.
Example code:
if ($values = array_filter($form_state['values']['external_languages'])) {
$unique = array_unique($values);
if ($diff = array_diff_key($values, $unique)) {
// Highlight all fields that contain errors in one operation. The message is added
// manually after.
array_map('form_error', array_intersect_key($form['external_languages'], $diff));
drupal_set_message(t('Each Drupal language should only be mapped once.'), 'error');
}
}
#65
@B_prod you will note that code is a simple re-roll I have a feeling the requirement you are describing is not what this issue is attempting to achieve, so you might want to open a seperate issue.
#66
The example above just demonstrate that if the developer throw an error with an empty message, it is his problem to explain the error to the final user, using
drupal_set_message().The point is: adding arbitrary error message if the passed one is empty will not solve this issue but create another one.
The issue discussed here is indeed what I am concerned about: if I do not handle the error class myself (here in the form theme function), the element has not the 'error' class. Here is an extract of the code used:
if (isset($form['external_languages'][$key]['#parents']) && NULL !== form_get_error($form['external_languages'][$key])) {$form['external_languages'][$key]['#attributes']['class'][] = 'error';
}//end if
#67
The use case for this is really I have 2 fields that both need to be entered. I have a combined message that say Field X and field Y must be entered. Currently I have to display the same message twice to have the error appear on both fields. The patch allows one error to be displayed and 2 fields to be highlighted.
#68
So #58 needs to be rerolled. Tagging as novice for that task.
#69
Reroll.
#70
Thanks @Niklas Fiekas. Few more things I noticed on re-read:
+++ b/core/modules/simpletest/tests/form.testundefined@@ -1075,6 +1075,52 @@ class FormsFormWrapperTestCase extends DrupalWebTestCase {
/**
+ * Test error class handling.
+ */
This should begin with "tests" rather than "test."
+++ b/core/modules/simpletest/tests/form.testundefined@@ -1075,6 +1075,52 @@ class FormsFormWrapperTestCase extends DrupalWebTestCase {
+ function testErrorClasses() {
This test method is sort of hard to scan; could we add a couple inline comments?
+++ b/core/modules/simpletest/tests/form_test.moduleundefined@@ -1429,6 +1437,76 @@ function form_test_form_form_test_state_persist_alter(&$form, &$form_state) {
+ * Form submit handler that just sets a message.
Should be "Form submission handler for formname_form()." Also, generally, we want to have @see on the form constructor to all its validation and submission handlers; and an @see on the validation and submission handlers that reference each other. Reference: http://drupal.org/node/1354#forms
#71
Ok, now beyond a blind reroll: Don't translate strings in test cases, use the testing profile. Not to forget fixing the doxygen after bugging xjm about it some more.