In the api docs on is stated that the return value is a boolean.
This not true. The return value is an (dangerous) integer 1 or 0.

Members fund testing for the Drupal project. Drupal Association Learn more


alexanderpas’s picture

Status: Active » Needs review
705 bytes
697 bytes

patches are here ;)

alexanderpas’s picture

should we note that it can also return FALSE when an error occurred?

also, i've added the comment to the 4.6 and 4.7 documentation.

the comment can be removed from the D6 documentation after it has been fixed ;)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Both of these patches look OK to me. The return values could have been wrapped in longer lines, but the format here does line them up nicely, so I won't complain.

Just as a note, this is a Drupal 6 and earlier issue only. If applied to Drupal 6, the version should be set to D5 so it can be applied to Drupal 5.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

As noted, FALSE is still a possible return value. So the patch is not ready.

jhodgdon’s picture

Hmmm... says that preg_match will return 1 if there is 1 match, 0 if there are 0 matches, and FALSE if there is an error.

One way to get an error would be if the regular expression has an error in it. But the regex doesn't have an error in it (we know it works and no way for the caller of valid_email_address to change it).

I suppose that probably if $mail is an array, object, or NULL, that might make an error? Should test...

Anyway, agreed: if we think it is possible to get an error output, we should add a note explaining what types of conditions on $mail would generate an error.

jhodgdon’s picture

Status: Needs work » Needs review
951 bytes

I ran some tests on my test box, running PHP 5.x

If you pass in a NULL value for the string in preg_match, you get a 0 return, just as you do if you pass in a string that doesn't match the regex.

If you pass in an array, you get a FALSE return (and a warning on the screen, if you have PHP/Apache set up to display PHP warnings).

I also tested the valid_email_address function itself, As currently written, it returns 0 if the input is an empty string.

So, revised doc patch is attached, which I think documents the actual behavior of the current function in D6.

joep.hendrix’s picture

$retval = false;
if (preg_match("/^$user@($domain|(\[($ipv4|$ipv6)\]))$/", $mail) === 1) {
  $retval = true;
return $retval;
alexanderpas’s picture

+++ includes/	6 Oct 2009 15:44:54 -0000
@@ -939,14 +939,15 @@
+ *   1 if the email address is valid, 0 if it is invalid or empty, and FALSE if
+ *   there is an input error (such as passing in an array instead of a string).

no need to define what's an input error, as we've defined the type already in the @param

This review is powered by Dreditor.

The code is frozen already, we can't change it as it might break some stuff out there, also your code contains some bugs.

jhodgdon’s picture

@alexanderpas: What wording would you suggest? I thought just saying "FALSE if there is an input error" was not enough, as it led me to wonder what would actually cause such an error.

jhodgdon’s picture

bump. Someone just reported this again:
#769330: Documentation problem with valid_email_address

Can someone please review again and/or suggest better wording?

jhodgdon’s picture

Anonymous’s picture

I would simply report that the returned value is 1 when the email address is valid, or 0 when the email address is not valid.

I agree with alexanderpas that saying that the function returns FALSE if its parameter is an array doesn't make sense, once the parameter is described as a string.
Drupal function descriptions don't normally report what happens if the parameter passed to the function is not correct.

jhodgdon’s picture

RE #12: The patch without the mention of FALSE was rejected by the committer in #4 above. So it needs to have FALSE in there.

Steven Jones’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #6 looks good to me.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Looks like does not apply to Drupal 7 as it is implemented differently ( Committed to Drupal 6.

Status: Fixed » Closed (fixed)

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