Am i being thick but shouldnt your new regex replace the previous regex and not as a secondary regex

@berserkerx, please have a look at #7041, where I make some substantial modifications to email addresses validation rules.

Also, please beware that according to RFCs, IP addresses as the domain-part of an email address must be enclosed between square brackets:

• user@127.0.0.1 <== wrong (actually it is formally correct, but not as an IP address)
• user@[127.0.0.1] <== correct

Also, that test at that point (and forgetting for a moment @brianteeman's remark which is IMO correct), is plainly wrong as if it will match something that looks like an IP address as the domain-part, then the local-part will not be tested, and this is obviously wrong.

The fact that this passes Travis unit tests is due to the fact that we don't test for domain parts looking as IP addresses (not true IP addresses, as per my previous comment)

Am i being thick but shouldnt your new regex replace the previous regex and not as a secondary regex
Nope. I provided fixed code in the commit proposal.
Firstly we check if domain looks numerish. Then we return the result of IP regular expression test.
smz, we check the local part before checking the domain, it's ok.

You can include the following code after "// Check the domain" but before your new lines:

// If domain looks like IP
if (preg_match('/^\[[0-9.]+\]$/', $domain))
{
  // Check it according to localpart@[x.x.x.x] RFC rule.
  $regex = '/^\[(?:[01]?\d\d?|2[0-4]\d|25[0-5])\.(?:[01]?\d\d?|2[0-4]\d|25[0-5])\.(?:[01]?\d\d?|2[0-4]\d|25[0-5])\.(?:[01]?\d\d?|2[0-4]\d|25[0-5])\]$/';
  return preg_match($regex, $domain) === 1;
}

To be honest I don't think we need this:

• what you did is not enough: validation at JS level and/or HTML5 level (probably...), will prevent you to enter such addresses. You'll need to modify in more places (see my PR...)

• in 39 years of email usage I never encountered the need for such addresses. The only valid case I can think of is for an address local to the current server, mailbox@[127.0.0.1], which is easily substituted with the universally accepted form mailbox@localhost

• that regexp is not exhaustive: 127.1 is a valid shorthand for 127.0.0.1

• it doesn't cope with IPv6 addresses

• I read (but I have no evidence for this) that using email addresses with an IP address as the domain-part is a technique used by spammers. I'm not sure about that, but anyway...

Anyway I insist that the first partial check followed by an exhaustive check is redundant and confusing: what do you think you can gain in terms of execution time? At most a couple of microseconds if the domain-part is not numeric with dots, while you loose some if the domain-part is numeric with dots, but an invalid IP address, I guess.

Edit: I based the above comment on the basis of what you wrote in your previous comment, but I now see you got rid of the redundant check in the PR...

I totally agree with you. Then it's better to disallow IP addresses as domains at all, like you did ))

Actually I didn't disallowed anything: domain-parts that "looks like" IP addresses are accepted by my regexp as they were before, and real IP addresses (within square brackets) are not allowed, as they weren't before...

Hint: to test regexps I found this very nice site: https://www.debuggex.com/ (remember to set regular expression type to PCRE if you are testing PHP regexps...)

Works as expected.

@slibbe

Works as expected.

... which is wrong!

Please test #7041

@berserkerx
Hi! If you still agree this PR is incomplete and #7041 is a better solution, can you close in favour of it? A test of #7041 would be much appreciated too...

Cheers! Sergio

If there's still discussions as to wether this PR should be tested or some other instead or none at all, please first close that discussion.
I see no use in testing if this point is not clear.

I see no use in testing if this point is not clear.

Agreed!

This is now the good PR!

I have tested this item unsuccessfully on 110af5d

If the patch is to test if a user can be set with q@7 - it still saves the user like that.

Hello @berserkerx

Thank you for your contribution.

The last comment here was on 24th October 2015. So the question is, Is this issue/pull request still valid?
If no reply is received within 4 weeks we will close this issue.

Thanks for understanding!

Due to inactivity I am closing this pull request. Thank you for your contribution, this pull request can always be reopened when you want to continue working on this.