Validation of email addresses is a huge subject of Discussion.

This is an interesting essai about the email filter quality and variants:
http://www.regular-expressions.info/email.html

Due to the variety of answers to this simple questions we should leave some decisions to the user which way to go.

We should collect here additional references to good discussions or papers and then think about a clean futureproof solution. Possibly for the next drupal core too.

In my opinion we should:
- Check for standard validation (which already is subject of many discussions how to check...)
- Optionally trigger additional modules to check address if available.
Custom modules even should have the possibility to allow mails which originally failed checking.

Core provides:
http://api.drupal.org/api/function/valid_email_address

From a sending recursion issue #780132: Mail/Send error leads to infinite loop and cron crash we have the suggested mail validation project:
http://drupal.org/project/email_verify
^^ which is currently abandoned and not updated for a year. There are critical issues!

CommentFileSizeAuthor
#8 simplenews_948894_simplenews_valid_email_2.patch589 bytesMarkus Winand
#5 simplenews_948894_simplenews_valid_email.patch4.98 KBsimon georges
#3 simplenews-emailverify.patch592 bytesjmrivero

Comments

jmrivero’s picture

jmrivero commented

Ok, here goes the first stone, Email Verify integration first try.

In simplenews.subscription.inc, line 168.
From this:

/*
 * FAPI BLOCK subscription form_validate.
 */
function simplenews_block_form_validate($form, &$form_state) {
  if (!valid_email_address($form_state['values']['mail'])) {
    form_set_error('mail', t("The email address you supplied is not valid."));
  }
}

To this:

/*
 * FAPI BLOCK subscription form_validate.
 */
function simplenews_block_form_validate($form, &$form_state) {
	if(module_exists("email_verify")){
		//Same validation as in email_verify.module on line 33
		if($validation = module_invoke("email_verify","check",$form_state['values']['mail']))
			form_set_error('mail', $validation);
	}
	else if (!valid_email_address($form_state['values']['mail'])) {
    	form_set_error('mail', t("The email address you supplied is not valid."));
  }
}
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Active » Needs work

Let's think about adding a custom function to implement the whole emailcheck variant - e.g. simplenews_email_verify(). It would implement the switch and add reusability of the code.

Your code is a great start but it only addresses one single case in simplenews.
We need to consider all other sn form cases where email addresses could be entered. Please check and complete. That's also why i thought about the API to check and reuse the API instead of copying the validation code.

BTW: How about providing a clean patch against DRUPAL-6--2 as an attachment? We should avoid adding too much code in the issue itself and it would allow me to apply your changes without any manual work.

Thanks!

jmrivero’s picture

jmrivero commented
StatusFileSize
new simplenews-emailverify.patch592 bytes

SN is using valid_email_address function for all email validations right?
if that is the case it wont be hard to create a simplenews_email_verify function and change all valid_email_address calls for the new one.
Where do you think that simplenews_email_verify should be implemented, simplenews.module?

#1 code patch attached ( my first patch so please verify file encoding and sintax).

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

I did a quick search in the code and find:

dev:/var/www/drupal/simplenews-6--2/sites/all/modules/simplenews# grep -Rn "valid_email_address" .
./simplenews.subscription.inc:172:  if (!valid_email_address($form_state['values']['mail'])) {
./simplenews.subscription.inc:294:  $valid_email = valid_email_address($form_state['values']['mail']);
./simplenews.subscription.inc:679:  $valid_email = valid_email_address($form_state['values']['mail']);
./simplenews.admin.inc:302:  if (!valid_email_address($form_state['values'][$field_name])) {
./simplenews.admin.inc:516:    if (valid_email_address($email)) {
./simplenews.admin.inc:680:    if (valid_email_address($email)) {
./simplenews.admin.inc:983:  if (!valid_email_address($form_state['values']['simplenews_from_address'])) {
./simplenews.module:381:            elseif (!valid_email_address($mail)) {

All occurrences need to be checked for extension coverage.

simon georges’s picture

simon georges commented
Status: Needs work » Needs review
StatusFileSize
new simplenews_948894_simplenews_valid_email.patch4.98 KB

Hi,

I'm providing a patch containing :
- simplenews_valid_email_address() function, containing in fact the patch similar to the one in #3
- replace every call to core valid_email_address() function by a call to this one

Best regards,

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Reviewed and committed to dev.

Now it's much more simple to even add more extended mail format checker or even add a hook.

Status: Fixed » Closed (fixed)

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

Markus Winand’s picture

Markus Winand commented
Category: feature » bug
Status: Closed (fixed) » Needs review
StatusFileSize
new simplenews_948894_simplenews_valid_email_2.patch589 bytes

It seems like the previous patch has a little glitch.

The simplenews_valid_email_address expects true/false while email_verify returns null/error-msg.

Minimalistic patch attached.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

When checking email_verify, i even see
http://drupalcode.org/project/email_verify.git/blob_plain/refs/heads/6.x...

function _email_verify_check($mail) {
  if (!valid_email_address($mail)) {
    // The address is syntactically incorrect.
    // The problem will be caught by the 'user' module anyway, so we avoid
    // duplicating the error reporting here, just return.
    return;
  }

While i'm strongly against this API definition - it should still provide a full check API, we need to check for this additionally in simplenews_valid_email_address().
Else, we'll always accept generally wrong formatted mails.

bensey’s picture

bensey commented

Hi there,

not sure where I should be reporting this, but in a search for this issue, this was the closest and most relevant result I found.

This is not a 2.x-dev issue either but I can't find any reference anywhere to it being fixed in 2.x. I'm running 6.x-1.3.

Anyway, I manage a rather busy site that receives quite a lot of new subscriptions, and I'm constantly finding sending errors to addresses ending in a full-stop eg. "me@mydomain.com."

Could this be added to Simplenews' validation, or should it maybe be added to core (or is it already), or could they be slipping through from my client's use of 'mass subscribe'?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Mass subscribes should also be validated like public subscriptions. Are we missing it currently?

We don't want to validate custom rules. This should happen via a clean/separate validation module.
If our current implementation is not enough, we should add a email validation hook you could implement in a custom module...
However such a feature should also be provided by drupal core registration email check. So we'll try to follow core as tight as possible.

rmcom’s picture

rmcom commented

For some, the patch in this comment may provide a solution.

http://drupal.org/node/1291774#comment-5957960 (patch waiting to be ported).