FAPI: Error class is not set if error message is empty

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.

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.

Forgot to set status. Reduced priority since the error does not actually occur.

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.

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 ?

Setting back to 'code needs review' until we understand the use case.

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Why would we want errors without messages? IMO that's confusing for users at least.

I don't see why we shouldn't show an error message to tell people their file upload has gone wrong?

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

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

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?

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

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:

form_set_error('testfield','Fields must have unique values');
form_set_error('testfield2');
form_set_error('testfield3');
//etc...

rather then the workaround

form_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:

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

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

function 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**

fixing the patch file.

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.

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.

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.

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.

Tagging this Usability, as I think this falls into that category and should be allowed for the post code freeze issues/

same bug here: http://drupal.org/node/135867#comment-2455910

maybe we'll mark that a dup since this has a 7.x patch?

this is surely a bug

This looks like a straight bug. Let's fix it and backport.

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.

oops - left an extra line in.

Actually - I kind of like the solution here better: http://drupal.org/node/135867

Here's the patch.

If we are there, why not is_string($form[$key]) ? $form[$key] : 'error'?

@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

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.

Now with tests.

Tests give 4 fails in the absence of the functional change in the patch in #36

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.

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.

+  /**
+   * Tests using the form in a usual way.
+   */

This is a bit cryptic, otherwise looks rtbc.

code comment fixed.

NIce.

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

    if ($record) {
      if ($message) {
        drupal_set_message($message, 'error');
      }
      else {
        $message = TRUE;
      }
      $form[$name] = $message;
    }

Powered by Dreditor.

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

Same patch applied against D6 when this is fixed in D7.

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

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

Revised patches that use the default string t('Error in @name field.', array('@name' => $name));

looks fine - this string is likely never seen by a humna - but possible it might be during a batch operation.

#53: 289452-form-set-empty-error-D7.patch queued for re-testing.

#53: 289452-form-set-empty-error-D7.patch queued for re-testing.

Test bot has has decided it doesn't want to run, but on second check this needs a reroll anyway.

remove windows line endings

Bot is being funny

#58: 289452-form-set-empty-error-D7_58.patch queued for re-testing.

Quick & dirty fix for this problem:

form_error($element, ' '); // used space

Works for D6.

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.

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

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

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

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.

So #58 needs to be rerolled. Tagging as novice for that task.

Reroll.

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

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.

I may be missing something, but wouldn't changing the behavior of form_get_error fix this entirely?

Instead of

if(isset($form[$key])){
  return $form[$key];
}

we could use

  return isset($form[$key]);

This would still allow empty messages (the default), and as far as I know, the only function calling form_get_error is _form_set_class.

*edit*
Otherwise, I'd like to put by vote in for the suggestion in #17. It seemed to have been forgotten about after comment #33, but I find it cleaner since it would allow form_get_error to return the actual message that was passed (in this case, an empty string).

The facility/feature to throw a validation error on an element or possibly even the entire form, without specifying an error message is used by many contributed modules.

Doing so can sometimes be the only possible action to prevent a form from being submitted. The form validation error ["message"] may be handled in a completely different way - hence an empty message string.

I don't think I support removing that facility/flexibility.

_form_set_class() should properly test whether there is any error instead.

Seems like this probably has been fixed with #2131851: Form errors must be specific to a form and not a global

Mentor updating issues.

I'm gonig to do reroll of this

Working on reroll right now.

I have grepped in /core for _form_set_class and did not get any results. It looks like the function is now called _form_set_attributes.

The logic for setting the error class is now wrapped with if (isset($element['#parents']) && isset($element['#errors']) && !empty($element['#validated'])) {

I have not looked at how isset($element['#errors']) is set and whether this addresses the issue.

Can somebody test this again to see if the fix is even necessary in D8? I'm not clear on the steps that are necessary to reproduce the problem.

So, just like @vegantriathlete just said this doesn't seem to make sense anymore. I was trying to see if maybe righting a patch for this. But saw that there is a lot of work needed first on the form_test module that probably should go first: https://drupal.org/node/1971384

I set the message to empty:

<?php
   public function setErrorByName($name, array &$form_state, $message = '') {
+    $message = '';
?>

and tested submitting the user create form with mismatching passwords.

The error class was correctly applied to the password fields.

"Form validator test" has some unit tests already.

Indeed this seems like a no issue anymore on D8, specially now that there are tests covering this case. So we should switch back to D7.