When mailhandler fetches more than one mail, and validate them, if a mail reports a validation error the same error will be "duplicated" to all messages after that.

The problem in in mailhandler_node_submit function, where it does:

  node_validate($node);
  $error = form_get_errors();

It does the 2 lines above for each mail, but nowhere there is a reset of form_errors, so after a validation for a message reports an error (via form_set_error), that error message will remain for all remaining mails.

The GREAT problem is that i can see no way to reset form_errors...

The only way seems to patch form.inc function "form_set_error" to implements a reset of the static $form variable.

What do you think?

CommentFileSizeAuthor
#16 mailhandler-validation-271975-16.patch2.47 KBIan Ward
#8 mailhandler-1.patch810 bytesgotheric
#6 mailhandler.patch2.15 KBgotheric
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Perhaps we need to submit the node using drupal_execute()? Would that still do all our validation? Would it reset errors? Ugh.

gotheric’s picture

gotheric CreditAttribution: gotheric commented

No, i don't think that will solve the problem.

The core doesn't support multiple validations, and i don't think of a "clean" way to do that.
(Or maybe i'm missing something...)

However i submitted a core support request:
#272042: There is no way to reset validation errors

gotheric’s picture

gotheric CreditAttribution: gotheric commented

Assuming the problem won't be fixed on 5.x (and i think 6.x too) i see only one way to fix the problem without patching the core.
It's not a "smart" way but it should work.

We must work with error messages set by "drupal_set_message" (form_set_error calls it for each validation error, and drupal_set_message data is contained on $_SESSION['messages'], which is visibile outside the function).

This should be something like that (this is just a quick example, if you think it should be a good way i'll do the patch):

//Before mail loop, i save error messages BEFORE mail validation
$saved_errors = $_SESSION['messages']['error'];
....

// Inside the loop
$_SESSION['messages']['error'] = array();
node_validate($node);
$validation_errors = array();
if (count($_SESSION['messages']['error'])) {
  $errors = form_get_errors();
  foreach ($_SESSION['messages']['error'] as $message) {
    if ($key = array_search($message, $errors)) {
      // This is a validation error
      $validation_errors[$key] = $message;
    } else {
      // Not a validation error (but an error, i'll print it)
      $saved_errors[] = $message;
    }
  }
}
// Now in $validation_errors we have the real errors
...

// After the loop, i restore real error messages
$_SESSION['messages']['error'] = $saved_errors;

Only an example that should be tuned (i should also catch errors on "..." code...).

Could that be a solution (hoping and waiting for a real fix on 7.x)?

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Yes, that looks like a clever solution. I'd welcome such a patch. Please add a comment so we know why we have to do this madness.

For 7.x, please help on a related patch in. See #254491: Standardize static caching

gotheric’s picture

gotheric CreditAttribution: gotheric commented

Sounds interesting, i'll look at it.

I'll check & submit mailhandler patch next week (monday or tuesday).

gotheric’s picture

gotheric CreditAttribution: gotheric commented
Status: Active » Needs review
FileSize
mailhandler.patch2.15 KB

Here is the patch.
(I think it should be applied easily to 6.x branch too).

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Fixed

I've committed this to 5.x and 6.x. I'd love some feedback in the next few days. If I hear no negative feedback, I will make new releases. I am not setup for testing this now, so I've committed blindly. Please give feedback, folks.

gotheric’s picture

gotheric CreditAttribution: gotheric commented
FileSize
mailhandler-1.patch810 bytes

Just a little mod, to avoid having the $_SESSION['messages']['error'] with an empty array (standard theme will display an empty error box).

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Committed. Please use braces even on 1 line if() statement. Just part of our coding standards.

I am going to make a new release of mailhandler since I can't seem to get any feedback otherwise.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I've just been setting mailhandler up for the first time, had it checking the inbox but no content was being created. Since I was adding more and more unread variations of the same message with different versions of the commands, I'd obviously ended up triggering this bug, new release of mailhandler fixed it and has probably saved me hours and hours of head scratching. Thanks!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Version: 5.x-1.2 » 6.x-1.x-dev
Priority: Critical » Normal
Status: Closed (fixed) » Active

#180063: No way to flush form errors during iterative programatic form submission has been committed to D6. we can undo the mess in mailhandler_node_submit() now.

z.stolar’s picture

z.stolar CreditAttribution: z.stolar commented

@Moshe: How? Is it by calling form_set_error with ($reset = TRUE) after node_validate?
Can you show me the way? (I'll walk it :-) )

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Yes, thats the idea

Email me when you have a need because I am rarely reading mailhandler issue mail these days.

Ian Ward’s picture

Ian Ward CreditAttribution: Ian Ward commented
Status: Active » Needs review

It looks like this can all boil down to:

    // Reset the static cache 
    form_set_error(NULL, '', TRUE);
    node_validate($node);
    $error_messages = form_set_error();

I am working on a 6.2 version which will hopefully be ready enough to commit back to CVS and I've tested the above in it which works. I tested by sending three emails, the first two with validation errors and the last without, and the last one passed validation fine while the first two were correctly found invalid. I'll likely commit this after another point release in 6.1.

Ian Ward’s picture

Ian Ward CreditAttribution: Ian Ward commented
FileSize
mailhandler-validation-271975-16.patch2.47 KB

Attached is the patch per #15

Ian Ward’s picture

Ian Ward CreditAttribution: Ian Ward commented
Status: Needs review » Patch (to be ported)

Applied to 6.1. Should be ported to 7.1

cor3huis’s picture

cor3huis CreditAttribution: cor3huis commented
Version: 6.x-1.x-dev » master

Set to HEAD so the port is not forgotten, also since work on a 7.1 is unlikely as I understood from other issues

cor3huis’s picture

cor3huis CreditAttribution: cor3huis commented

Bump, unclear if patch is integrated. If so plz close this issue, if not it would be lovely to have it integrated in main.

Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell commented
Version: master » 7.x-1.x-dev
Dane Powell’s picture

Dane Powell CreditAttribution: Dane Powell commented
Status: Patch (to be ported) » Fixed

Yes, this has been committed to 7.x-1.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.