- Due to the nested and long conditions the code becomes hard to follow
at some points. Early returns prevent nesting of conditions.
- Some code is duplicate (step 9)
- Possibly some values could be overwritten by url-forging (depending on the server-configuration). (step 5)
I'll do a step-by-step refactoring here with some explanations.
Move everything in a function. This eases early-return.
function sendMail() {
// your code here.
}
While not necessary, it's I like to invert the conditions so the static expression stands on the left side. This avoids accidental assignment in conditions. Avoids tricky to detect bugs (though most IDEs and codesniffer emit a warning).
Using the === operator for comparison avoids bugs to unforeseen casting
function sendMail() {
if ('POST' !== $_SEVER['REQUEST_METHOD']) {
return '';
}
if ($_SESSION['token'] !== $_POST['token']) {
return '0';
}
// remaining code
}
What's the purpose of your output-cachting? I can't see any outputs being made? Trying to catch any errors? Hiding them is a bad idea, catch and handle them. If simply removed them.
- You are using
$_REQUEST instead of $_POST. Sure you do check if the request type. But this doesn't prevent anyone from submitting both, GET and POST values. And here lies a dangerous problem: If I send you a link to form.php?message=something this does get send to the server as well when you submit the form. Now GET and POST both having the key name collide. Depending on the variables_order the GET value I set overwrites your message. If you use $_POST to access your variables instead this issue can be prevented.
I have extracted a simple accessor function to check for existence of a $_POST variable and return else:
function getPostValue($key) {
return isset($_POST[$key]) ? $_POST[$key] : null;
}
function sendMail() {
if ('POST' !== $_SEVER['REQUEST_METHOD']) {
return '';
}
if ($_SESSION['token'] !== $_POST['token']) {
return '0';
}
$_SESSION['token'] = '';
$name = getPostValue('name');
$email = getPostValue('email');
$message = getPostValue('message');
$number = getPostValue('number');
$date = getPostValue('date');
$select = getPostValue('select');
$radio = getPostValue('radio');
$checkbox = getPostValue('checkbox');
if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
$response = "<b style='color: red'>Error</b>";
}
}
All those input validations go with early return again (snippet only):
$spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
if (!preg_match($spam_pattern, $name)
|| !preg_match($spam_pattern, $number)
|| !preg_match($spam_pattern, $date)
|| !preg_match($spam_pattern, $radio)
|| !preg_match($spam_pattern, $checkbox)
|| !preg_match($spam_pattern, $select)
|| !preg_match($spam_pattern, $message)) {
return '<b style="color: red">Invalid Request!</b>';
}
The $to variables and others you use as parameters for mail are only used once. Furthermore the $message variable is overwritten, removing the original content. Variable reuse makes stuff hard to track though. I just do this all as arguments in the mail function (snippet):
$sendResult = mail(
'[email protected]',
'New Message From: ' . $name,
"Name: <br/>
number: $number<br/>
date: $date<br/>
select: $select<br/>
radio: $radio<br/>
checkbox: $checkbox<br/>
Email: $email<br/>
Message: $message",
"MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
);
All those returns have their html-markup in common. We sure can extract this into a common helper:
function getResponse($message, $isError) {
return '<b style="color: ' . ($isError ? 'red' : 'green') . '">' . $message . '</b>';
}
Final remarks & further points of improvement:
- Indention of three characters is unusual. Most times its two or four.
- This can of course be further refactored: Especially creating local variables from
$_POST is a point of repetation. Maybe it could be handled as an array. That'd make mass-checking (preg_match) easier: just loop over all input variables.
- If desired, this could of course be moved into a dedicated mailer class
- The functions depend on the global state (
$_POST). A next step would be to pass the required input variables as argument (maybe an array :)). This would make this function reusable as well.
- Returning formated strings limits reusability to HTML output. Throwing expections or error codes puts this into the responsibility of the caller, not the callee.
My final result:
function getPostValue($key) {
return isset($_POST[$key]) ? $_POST[$key] : null;
}
function getResponse($message, $isError) {
return '<b style="color: ' . ($isError ? 'red' : 'green') . '">' . $message . '</b>';
}
function sendMail() {
if ('POST' !== $_SEVER['REQUEST_METHOD']) {
return '';
}
if ($_SESSION['token'] !== $_POST['token']) {
return '0';
}
$_SESSION['token'] = '';
$name = getPostValue('name');
$email = getPostValue('email');
$message = getPostValue('message');
$number = getPostValue('number');
$date = getPostValue('date');
$select = getPostValue('select');
$radio = getPostValue('radio');
$checkbox = getPostValue('checkbox');
if (!isset($name, $email, $message, $number, $date, $select, $radio, $checkbox)) {
$response = getResponse('Error', true);
}
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
return getResponse('Invalid Email Address!', true);
}
$spam_pattern = "/[\r\n]|Content-Type:|Bcc:|Cc:/i";
if (!preg_match($spam_pattern, $name)
|| !preg_match($spam_pattern, $number)
|| !preg_match($spam_pattern, $date)
|| !preg_match($spam_pattern, $radio)
|| !preg_match($spam_pattern, $checkbox)
|| !preg_match($spam_pattern, $select)
|| !preg_match($spam_pattern, $message)) {
return getResponse('Invalid Request!', true);
}
$sendResult = mail(
'[email protected]',
'New Message From: ' . $name,
"Name: <br/>
number: $number<br/>
date: $date<br/>
select: $select<br/>
radio: $radio<br/>
checkbox: $checkbox<br/>
Email: $email<br/>
Message: $message",
"MIME-Version: 1.0 r\n Content-type: text/html; charset=utf-8 \r\n From: '.$email . '\r\n"
);
if (true === $sendResult) {
return getResponse('Success! Your submission has been sent.', false);
} else {
return getResponse('Error! There was a problem with sending.', true);
}
}
