0

What I am trying to achieve

I am trying to update my database with values filled out in a form by the user.

I've spent a few months now with endless Google searches avoiding asking a question to StackOverflow, and every site has taught me a bit but I havent been able to finish this product.

The issue I am struggling with

On submission, the PDO::execute does not execute and database is not updated. Previously I was receiving errors of "Undefined index" but those errors has been solved by passing the variables correctly. Now I am not getting any errors.

My question

What am I doing wrong? Considering I am not getting any errors in console or anywhere (and error_reporting is on). The if statement where I check if it has been executed, always gives nope so somewhere in the query it's failing.

Also how can I debug this better?

What I've tried (code's been minified to reveal relevant content)

index.php (displaying table with an Update button redirecting me to update.php)

<?php
session_start();

require 'assets/php/database.php';
require 'assets/php/usersession.php';

?>
<!DOCTYPE html>
<html>
<head>
<!-- irrelevant content -->
</head>
<body>
    <?php
    $sql = "SELECT * FROM utlansliste";

    $stmt = $conn->prepare($sql);
    $stmt->execute();
    $result = $stmt->fetchAll();
    ?>

    <table>
        <thead>
            <th>ID</th>
            <th>Brukernavn</th>
            <th>Datamaskin</th>
            <th>Periferiutstyr</th>
            <th>Start dato</th>
            <th>Slutt dato</th>
            <th>Oppdater tabell</th>
        </thead>

        <?php
        foreach($result as $rows) {
        ?>

        <tr>
            <td><?php echo $rows['id']; ?></td>
            <td><?php echo $rows['username']; ?></td>
            <td><?php echo $rows['hostname']; ?></td>
            <td><?php echo $rows['peripherals']; ?></td>
            <td><?php echo $rows['start_date']; ?></td>
            <td><?php echo $rows['end_date']; ?></td>
            <td><a href="update.php?id=<?php echo $rows['id'];?>&hostname=<?php echo $rows['hostname'];?>"><div class="btn btn-primary">Oppdater</div></a></td>
        </tr>

        <?php
        }
        ?>

    </table>
</body>
</html>

update.php (where the user fills out the form and where the execution should happen)

<?php

session_start();

require 'assets/php/database.php';
require 'assets/php/usersession.php';

if(isset($_GET['id'])) {

    $id = $_GET['id'];
    $hostname = $_GET['hostname'];

    global $conn;
    $sql = "SELECT * FROM utlansliste WHERE id='$id';";
    $stmt = $conn->prepare($sql);
    $stmt->execute();
    $row = $stmt->fetchAll();

    $username = $row[0]['username'];
    $peripherals = $row[0]['peripherals'];
    $start_date = $row[0]['start_date'];
    $end_date = $row[0]['end_date'];

    if(isset($_POST['submit'])) {

        try {

            global $conn;
            $sql = "UPDATE utlansliste SET username = ':username', peripherals = ':peripherals', start_date = ':start_date', end_date = ':end_date', WHERE id = ':id'";

            $stmt = $conn->prepare($sql);
            $stmt->bindParam(":username", $username, PDO::PARAM_STR);
            $stmt->bindParam(":peripherals", $peripherals, PDO::PARAM_STR);
            $stmt->bindParam(":start_date", $start_date, PDO::PARAM_STR);
            $stmt->bindParam(":end_date", $end_date, PDO::PARAM_STR);
            $stmt->bindParam(":id", $id, PDO::PARAM_STR);
            $stmt->execute();

            if ($stmt->execute()) { 
               echo "gg";
            } else {
               echo "nope";
            }

            /*header('Location:index.php');*/
            }

            catch(PDOException $exception)  {
            echo "Error: " . $exception->getMessage();
        }
    }
}
?>
<html>
<head>
<!-- irrelevant content -->
</head>
<body>
    <?php include 'assets/php/header.php' ?>
    <?php if( !empty($user) ): ?>
    <div class="content">
        <strong>Oppdater utlånsliste for <?php echo $hostname; ?></strong>
        <form name="form" method="POST" action="">
            <div class="updatebox" style="text-align:center;">
                <label for="username">Brukernavn</label>
                <div><input type="text" name="username" value="<?php echo $username;?>" id="username" required/></div>

                <label for="peripherals">Periferiutstyr</label>
                <div><input type="text" name="peripherals" value="<?php echo $peripherals;?>" id="peripherals"/></div>

                <label for="startdate">Låne fra - dato</label>
                <div><input data-date-format="YYYY/MM/DD" type="date" name="start_date" value="<?php echo $start_date;?>" id="start_date" required/></div>

                <label for="enddate">Låne til - dato</label>
                <div><input data-date-format="YYYY/MM/DD" type="date" name="end_date" value="<?php echo $end_date;?>" id="end_date" required/></div>

                <input name="id" type="hidden" id="id" value="<?php echo $id;?>"/>
                <input name="hostname" type="hidden" value="<?php echo $hostname;?>" id="hostname"/>
                <input type="submit" name="submit" value="Submit"/>
            </div>
        </form> 
    </div>
