What is wrong with my 'update' code for a CRUD system?

[jazza96]

Hi

 

I just finished this tutorial('http://www.startutorial.com/articles/view/php-crud-tutorial-part-3/)

and everything was working fine until I decided to add last names to the application. 

I got everything working on all the other pages except the Update page. 

This is my php code. 

 

d = $_REQUEST['id'];

    }

     

    if ( null==$id ) {

        header("Location: index.php");

    }

     

    if ( !empty($_POST)) {

        // keep track validation errors

        $nameError = null;

        $lastError = null;

        $emailError = null;

        $mobileError = null;

         

        // keep track post values

        $name = $_POST['name'];

        $last = $_POST['last'];

        $email = $_POST['email'];

        $mobile = $_POST['mobile'];

         

        // validate input

        $valid = true;

        if (empty($name)) {

            $nameError = 'Please enter Name';

            $valid = false;

        }

 

        $valid = true;

        if (empty($last)) {

        $lastError = 'Please enter last name';

        $valid = false;

        }

         

        if (empty($email)) {

            $emailError = 'Please enter Email Address';

            $valid = false;

        } else if ( !filter_var($email,FILTER_VALIDATE_EMAIL) ) {

            $emailError = 'Please enter a valid Email Address';

            $valid = false;

        }

         

        if (empty($mobile)) {

            $mobileError = 'Please enter Mobile Number';

            $valid = false;

        }

         

        // update data

        if ($valid) {

            $pdo = Database::connect();

            $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

            $sql = "UPDATE customers  set name = ?, last = ?, email = ?, mobile =? WHERE id = ?";

            $q = $pdo->prepare($sql);

            $q->execute(array($name, $last,$email,$mobile,$id));

            Database::disconnect();

            header("Location: index.php");

        }

    } else {

        $pdo = Database::connect();

        $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        $sql = "SELECT * FROM customers where id = ?";

        $q = $pdo->prepare($sql);

        $q->execute(array($id));

        $data = $q->fetch(PDO::FETCH_ASSOC);

        $name = $data['name'];

        $last = $data['last'];

        $email = $data['email'];

        $mobile = $data['mobile'];

        Database::disconnect();

    }

?>

 

When I hit Update it is directing me to a white screen instead of the index.php page. 

 

 

[maxxd]

Turn on error reporting - white screens are typically fatal errors in the php that aren't being displayed to screen. Put

error_reporting(-1);

ini_set('display_errors',true);

at the top of your file.

[mac_gyver]

the only things positive about the tutorial you found are - 1) it has separated the business logic on each page from the presentation logic (but it should not have separate pages for each different type of action), 2) it is using php's filter function to validate the email (but it should be using similar functions to validate the other form fields), and 3) it is using PDO as the database library (but it is not using it very well.)

 

the code is doing a bunch of things that are or can be problematic and should be avoided and not posted in a tutorial for others to repeat -

 

1) code that lets you alter data on the server should be secured so that only users with the proper permissions can change the data.

 

2) the code expects the id as a $_GET variable. it should only use $_GET['id'] to access the value. by using $_REQUEST, should the code where this is being used at ever add a $_POST['id'] or a $_COOKIE['id'] with a different meaning, these will overwrite the id that the code is expecting.

 

3) when there isn't a non empty id supplied to this code, it simply redirects back to the index.php page, without any indication of why. this will leave you wondering what the code did do. it is always best to display a specific error message for everything that your code detects that it didn't expect. the id is just another input value to the code on the page. it should be validated, with appropriate error message(s), the same as the $_POST data from the form.

 

4) the id is expected to be a positive integer. validating it and displaying an error message when it isn't an expected value will prevent nefarious use of your page (cross site scripting for example) and will help you when writing and debugging your code (if you have a coding error that didn't supply a proper id value.)

 

5) the header() redirects all need an exit; statement after them to prevent the remainder of the code after the header() statement from running. in the current code, if there isn't an id, but there is $_POST data, all that code will still run and attempt to update the row in the database, using a php null value as the row id (not sure, but that could throw query error/exception.)

 

6) the code should be using an array to hold the validation error messages. this will simplify the logic and would have prevented a problem that you introduced when you copy/pasted code to add the last name (you have added a line that set $valid = true;, that should have only been done once at the start of the code.) by using an array, such as $errors = array(); to hold the validation messages, you don't need to define individual variables for each error message and you don't need the $valid variable at all. you can just test if the array is empty or not to know if there are any validation errors.

 

