Ok, this may come across as blunt or harsh, but IMO, that's what CodeReview has to do, as I explained earlier here.
So please, keep that in mind while reading this post.
Before I even start dealing with your code, I'd like to point out to you that you're using an unreliable, incomplete and misleading resource: w3schools. Sure, it may get some things right, but IMO, it's a misleading site. Many think the site is condoned, or even part of the W3C org. This is not the case, Please check the criticisms.
You are, of course, free to use w3schools, but be aware that it's not the best resource out there.
Is it secure?
No. I'd like to be kind about this, but that wouldn't help you. No, your code is still vulnerable to things like mail injection. I could easily copy paste some mail headers into your form and have your server, seemingly, forward a message sent yesterday by your rich, recently so tragically deceased alienated uncle in Barbados who left you his entire fortune.
I've included a link on mail injection in the bottom of this answer. This is an important issue that you need to address, still.
Now, the code:
I'll be going over your code block by block, sometimes even line-by-line:
<?
Don't use the short tag! Never. Period. It's not a lot of effort to write <?php
instead, and it's far more reliable (<?php
won't be confused with the odd <? xml...
), and doesn't rely on the short-tags being enabled on your server.
$name = filter_var($_POST[name], FILTER_SANITIZE_STRING);
$email = filter_var($_POST[email], FILTER_SANITIZE_EMAIL);
$phone = filter_var($_POST[phone], FILTER_SANITIZE_NUMBER_INT);
$usermessage = filter_var($_POST[message], FILTER_SANITIZE_STRING);
$businessmessage = filter_var($_POST[businessmessage], FILTER_SANITIZE_STRING);
Ok, this block is essentially doing the same thing several times, and suffers from the same issue throughout.
First off, when setting the error reporting level to the Correct debugging levels of E_STRICT | E_ALL
, you'll see a ton of notices and warnings pop up:
- PHP Notice: Use of undefined constant name - assumed 'name'
This means you forgot to quote the array keys, PHP will helpfully assume you meant 'name', but it will issue a notice. This makes your code look sloppy and amateurish. More over, it costs resources, slowing your code down.
- PHP Notice: Undefined index: name
If your script is being executed and none, or not all of the post parameters have been sent, you're still accessing those keys in the $_POST
super global. At no point are you checking if there has been a POST request.
This can produce the notice above, for each time you access the POST array. Add some basic checks:
<?php//full tag
if (!$_POST) exit();//no post request, no magic!
$name = isset($_POST['name']) ? filter_var($_POST['name'], FILTER_SANITIZE_STRING) : null;
Where your usage of filter_var($email, FILTER_VALIDATE_EMAIL)
is to be commended, your code:
$phone = filter_var($_POST[phone], FILTER_SANITIZE_NUMBER_INT);
is not the best of ideas. Now I don't know how you get the input from the user, but if I were to punch in "+10800123456", I'd translate that +
to double 0, to standardize the input.
Apart from that FILTER_SANITIZE_NUMBER_INT
will clean up input like "123.313.456,1", removing the comma and dots, but it will leave the dashes and plusses in "+123-313+456-1" untouched. If I were you, I'd simply do something like:
if ($phone{0} == '+')
$phone = '00'.$phone;
preg_replace('/[^0-9]+/','',$phone);
This expands a leading + to double zeroes and removes everything Except digits from the string. You can then easily check the length, to determine (roughly) the validity of the input. If it's 3 digits long, it probably isn't valid.
mail
is good for testing, less so for production
Sure mail
is easy (once you get your server settings worked out), and does what it says on the tin: it sends an email.
However, sometimes you might require an alt-body (text and HTML), sometimes you may want to add attachments. Writing those headers all the time is a bit of a faff, and has been done before. Good programmers are lazy: they won't bother writing code that exists, and is free to use. The PHPMailer
class is in common usage and is very easy to set up, and quite versatile. Though, as you can see here, still somewhat open to mail injection.
In terms of security, PEAR::Mail
is still the better option (though not in terms of clean code IMO). Read through the link, and learn about mail injection some more.
E_STRICT | E_ALL
for debugging, and show the errors! The notices are there to help you, don't ignore them \$\endgroup\$