Crazy Horse
asked on
enctype="multipart/form-data" causing validation to fire
This is strange. If I do this:
The issue is now that whether I select a file or not the error message appears. Why is this?
I have a custom bootstrap file field:
<form method="post">
my form validation works fine. If I don't select a file to upload I get an error when I try submit the form. If I do select a file then I get no error. Perfect. However, for file uploads I believe I need:<form method="post" enctype="multipart/form-data">
The issue is now that whether I select a file or not the error message appears. Why is this?
if(empty($_POST['fileToUpload'])) {
$message .= "Featured image required";
}
I have a custom bootstrap file field:
<div class="form-group">
<label>Upload Featured Image</label>
<div class="input-group">
<label class="input-group-btn"> <span class="btn btn-danger">
Browse… <input type="file" name="fileToUpload" style="display: none;" multiple>
</span> </label>
<input type="text" class="form-control" readonly> </div>
</div>
ASKER CERTIFIED SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
ASKER
@ Ray, I updated the question just before you posted I think.. If I understand correctly what you were looking for.
ASKER
@ Julian, I think I understand. I changed it to:
and it seems to work now. Is that what you meant?
if(empty($_FILES['fileToUpload']['name'])){
$message .= "Featured image required";
}
and it seems to work now. Is that what you meant?
I think Julian has the right idea, but the array keys in the PHP superglobal arrays are case-sensitive, so fileToUpload != filetoupload.
The PHP web site has some "required reading" about how to do file uploads.
http://php.net/manual/en/reserved.variables.files.php
http://php.net/manual/en/features.file-upload.php
http://php.net/manual/en/features.file-upload.post-method.php
http://php.net/manual/en/features.file-upload.common-pitfalls.php
http://php.net/manual/en/features.file-upload.errors.php
http://php.net/manual/en/features.file-upload.multiple.php
http://php.net/manual/en/function.move-uploaded-file.php
The PHP web site has some "required reading" about how to do file uploads.
http://php.net/manual/en/reserved.variables.files.php
http://php.net/manual/en/features.file-upload.php
http://php.net/manual/en/features.file-upload.post-method.php
http://php.net/manual/en/features.file-upload.common-pitfalls.php
http://php.net/manual/en/features.file-upload.errors.php
http://php.net/manual/en/features.file-upload.multiple.php
http://php.net/manual/en/function.move-uploaded-file.php
ASKER
I think I got it now.
This is the check:
And this is how to set the variable which I was also struggling with:
Using that seems to insert the record into the database as expected.
This is the check:
if(empty($_FILES['fileToUpload']['name'])){
$message .= "Featured image required";
}
And this is how to set the variable which I was also struggling with:
$featured_image = $_FILES['fileToUpload']['name'];
Using that seems to insert the record into the database as expected.
and it seems to work now. Is that what you meant?Yes - the reflect script will help you to see the data that is sent up - always helps to be able to visualise the structure - good practice to use it on your forms to see what is actually being sent to the server.
SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
ASKER
@ Ray,
Thanks for the input. Just to explain a bit, this is the first time I am really working with file fields so it is easier for me to do this:
especially since I am not used to having two together i.e.: ['fileToUpload']['name']
Also, I have 11 fields that need to be entered into the database and I find it much easier to do this:
as opposed to:
In the back of my mind I realize that there could be real trouble if file upload functions are not sanitized etc. but I was just so excited that I was finally going to upload a file and insert a record into the database at once. It might sound trivial to you guys but that is totally awesome for me and I just wanted to do it! :)
Now that I have, I will certainly take a look at how to add some security measures to it.
Lastly, I like to do something like this where I can which I think perhaps justifies not just using $_POST['whatever'] but rather declaring a variable. Maybe it isn't necessary, but while I am learning I just find it easier to work with and less confusing when I have to for example, enter 11 fields into the database.
Thanks for the input. Just to explain a bit, this is the first time I am really working with file fields so it is easier for me to do this:
$featured_image = $_FILES['fileToUpload']['name'];
especially since I am not used to having two together i.e.: ['fileToUpload']['name']
Also, I have 11 fields that need to be entered into the database and I find it much easier to do this:
$stmt->bind_param("siissddsiss", $prod_name, $prod_cat_id, $prod_location_id..............
as opposed to:
$stmt->bind_param("siissddsiss", $_POST['prod_name'], $_POST['prod_cat_id'], $_POST['location_id']...........
In the back of my mind I realize that there could be real trouble if file upload functions are not sanitized etc. but I was just so excited that I was finally going to upload a file and insert a record into the database at once. It might sound trivial to you guys but that is totally awesome for me and I just wanted to do it! :)
Now that I have, I will certainly take a look at how to add some security measures to it.
Lastly, I like to do something like this where I can which I think perhaps justifies not just using $_POST['whatever'] but rather declaring a variable. Maybe it isn't necessary, but while I am learning I just find it easier to work with and less confusing when I have to for example, enter 11 fields into the database.
$email = $_POST['email'];
$safe_email = filter_var($email, FILTER_SANITIZE_EMAIL);
Suit yourself, just don't put that code snippet on your résumé!
Here's a "dictionary picture" of what's wrong:
The problem with code like this is that it contains unnecessary parts and loose ends. One day, an unexpected or uninitialized variable is going to turn up in an expression with the potential for causing a run-time error. When you've studied the process and practice of programming, you learn that the earliest part of planning your code takes the form of knowing what your inputs are and what your outputs must be. Getting from inputs to outputs with a minimal amount of variable proliferation reduces the "footprint" of your code in the symbol table and makes it possible to use automated test cases. This is a large part of the reason for object-oriented design and programming. It lets us encapsulate our code and data in ways that isolate the parts and prevent unwanted cross-interference.
Anyway, best of luck with the project! ~Ray
Here's a "dictionary picture" of what's wrong:
$email = $_POST['email'];
$safe_email = filter_var($email, FILTER_SANITIZE_EMAIL);
After you run that code, you have not two, but THREE variables, one with global scope and two with local scope. Only one of them has been filtered, and unless you test the return value in $safe_email, you don't know whether that variable contains an email address or FALSE. And there is no reason you would you want to put $email into the symbol table at all.The problem with code like this is that it contains unnecessary parts and loose ends. One day, an unexpected or uninitialized variable is going to turn up in an expression with the potential for causing a run-time error. When you've studied the process and practice of programming, you learn that the earliest part of planning your code takes the form of knowing what your inputs are and what your outputs must be. Getting from inputs to outputs with a minimal amount of variable proliferation reduces the "footprint" of your code in the symbol table and makes it possible to use automated test cases. This is a large part of the reason for object-oriented design and programming. It lets us encapsulate our code and data in ways that isolate the parts and prevent unwanted cross-interference.
Anyway, best of luck with the project! ~Ray
ASKER
Darn, I didn't know it was that bad!
It just seemed to make more sense to me. Could you possibly show me the right way for these 2 examples and then I will close this out? If you have time.
And if I look at one of your examples from you article, aren't you doing a similar thing by doing this?
You have used $email_address multiple times so I am just trying to understand what is actually the correct way to do it.
It just seemed to make more sense to me. Could you possibly show me the right way for these 2 examples and then I will close this out? If you have time.
And if I look at one of your examples from you article, aren't you doing a similar thing by doing this?
$email_address = trim($_POST["e"]);
$email_address = strtolower($email_address);
$safe_email_address = $mysqli->real_escape_string($email_address);
You have used $email_address multiple times so I am just trying to understand what is actually the correct way to do it.
You are needing to do a couple of things here
1. You need to make sure there is something in $_POST['email'] - it may or may not exists so you cannot use it on the assumption it does
2. If it does exist you need to check if it is valid.
You can do it in one line
After running either of the above $email will either have a valid email address or it will be false.
You can now test for validity by doing an simple if
When uploading files things are bit more complex because you have to
1. Check the file is valid
a) Size is within limit
b) Mime type is in set of acceptable types
2. Move file to permanent location
3. Potentially rename file using generated name
There could be issues at at either of the first 2 of those which could result in a failure.
A good practice is to have a class or function that handles image uploads that gives a status - which can then be used in the same if statement above.
1. You need to make sure there is something in $_POST['email'] - it may or may not exists so you cannot use it on the assumption it does
2. If it does exist you need to check if it is valid.
You can do it in one line
$email = isset($_POST['email']) ? filter_var($_POST['email'], FILTER_SANITIZE_EMAIL): false;
Which is the same as if (isset($_POST['email'])) {
$email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);
}
else {
$email = false;
}
I prefer the first because essentially what we are doing here is an assignment - which in simple situations is a one line operation. To have assignments spread over 6 lines - especially when you have to do a lot of these - can make for very lengthy and difficult to read code.After running either of the above $email will either have a valid email address or it will be false.
You can now test for validity by doing an simple if
if ($email) {
// email is present and valid so we can use it
}
This can be expanded to other post variables where you could end up withif ($post1 && $post2 && $post3) {
// valid input
}
else {
// at least one field was invalid so deal with it
}
When uploading files things are bit more complex because you have to
1. Check the file is valid
a) Size is within limit
b) Mime type is in set of acceptable types
2. Move file to permanent location
3. Potentially rename file using generated name
There could be issues at at either of the first 2 of those which could result in a failure.
A good practice is to have a class or function that handles image uploads that gives a status - which can then be used in the same if statement above.
ASKER
Thanks Julian,
So, is my whole approach incorrect? If I give you a simple example, I just want to know why it's wrong so I can understand. For simplicity's sake, lets say I just want to check that a name field is not empty. What I normally do is, check that is isn't empty and if it's not, I put it into a variable so that when it comes time to insert it into a database, it just seems less complicated to use it like that. For example:
So, I would never be able to execute the SQL unless $name had a value. Is this not a good enough way to determine if there is a value in $name?
But it sounds like Ray is saying that I should not do:
It just seems less messy when trying to insert into a database to use $name, $email, $whatever instead of
But if I am wrong then I am wrong and I need to correct it. But if I am going to correct it I need to understand it and nip it in the bud now.
So, is my whole approach incorrect? If I give you a simple example, I just want to know why it's wrong so I can understand. For simplicity's sake, lets say I just want to check that a name field is not empty. What I normally do is, check that is isn't empty and if it's not, I put it into a variable so that when it comes time to insert it into a database, it just seems less complicated to use it like that. For example:
if(empty($_POST['name'])) {
$message = "Please enter your name";
}
if(!$message) {
$name = $_POST['name'];
$stmt = $link->prepare("INSERT INTO `users` (`name`) VALUES (?)");
$stmt->bind_param("s", $name);
So, I would never be able to execute the SQL unless $name had a value. Is this not a good enough way to determine if there is a value in $name?
But it sounds like Ray is saying that I should not do:
$name = $_POST['name'].
It just seems less messy when trying to insert into a database to use $name, $email, $whatever instead of
$_POST['name'], $_POST['email'], $_POST['whatever']
But if I am wrong then I am wrong and I need to correct it. But if I am going to correct it I need to understand it and nip it in the bud now.
... seems less messy ...This is why we use frameworks and Active Record design patterns. The messy parts get abstracted away and we don't have to look at the validation code because it becomes part of the woodwork. And if it's all wrapped inside a function (aka class method) it can't pollute other scopes with unwanted variables. You can just write something like filter_post_input() and your custom filter_post_input() function will make all your troubles go away, leaving you with a sanitized data set and no side-effects in the global scope.
The notion of scope exists in all programming languages, and it's a little bit different in each of them. Here is PHP's take on scope.
http://php.net/manual/en/language.variables.scope.php
ASKER
Thank you both. I will just have to keep practicing and make an effort to get it right.
The pattern I use when processing forms is to name all my controls as an array
Validation can now be processed with a loop and a rules array
Now if $errors is non-empty I know validation failed - in addition the validate() function returns an array of error messages with the same index as the data in the $data array - so now I can do two things
1. If I am regenerating the form to display the form with errors - I simply use the data index to pull the current value and the message
2. If it is an ajax request I json_encode the array and send it back to the form.
Validate rules can be regular expressions passed in an array with error messages for each data field
Example:
<input type="text" name="data[firstname]" />
<input type="text" name="data[surname]" />
<input type="text" name="data[email]" />
That way when the data arrives I do this$data = isset($_POST['data']) ? $_POST['data'] : false;
Now if $data is false - then we did not arrive here by a form submit - otherwise I validate it.Validation can now be processed with a loop and a rules array
$errors = validate($data, $rules);
Now if $errors is non-empty I know validation failed - in addition the validate() function returns an array of error messages with the same index as the data in the $data array - so now I can do two things
1. If I am regenerating the form to display the form with errors - I simply use the data index to pull the current value and the message
2. If it is an ajax request I json_encode the array and send it back to the form.
Validate rules can be regular expressions passed in an array with error messages for each data field
Example:
$rules = array(
'firstname' => array('regx' => '/^[a-zA-Z]{3,50}$/', 'msg' => 'A firstname > 2 characters and < 50 is required'),
'surname' => array('regx' => '/.+/', 'msg' => 'A surname is required')
);
Validation would then be something like thisfunction validate($data, $rules)
{
$error = array();
foreach($rules as $id => $r) {
// If there is no element in the data array for this rule
// or the data element fails the RegEx then we have an error
if (!isset($data[$id]) ||!preg_match($r['regx'], $data[$id])) {
$error[$id] = $r['msg'];
}
}
return $error;
}
Have you tried using var_dump() to visualize $_POST and $_FILES?