7) in general, all external data should be trimmed before using it. the user may have accidentally entered white-space or non-printing characters before/after the value when typing it or copy/pasted it from somewhere and it could have white-space or non-printing characters before/after the value.

 

the code creating a database connection and setting the database error mode should not be repeated in multiple places in the code - DRY (Don't Repeat Yourself.) in general, you should not have any repetition of code in your code. also, for pdo, the default is to emulate prepared queries. you should add a setAttribute() statement to set emulated prepares to false - $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES,false); this is a prime example of using DRY when programming. you should only have to find and make this change in one place in your code. the setAttribute() statements should actually be in the database class, right after you make the connection.

 

9) after successfully processing the submitted form data, the page should redirect back to the exact same url that the form submitted to, in order to cause a get request for that page. this will prevent the browser from attempting to resubmit the form data should you refresh the page or browse back to that same url.

 

10) any exceptions the pdo connect method or prepares/queries throw should be caught and handled so as to not expose the details contained in the php/mysql error messages to the visitors.

 

11) the database class shouldn't die() and display any connection errors (see item #10 above.)

[jazza96]

the only things positive about the tutorial you found are - 1) it has separated the business logic on each page from the presentation logic (but it should not have separate pages for each different type of action), 2) it is using php's filter function to validate the email (but it should be using similar functions to validate the other form fields), and 3) it is using PDO as the database library (but it is not using it very well.)

 

the code is doing a bunch of things that are or can be problematic and should be avoided and not posted in a tutorial for others to repeat -

 

1) code that lets you alter data on the server should be secured so that only users with the proper permissions can change the data.

 

2) the code expects the id as a $_GET variable. it should only use $_GET['id'] to access the value. by using $_REQUEST, should the code where this is being used at ever add a $_POST['id'] or a $_COOKIE['id'] with a different meaning, these will overwrite the id that the code is expecting.

 

3) when there isn't a non empty id supplied to this code, it simply redirects back to the index.php page, without any indication of why. this will leave you wondering what the code did do. it is always best to display a specific error message for everything that your code detects that it didn't expect. the id is just another input value to the code on the page. it should be validated, with appropriate error message(s), the same as the $_POST data from the form.

 

4) the id is expected to be a positive integer. validating it and displaying an error message when it isn't an expected value will prevent nefarious use of your page (cross site scripting for example) and will help you when writing and debugging your code (if you have a coding error that didn't supply a proper id value.)

 

5) the header() redirects all need an exit; statement after them to prevent the remainder of the code after the header() statement from running. in the current code, if there isn't an id, but there is $_POST data, all that code will still run and attempt to update the row in the database, using a php null value as the row id (not sure, but that could throw query error/exception.)

 

6) the code should be using an array to hold the validation error messages. this will simplify the logic and would have prevented a problem that you introduced when you copy/pasted code to add the last name (you have added a line that set $valid = true;, that should have only been done once at the start of the code.) by using an array, such as $errors = array(); to hold the validation messages, you don't need to define individual variables for each error message and you don't need the $valid variable at all. you can just test if the array is empty or not to know if there are any validation errors.

 

7) in general, all external data should be trimmed before using it. the user may have accidentally entered white-space or non-printing characters before/after the value when typing it or copy/pasted it from somewhere and it could have white-space or non-printing characters before/after the value.

 

the code creating a database connection and setting the database error mode should not be repeated in multiple places in the code - DRY (Don't Repeat Yourself.) in general, you should not have any repetition of code in your code. also, for pdo, the default is to emulate prepared queries. you should add a setAttribute() statement to set emulated prepares to false - $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES,false); this is a prime example of using DRY when programming. you should only have to find and make this change in one place in your code. the setAttribute() statements should actually be in the database class, right after you make the connection.

 

9) after successfully processing the submitted form data, the page should redirect back to the exact same url that the form submitted to, in order to cause a get request for that page. this will prevent the browser from attempting to resubmit the form data should you refresh the page or browse back to that same url.

 

10) any exceptions the pdo connect method or prepares/queries throw should be caught and handled so as to not expose the details contained in the php/mysql error messages to the visitors.

 

11) the database class shouldn't die() and display any connection errors (see item #10 above.)

 

 

Thanks for those tips. I have started a new project where I am building a social networking site like facebook.