<body>
</html>

Additional comments

I've looked into many sites looking for a help and knowledge. These are a few of many more. I am mainly looking for knowledge to learn this secure method of updating a database, so even just a comment helps a ton!

PHPDelusions

Edit form with PHP PDO

Form in PDO to update data

The official PDO manual

The unofficial PDO manual, (PDODelusions)

Sanguinary
  • 354
  • 2
  • 3
  • 16
  • 2
    lose the quotes around the placeholders `username = ':username', peripherals = ':peripherals',` ~ try instead `username = :username, peripherals = :peripherals,` etc and only one `$stmt->execute()` – Professor Abronsius Jan 22 '19 at 11:12
  • @RamRaider Did not solve it. And what do you mean by only one `$stmt->execute();` ? I only got one, the other is just a `if` statement checking on the query. – Sanguinary Jan 22 '19 at 11:14
  • the 2nd `$stmt->execute` is still an execute statement – Professor Abronsius Jan 22 '19 at 11:16
  • @RamRaider I commented it away, however still not working. What would you suggest to be a better solution to debug `PDO::execute()` then? – Sanguinary Jan 22 '19 at 11:18
  • 1
    You linked PHPDelusions and more specifically the update part of that guide and it shows how [named parameters](https://phpdelusions.net/pdo_examples/update#named) are not quoted, I know @RamRaider mentioned this already but the reason I'm bringing it up again is that although you've listed a bunch of resources that you've looked through, how many have you actually *read*, properly? – Script47 Jan 22 '19 at 11:20
  • 1
    As for the error reporting, the last link in your resource list will lead you to [this](https://phpdelusions.net/pdo#dsn) which will teach you to set the property so PDO throws exceptions. – Script47 Jan 22 '19 at 11:22
  • In regards to specifying the data type in your `bindParam`, if you omit it, it will default to `PDO::PARAM_STR` anyway so no need to do that. – Script47 Jan 22 '19 at 11:26
  • As for checking if the insert was successful: https://stackoverflow.com/questions/1661863/pdo-mysql-how-to-know-if-insert-was-successful - **TL;DR:** Assign the return value of `$pdo->execute()` to a variable and check that variable's value instead of calling execute twice which in turn fires the query twice. – Script47 Jan 22 '19 at 11:27
  • @Script47 Thanks for commenting, I did not have the parameters quoted previously. I found a solved answer which was the reason I chose to quote them whih seemed strange. The `error_reporting` in my connection file is already set up as the [reference](https://phpdelusions.net/pdo#dsn) you linked to explained. – Sanguinary Jan 22 '19 at 11:31
  • 1
    @Sanguinary can you please link me that answer? That way I can leave a note on the answer to try and prevent the dissemination of misinformation. – Script47 Jan 22 '19 at 11:33
  • @Script47 If I find it again I will definitely link it. – Sanguinary Jan 22 '19 at 11:42

1 Answers1

2

The sql was incorrect before - aside from the single quotes around the placeholders there was a comma before the where clause.

$sql = "UPDATE utlansliste SET 
            username = :username, 
            peripherals = :peripherals, 
            start_date = :start_date, 
            end_date = :end_date 
        WHERE id = :id";

Can you confirm that the update statement actually gets called? Try printing the sql before/after the execute method is called to ensure the program gets to that point

Gone through the PHP quickly and made a few little changes which MIGHT ( or might not ) help.

<?php

    try{
        session_start();
        $id='';

        require 'assets/php/database.php';
        require 'assets/php/usersession.php';

        if( isset( $_GET['id'], $_GET['hostname'] ) ) {

            $id = $_GET['id'];
            $hostname = $_GET['hostname'];


            /*
                global $conn;

                The `global` keyword is used within functions to allow a variable declared outside the function
                to be used within the function... think `scope`
            */


            /*
            $sql = "SELECT * FROM utlansliste WHERE id='$id';";
            As the problem revolves around prepared statements why not use a prepared statement here
            and avoid the possibility of sql injection??
            */

            $sql='select * from `utlansliste` where id=:id;';
            $args=array( ':id' => $id );
            $stmt = $conn->prepare($sql);
            $res=$stmt->execute( $args );

            if( !$res )throw new Exception(' Failed to SELECT records ');



            $row = $stmt->fetchAll();

            $username = $row[0]['username'];
            $peripherals = $row[0]['peripherals'];
            $start_date = $row[0]['start_date'];
            $end_date = $row[0]['end_date'];

            /* make sure that all variables are available... */
            if( isset( $_POST['submit'], $username,$peripherals,$start_date,$end_date ) ) {

                try {
                    /* same issue, global is NOT required */
                    #global $conn;

                    $sql = "UPDATE utlansliste SET username = :username, peripherals = :peripherals, start_date = :start_date, end_date = :end_date WHERE id = :id";

                    $stmt = $conn->prepare( $sql );
                    $stmt->bindParam(":username", $username, PDO::PARAM_STR);
                    $stmt->bindParam(":peripherals", $peripherals, PDO::PARAM_STR);
                    $stmt->bindParam(":start_date", $start_date, PDO::PARAM_STR);
                    $stmt->bindParam(":end_date", $end_date, PDO::PARAM_STR);
                    $stmt->bindParam(":id", $id, PDO::PARAM_STR);

                    $result = $stmt->execute();

                    if ( $result ) { 
                       echo "gg";
                    } else {
                       echo "nope";
                    }

                    /*header('Location:index.php');*/
                }catch(PDOException $exception)  {
                    echo "Error: " . $exception->getMessage();
                }
            }
        }


    }catch( Exception $e ){
        exit( $e->getMessage() );
    }

?>

Incidentally it is not neccessay to use a prepared statement in the following because there is no user supplied variables and no possibility of sql injection

$sql = "SELECT * FROM utlansliste";

$stmt = $conn->prepare($sql);
$stmt->execute();
$result = $stmt->fetchAll();

If there was a where clause then it might be different... maybe

update To aid debugging PDO queries I use the following function debugpdo ~ an example is given as to it's usage.

function debugpdo( $sql=false, $args=array() ){
    if( $sql && !empty( $args ) ){
        $params = array();
        $keys = array();
        foreach( $args as $placeholder => $value ){
            if( is_numeric( $value ) )$params[]=sprintf('set @%s=%d;',str_replace( ':', '', $placeholder ), $value );
            else $params[]=sprintf('set @%s="%s";',str_replace( ':', '', $placeholder), str_replace( '"',"'", $value ) );
            $keys[]=str_replace(':','@',$placeholder);
        }
        printf( 
            "<pre><h1>Copy & Paste this SQL into mySQL GUI Application</h1>%s\n\n%s;</pre>",
            implode( PHP_EOL, $params ),
            str_replace( array_keys( $args ), $keys, $sql )
        );
    }
}


$sql = "update `utlansliste` set `username`=:username, `peripherals`=:peripherals, `start_date`=:start_date, `end_date`=:end_date where `id`=:id";
$args = array(
    ':username'     =>  $username,
    ':peripherals'  =>  $peripherals,
    ':start_date'   =>  $start_date,
    ':end_date'     =>  $end_date,
    ':id'           =>  $id
);


/* To debug the sql, uncomment this and run... */
exit( debugpdo( $sql, $args ) );

/* code continues... */
$stmt = $conn->prepare( $sql );
$result = $stmt->execute( $args );
Professor Abronsius
  • 33,063
  • 5
  • 32
  • 46
  • It is as if it's ignores the other columns and only returns the `id`. – Sanguinary Jan 22 '19 at 11:56
  • Printing `$row` works fine and all values are displayed. The issue must then be at the SQL query itself. It's strange that it doesn't update the database at all. – Sanguinary Jan 22 '19 at 12:31
  • Anywho, I'm really grateful for the help you've provided with me. This is more secure than the one I wrote and I will continue to use this from now. I also appreciate the comments where you didnt just remove my code, but commented it out and explained why. Helps alot for my learning process. Thanks. I will most likely accept this as the answer if I find the solution later on. – Sanguinary Jan 22 '19 at 12:37
  • `It is as if it's ignores the other columns and only returns the id` ? Do you refer to the `select * from ...` query? – Professor Abronsius Jan 22 '19 at 13:18
  • Nope. I `echo $result;` after the PDO execution and it returns the `$id` only. – Sanguinary Jan 22 '19 at 13:32
  • ah! `$result` will return a boolean ( 1 / 0 ) I think - so if you see `1` then that's good... – Professor Abronsius Jan 22 '19 at 13:38
  • That was my first thought, but was unsure. Thanks for clarifying! It's strange that it returns 1 but the SQL query wasn't executed. Something doesn't add up, but no errors are confirming it. :s – Sanguinary Jan 22 '19 at 13:40
  • Wow, really nice efficient debugging tool! I like it. By using it I'm pretty sure I found out that the form is not submitted correctly. It doesn't submit the values I've inputted, can you double check that my HTML form is submitting the values correctly? – Sanguinary Jan 22 '19 at 13:48
  • looking closer at the code I can't help but wonder what the point of the `select * from `utlansliste` where id=` is? If that is simply to test whether the user or whatever the item is ( referred to by id ) actually exists then the next part should be to take the POST variables and use those rather than the existing values from the db???? – Professor Abronsius Jan 22 '19 at 15:40
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/187133/discussion-between-ramraider-and-sanguinary). – Professor Abronsius Jan 22 '19 at 16:07
  • pastebin.com/raw/itDjT1xB – Professor Abronsius Jan 22 '19 at 19:36
  • Pastebin worked like a charm! That's amazing. Thank you so much and I really appreciate the comments you left explaining your process in-depth. There is a lot of functionalities you've used I need to read myself more up on before I move on. Once again, thank you for your huge effort, patience and the knowledge you fed my brain with! – Sanguinary Jan 23 '19 at 09:49