Anonymous user e-mail validation should set form error, not just write a message [#713020]

Hiya,

Just a small request - when anonymous checkout is enabled and the user has to provide his e-mail address, the email is validated in uc_cart_checkout_pane.inc, but if the validation fails, it just writes an error message with:
drupal_set_message(t('You must enter a valid e-mail address.'), 'error');
and the validation actually succeeds.

Could you change this and others to something like
form_set_error('mail', t('You must enter a valid e-mail address.'));

That way, the field would be properly highlighted and the form state would be correct.

Let me know if you need more details.

Thanks a lot,
Martin

Comment	File	Size	Author
#4	co_validation.patch	2.8 KB	arski
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

TR CreditAttribution: TR commented 17 February 2010 at 03:15

Any chance you could roll a patch for this?

Comment #2

arski CreditAttribution: arski commented 18 February 2010 at 18:47

Well, I just noticed you got a lot of places in there that set a usual error message instead of a proper form error.. any particular reason for that? I guess it would make sense to fix this for all the fields at the same time, I just don't know if this will have any consequences..

Comment #3

TR CreditAttribution: TR commented 18 February 2010 at 18:57

I didn't write the code, but I suspect the reason form_set_error() wasn't used was first because the authors had limited Drupal experience when they wrote this, and second because of the way checkout panes are structured, form validation and error handling is a bit problematic, although it should work just fine on the billing and shipping address panes.

It would be great if you could write a patch that would deal with all the fields. Don't worry too much about side effects, that's what the review process is for.

Comment #4

arski CreditAttribution: arski commented 19 February 2010 at 16:08

File	Size
co_validation.patch	2.8 KB

All right, here goes. Let me know if there's anything wrong with it.

Cheers

Comment #5

arski CreditAttribution: arski commented 19 February 2010 at 17:06

Version:

6.x-2.2

» 6.x-2.x-dev

I was told patches should be pointing to dev :)

Comment #6

TR CreditAttribution: TR commented 20 February 2010 at 19:15

Status:

Active

» Needs review

It's preferable to develop and generate patches using the dev version because there may have been other changes to this section of code between 2.2 and 2.2-dev, and if there were the patch would fail with a conflict. Or, this problem may have been already fixed in a different manner, in which case there is no need for a patch.

But the "version" assigned to this issue should correspond exactly to the version you used to generate the patch. I'll leave that for you to change if necessary, because you're the one who knows what version you used.

Also, when you post a patch, set the issue status to "needs review".

Comment #7

arski CreditAttribution: arski commented 21 February 2010 at 11:49

Hi,

Thanks a lot for the explanation, I think I got the idea now.

The patch was indeed applied to the 2.2 version first, but having applied the same changes to the -dev version, the resulting patch was exactly the same, so everything is as it should be.

Awaiting review :)

Cheers,
Martin

Comment #8

kostajh CreditAttribution: kostajh commented 26 March 2010 at 19:37

I tested the valid e-mail address part of this patch on anonymous user checkout of a site running Ubercart 2.2. The patch works for that scenario, although "test@example" is returned as a valid email (while "test" is not). That seems like a problem with valid_email_address() though.

Thanks Martin.

Comment #9

TR CreditAttribution: TR commented 26 March 2010 at 20:04

@kostajh: Thanks for testing. Yes, valid_email_address() says "test@example" is valid, because according to the RFC it *is* valid (root@localhost is an example where this occurs in real life). valid_email_address() has a lot of other problems, and there are some long-open threads trying to improve it, but I think Ubercart should stick to using valid_email_address() simply because that way e-mails are handled the same as in Drupal, and because any community improvement in valid_email_address() will immediately benefit Ubercart, which would not be true if we wrote our own.

I plan to commit this patch after I've had a chance to test it myself...