0

So i have a notification page created using bootstrap alerts. here's a snippet, each notification is an echo from a row of the database

<div class="container" >

<?php 
$fetch = $conn->query("SELECT * FROM notifications"); 

while($row = $fetch->fetch_assoc()) {
  $id = ''.$row["id"].'';
  $notification = ''.$row["notifications"].'';

?>

<div class="alert alert-warning alert-dismissable ">
  <a href="#" class="close" data-dismiss="alert" aria-label="close" id="close">
    <form action='deletenotifications.php' method='post' id='myform' >
        <input type='hidden' name='id' id='id' value="<?php echo $id ?>" />
        <span type="submit">&times;</span>
    </form></a>
  <?php echo $notification; ?>
</div>


<?php } ?> 

</div>

The user when pressing X needs to delete the notification, thus from the database too, so a hidden form containing the id of the alert to be sent to action deletenotication.php using jquery AJAX method

<script type="text/javascript">
 $('#close').click(function(){
   $.post( 
     $('#myform').attr('action'),
     $('#myform :input').serializeArray(),
   );
});
</script>

and here is the deletenotification.php snippet

$id = $_POST['id']; 


$sqlf = $conn->query("SELECT * from notifications");

while($rown = $sqlf->fetch_assoc()){
    $idbase = ''.$rown['id'].'';

    if($id == $idbase){
        $sql = $conn->query("DELETE FROM notifications WHERE id=$id"); 

    }

}

it is deleting from the database but only if the alerts are closed in order, and only one alert is deleted, in order to delete the successive one, the page need to be refreshed.

closing the alerts 2, 3 and 4 wont delete the rows unless notification 1 is deleted,

I need if the user close ANY random alert to be deleted, and not in order

Thank you!

SamHecquet
  • 1,818
  • 4
  • 19
  • 26
  • 1
    **WARNING**: When using `mysqli` you should be using [parameterized queries](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and [`bind_param`](http://php.net/manual/en/mysqli-stmt.bind-param.php) to add user data to your query. **DO NOT** use string interpolation or concatenation to accomplish this because you have created a severe [SQL injection bug](http://bobby-tables.com/). **NEVER** put `$_POST`, `$_GET` or **any** user data directly into a query, it can be very harmful if someone seeks to exploit your mistake. – tadman Nov 06 '17 at 19:24
  • It seems odd to spin through every single notification in your system and then, when you find a match, go and delete that matched record. Why not just execute the `DELETE FROM` and be done with it? You could easily have millions of notification records. This code will get slower and slower over time, to an absurd degree. – tadman Nov 06 '17 at 19:24
  • i have tried that method at first, but it didn't seem to work either, so i tried the while loop – Rodi Chamii Nov 06 '17 at 19:30
  • If it "didn't work" you need to find a reason why. A lot of problems can be detected and resolved by [enabling exceptions in `mysqli`](https://stackoverflow.com/questions/14578243/turning-query-errors-to-exceptions-in-mysqli) so mistakes aren't easily ignored. – tadman Nov 06 '17 at 20:22

1 Answers1

1

You don't need to iterate through all of the results just to delete a specific row by its ID. That costs you two queries instead of one, and the first one could potentially be costly if there are a ton of notifications in the database. Instead, you can just delete by the ID, like so.

if ( ! empty( $_POST['id'] ) && (int) $_POST['id'] ) {
    $id  = (int) $_POST['id']; 
    $conn->query("DELETE FROM notifications WHERE id=$id");
}

In the example above, I am casting the ID to an integer, for safety. But really, you should look at the database class you're using there, and instead use a prepared statement.


Aside: I noticed a couple of lines like this one with two single quotes. You don't need all of those. They are only helpful if you're concatenating. Or, if you were you defining a string literal.

$id = ''.$row["id"].'';

Instead, just assign it the value of the ID.

$id = $row['id'];

It's also a good idea to use and verify a CSRF token before responding to a request to delete a row from the database; i.e., make sure this request is a legitimate one. Maybe you're already doing this elsewhere, but I wanted to caution you in case.

See: https://en.wikipedia.org/wiki/Cross-site_request_forgery

jaswrks
  • 1,255
  • 10
  • 13