5
\$\begingroup\$

I have the following PHP function, which is admittedly not quite as robust as it could be (I'm missing some sanity checks including a proper one for $sendTo for example; odd choices in text formatting, too) but I don't see why it shouldn't work. A version of it certainly worked the other day, though I made a few small changes (allegedly for the better!) today. When the function executes, my condition if (mail(etc)) indeed appends a success string to my content.

<?php
$FROM_EMAIL = "foo@bar.com";
$email = "foo@bar.com";
$content = "";
$emailMessage = "";
$sendTo = "";

if ($_SERVER['REQUEST_METHOD'] === 'POST') {
  foreach ($_POST as $key => $value) {
    $value = (string) $value;
    if ($key === "DESCRIPTION_OF_ITEMS")
      $value = str_replace("\r", '<br>', $value);
    if ($value !== "" && $key !== "hiddenField" && $key !== "button2" && $key !== "cloneAdmin")
      $emailMessage .= "<strong>$key</strong> - $value <br/>\n";
    if ($key === "EMAIL")
      $sendTo = $value;
  }

  if ($emailMessage !== "") {
    $content .= "<h3>Thank you for submitting your registration form... we'll get back to you shortly.</h3><p>You can expect to receive an email with the following registration information:</p>";
    $content .= $emailMessage;
    if (mail("foobar@bazbat.com,$sendTo", "Foobar Registration Form", $emailMessage, "MIME-Version: 1.0\r\nContent-type: text/html; charset=iso-8859-1\r\nFrom: $FROM_EMAIL\r\nReply-to: $email")) {
      $content .= "<h3>Email successfully sent!</h3>";
    } else {
      $content .= "<h3>We encountered an error and the email was not successfully sent.</h3>";
    }
  } else {
    $content .= "No Email message";
  }
  $content .= "</p>";
} else {
  $content .= "</p><h3>A blank or invalid form was sent; your submission has not been successful.</h3>";
}
?>

For the record, I tried the mail() function in its most basic form as well:

mail('foo@bar.com', 'My Subject', $emailMessage);

and it didn't make a difference. The successful trial earlier this week used the additional header parts.

Unfortunately I don't have access to the PHP logs; I only have FTP access. Also as you may have been able to guess from the coding style, I'm generally more of a JavaScript guy, so I'm not sure what my options are for capturing errors without the PHP log.

Is it possible that there's just a problem (or security measures?) with the mail server? That the PHP code is fine and the problem is elsewhere? Maybe my own mail server is flagging spam and not even passing it into my junk box?

\$\endgroup\$
3
  • \$\begingroup\$ No idea, the code looks ok. Well, I can post some paranoid ideas, but it seems to me that the problem is outside. Anyway, try diff and figure what have you changed actually? \$\endgroup\$ – Michael Apr 14 '12 at 8:43
  • \$\begingroup\$ Thanks for looking, Michael. Diff just reveals some bad code that I inherited; $$var = $var and stuff. I rolled back to the original version and had the same problem, making it even more likely that it's outside of this code. Thanks again! \$\endgroup\$ – Greg Pettit Apr 15 '12 at 14:31
  • \$\begingroup\$ No problems! Good luck! \$\endgroup\$ – Michael Apr 16 '12 at 0:39
3
\$\begingroup\$

Don't know if you wanted a full review or not so I supplied one since I was going over it anyways. Not really much wrong, mostly aesthetics. I could find no errors in your code, but maybe one of these fixes will ninja your problem away. You never know, the all-powerful PHP god's might be feeling better today :)

POST Data

Not sure if better, but if I'm looking for $_POST variables, I usually just do the following. Up to you though, nothing wrong with what you've got.

if($_POST) { etc... }

You should have some sort of sanitizer for those user inputs though. This is the only real issue I see with your code. Take a look at filter_input if you have PHP 5.2 or greater, otherwise you'll have to take a look around at what others are doing. It is never a good idea to take raw user input and run it in your program. That can lead to all sorts of nasty things.

Ignoring Keys

If you have a list of keys, or other items that you want to exclude, make them into an array and then check against it. Its cleaner than having a bunch of if statements and allows for extension later.

$exceptions = array(
    'hiddenField',
    'button2',
    'cloneAdmin',
);

if ($value !== "" && ! in_array($key, $exceptions)) { etc... }

Also, please use curly braces everywhere and not just on outter or large if statements. Yes PHP can work without them, but debugging your code will become very difficult later. Many might argue this point, but it truly is easier to read.

Since you are already checking if the value is blank, you might as well move that to the beginning and place all other checks inside it. What's the use of running all that code if the value is blank or a key you don't want to process? Better yet, just add the following to the top of the foreach loop and it will skip those records entirely.

if($value === '' || in_array($key, $exceptions)) { continue; }

Multiple If Statements

Don't use if statements for checking variables that you know a range of values for. Switch statements are faster and easier to read.

//Don't do this
if($key === 'DESCRIPTION_OF_ITEMS') { etc... }
if($key === 'EMAIL') { etc... }

//Do this
switch($key) {
    case 'DESCRIPTION_OF_ITEMS':
        //etc...
    break;
    case ''EMAIL':
        //etc...
    break;
}

Long Variables

Heredoc will make your long variables easier to read.

$content .= <<HTML
<h3>Thank you for submitting your registration form... we'll get back to you shortly.</h3>
<p>You can expect to receive an email with the following registration information:</p>
$emailMessage
HTML;

Long Strings as Arguments

If you are going to use long strings in functions you should replace them with variables, even if you aren't going to use those variables again. It makes it cleaner and easier to read.

$recipients = array();//You'll see why I used an array soon
$recipients[] = "foobar@bazbat.com";
$recipients[] = "$sendTo";
$recipients = implode(', ', $recipients);//See :)

$subject = "Foobar Registration Form";

$headers = array();//Same as recipients
$headers[] = "MIME-Version: 1.0";
$headers[] = "Content-type: text/html;";//This line didn't originally have "\r\n" so maybe this was your problem?
$headers[] = "charset=iso-8859-1";
$headers[] = "From: $FROM_EMAIL";
$headers[] = "Reply-to: $email";
$headers = implode("\r\n", $headers);

mail($recipients, $subject, $emailMessage, $headers);//See much cleaner

Sorry I couldn't be of more use in finding your error. Since you are indeed receiving a success string, I would say that your send to line is just wrong somehow. Try dumping its contents before sending the message to manually check if anything is wrong. Only other thing I can think of is that maybe your server doesn't have the mail() function installed. Though, since you said it succeeded at some point, I don't know what to tell you. Check php_info() anyways to see if its enabled. As for if this is the wrong stackoverflow subsite? Not really. It could have gone on stackoverflow and no one would have complained. Here you just get the added benefit that I reviewed your code too :)

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for the super-detailed response and code review. Since it's largely inherited code, it becomes a 2-step process for me: 1. fix some crap they've provided (trust me, it was even worse!) and 2. fix the stuff you've suggested -- in particular, I like the organizational trick of using an array and then imploding it. Nice! I'll let you know if any ninjitsu is released from the changes. \$\endgroup\$ – Greg Pettit Apr 21 '12 at 20:22
  • \$\begingroup\$ No prob. The implode trick actually wasn't mine, I found it on the PHP manual. Lots of good tricks there :) Hope you find the issue. \$\endgroup\$ – mseancole Apr 21 '12 at 23:22
  • \$\begingroup\$ Just realized I had put the wrong PHP version in there for filter_input, went ahead and changed that. (Should have been >=5.2) \$\endgroup\$ – mseancole Apr 23 '12 at 16:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.