0

I have found several threads on stackOverflow that address how to add a 'delete' button to PHP results tables, but none seem to address my specific use case.

I have a PHP results table pulled from a MySql database. I have added a 'delete' link to each row of the table, but when I click the 'delete' link on a given row it deletes all rows in the table.

I suspect that I am implementing the 'delete' link improperly, but I am not adept enough with PHP to figure out what exactly is wrong.

Below is the code that generates the table. Note the 'delete' link is echoed in the endChildren() function.

        <?php
        echo "<table style='border: solid 1px black;'>";
        echo "<tr><th>Id</th><th>Name</th><th>Number</th><th>Part A</th><th>Part B</th><th>Full Name</th><th>Address</th><th>Apt.</th><th>City</th><th>State</th><th>Zip</th><th>Remove</th></tr>";

        class TableRows extends RecursiveIteratorIterator { 
            function __construct($it) { 
                parent::__construct($it, self::LEAVES_ONLY); 
            }

            function current() {
                return "<td>" . parent::current(). "</td>";
            }

            function beginChildren() { 
                echo "<tr>"; 
            } 

            function endChildren() { 
                echo "<td><a href='delete.php?id=".$row['id']."'>Delete></a></td>";
                echo "</tr>" . "\n";
            } 
        } 


        $servername = "localhost:3306";
        $username = "xxxxxxxxxx";
        $password = "xxxxxxxx";
        $dbname = "xxxxxxxxx";

        try {
            $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $conn->prepare("SELECT id, Name, Number, PartA, PartB, Full_Name, Address, Apt, City, State, Zip FROM Medicard"); 
            $stmt->execute();

            // set the resulting array to associative
            $result = $stmt->setFetchMode(PDO::FETCH_ASSOC); 

            foreach(new TableRows(new RecursiveArrayIterator($stmt->fetchAll())) as $k=>$v) { 
                echo $v;
            }

        }
        catch(PDOException $e) {
            echo "Error: " . $e->getMessage();
        }
        $conn = null;
        echo "</table>";
        ?> 

Here is the delete.php script:

    <?php
    $servername = "localhost:3306";
    $username = "xxxxxxxx";
    $password = "xxxxxxxx";
    $dbname = "xxxxxxxx";

    try {
        $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
        // set the PDO error mode to exception
        $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        // sql to delete a record
        $sql = "DELETE FROM Medicard WHERE id=id";

        // use exec() because no results are returned
        $conn->exec($sql);
        echo "Record deleted successfully";
        }
    catch(PDOException $e)
        {
        echo $sql . "<br>" . $e->getMessage();
        }

    $conn = null;
    ?>            

When I click on the delete link in a given row it deletes all rows in the table. I am not sure if this is because of where I added the 'delete' link, or if my delete.php code is incorrect.

I am relatively new to PHP and MySql. Any guidance would be appreciated. Thanks.

