returning negative values

Question

Hi. I have this very simple function...

function send_mail($to, $from, $from_mail, $subject, $message) {

  if ( empty($from) || empty($from_mail) || empty($subject) || empty($message) ) {
      return -1;
  }

  if ( isset ($_SESSION['last_mailed']) )
  {
    if ( $_SESSION['last_mailed'] + 180 < time() )
        return -2;
  }
  $_SESSION['last_mailed'] = time();

  if ( !validEmail($from_mail) )
     return -3;

  $from = strip_mail_headers_single($from);
  $from_mail = strip_mail_headers_single($from_mail);
  $subject = strip_mail_headers_single($subject);
  $message = strip_mail_headers_multi($message);

  return mail($to, $subject, $message, "From: $from <$from_mail>\r\n");

}

if ( !empty($_POST) ) {
  $result = send_mail($mail_to, $_POST['from'], $_POST['from_mail'], $_POST['subject'], $_POST['message']);

  if ( $result == -1 ) 
  {
      echo "<p>You need to complete all the fields.</p>";
  } 
  elseif ( $result == -2 ) 
  {
      echo "<p>You can only send one mail every three minutes.</p>";
  } 
  elseif ( $result == -3 ) 
  {
      echo "<p>Please enter a valid email address.</p>";
  } 
  else 
  {
      echo "<p>Mail sent successfully!</p>";
  }
}

I am getting some strange results. The mail() function returns, result is set to 1 and the mail is sent. However, "if ( $result == -1 )" evaluates to true for some reason and the corresponding error message is printed out. Why is this? Any ideas?

Answer 1

It's because you're returning the result of mail(), which is true in the case of success, and PHP has this really stupid stuff going on where the semantics of true are that it's the same (in weak == comparison) as every boolean-true expression, including -1.

Recommendation:

return mail($to, $subject, $message, "From: $from <$from_mail>\r\n") ? 1 : 0;

and add handling for the 0 case indicating that mail() failed.

Comparing using === instead of == would also work.

Answer 2

For starters your logic is wrong here:

if ( $_SESSION['last_mailed'] + 180 < time() )
    return -2;
}
$_SESSION['last_mailed'] = time();

// snip

elseif ( $result == -2 ) 
{
    echo "<p>You can only send one mail every three minutes.</p>";
}

This says in plain-English, if more than 3 minutes have elapsed since the last email was sent, then return -2.

Secondly the mail function in PHP returns a bool true or false. Do not try to compare this to -1.

Answer 3

In the first line, where you're checking for empty values, you're forgetting to check $to

if ( empty($from) || empty($from_mail) || empty($subject) || empty($message) )

Which then skips that if-condition because, even if it's blank, you're not checking for it and so nothing's really wrong.

Then when you get to the mail() function, the email is successfully sent, regardless in this case of no to name and then returns the boolean value of true or 1.

Additionally, because you're testing this:

if ($result == -1)

It will always evaluate to true because in the world of PHP, boolean types are true unless they're false or empty.

Change those checks to quoted, like so:

if ($result == "-1")