5150 Design
  • 179
  • 14
  • Purely based on the core you posted - your ID is not being passed as a variable. Assuming your id=id is actually a valid variable which you have not posted then, I would test by hardcoding the $id value = 3 or whatever number and see if the delete script works. If you confirm it works, inspect the html dom element and see what your actual URL is. Lastly - I hope you are using this for learning and not in any production environment as there are security risks you run into by exposing the ID number to the client. – alpharomeo Jul 03 '19 at 16:46
  • @alpharomeo why would exposing an ID to a client be a security risk? The whole point of an ID is to uniquely identify a piece of data to those who need to know (such as, in this case, people who might want to delete it) but without implying any other meaning or telling you anything about what the data is. The ID is generally about the only field from the data which you _can_ share publicly without compromising the security of the data. For that reason I don't understand your statement at all. Can you explain? – ADyson Jul 03 '19 at 16:51
  • 1
    Exposing the ID is not a security risk if implemented correctly. You have to make sure the user can only delete those row he is allowed to delete, but hiding identifiers is not necessary. See [here](https://stackoverflow.com/questions/396164/exposing-database-ids-security-risk) for a related discussion – Kryptur Jul 03 '19 at 16:51

1 Answers1

3

You delete all rows where the column id matches the column id - which is of course the case for all rows.

$sql = "DELETE FROM Medicard WHERE id=id";

You need to read the ID from the GET parameter and adjust your query:

  <?php
    $servername = "localhost:3306";
    $username = "xxxxxxxx";
    $password = "xxxxxxxx";
    $dbname = "xxxxxxxx";

    $id = $_GET["id"];

    try {
        $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
        // set the PDO error mode to exception
        $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

        // sql to delete a record
        $sql = $conn->prepare("DELETE FROM Medicard WHERE id = ?");
        $success = $sql->execute(array($id));

        echo $success ? "Record deleted successfully" : "Record not deleted";
        }
    catch(PDOException $e)
        {
        echo $e->getMessage();
        }

    $conn = null;
    ?>    

The table generation part (I replaced the iterator you implemented by a simple loop)

<?php
        echo "<table style='border: solid 1px black;'>";
        echo "<tr><th>Id</th><th>Name</th><th>Number</th><th>Part A</th><th>Part B</th><th>Full Name</th><th>Address</th><th>Apt.</th><th>City</th><th>State</th><th>Zip</th><th>Remove</th></tr>";

        $servername = "localhost:3306";
        $username = "xxxxxxxxxx";
        $password = "xxxxxxxx";
        $dbname = "xxxxxxxxx";

        try {
            $conn = new PDO("mysql:host=$servername;dbname=$dbname", $username, $password);
            $conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $stmt = $conn->prepare("SELECT id, Name, Number, PartA, PartB, Full_Name, Address, Apt, City, State, Zip FROM Medicard"); 
            $stmt->execute();

            // set the resulting array to associative
            $result = $stmt->setFetchMode(PDO::FETCH_ASSOC);

            foreach ($stmt->fetchAll() as $row) {
                echo "<tr>"; 
                foreach ($row as $val) {
                    echo "<td>" . $val . "</td>";
                }
                echo "<td><a href='delete.php?id=".$row['id']."'>Delete></a></td>";
                echo "</tr>";
            } 

        }
        catch(PDOException $e) {
            echo "Error: " . $e->getMessage();
        }
        $conn = null;
        echo "</table>";
?> 

Kryptur
  • 745
  • 6
  • 17
  • Thank you for your quick response. I tried the above code and received an error (DELETE FROM Medicard WHERE id=. SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1). – 5150 Design Jul 03 '19 at 16:52
  • "Make sure to use escape_string to prevent SQL injection!" Actually there are edge cases where that still doesn't provide security. The correct solution (and has been for years and years) is to use prepared statements and parameterised queries. A better answer would actually a) use the correct solution and b) show an example of it rather than just mentioning it in passing. Change that and you can have my upvote, since you diagnosed the issue correctly, but just proposed a poor quality solution. – ADyson Jul 03 '19 at 16:53
  • I tried moving the " outside of the $id and removing the spaces in between, but still drawing a syntax error? – 5150 Design Jul 03 '19 at 16:54
  • 1
    I changed the code to utilize a prepared statement. Are you sure you also added the part `$id = $_GET["id"]`?. – Kryptur Jul 03 '19 at 16:58
  • @Kryptur Thank you for adding the prepared statements (that was above and beyond). Unfortunately, now I am getting a success message, but the records are not being deleted, even after refresh. I copied the code exactly as you provided. – 5150 Design Jul 03 '19 at 17:06
  • 1
    What does `echo $id` give you? You can also try to get more information about errors with `print_r($sql->errorInfo());` after the statement is executed. – Kryptur Jul 03 '19 at 17:14
  • With print_r($sql->errorInfo()); after the statement is executed I get this: Array ( [0] => 00000 [1] => [2] => ) Record deleted successfully – 5150 Design Jul 03 '19 at 17:21
  • 1
    Ah, in your `endChildren` function, you access `$row` which is not defined - thus the ID is empty. The link of your delete button seems to point to `delete.php?id=` – Kryptur Jul 03 '19 at 17:25
  • So, I am adding the 'delete' link improperly? – 5150 Design Jul 03 '19 at 17:32
  • 1
    Yes, I updated the answer again to include that part – Kryptur Jul 03 '19 at 17:50
  • @Kryptur I appreciate all of your effort. Unfortunately, now I have a syntax error related to this line of code: echo "" . $val . ";. Syntax error Unexpected ';'. – 5150 Design Jul 03 '19 at 18:01
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/195947/discussion-between-kryptur-and-5150-design). – Kryptur Jul 03 '19 at 18:17
  • @Kryptor My mistake. I made an error of my own. Your last code sample works perfectly! Thank you very much for your help! – 5150 Design Jul 03 '19 at 18:24