-1

I have been working on the admin side of my website now for a while.I have successfully created an add page to add items to data in database mysql.But when adding a delete.php page it does not work quite so well.When I select the one I no longer require and click delete button this msg come out and no error also when I check my site it does not delete the article.

enter image description here

this is my delete.php coding

<?php
include ("dbase.php");

//Use method $_Get to retrieve data from url
$id=$_GET['id'];

mysql_query("DELETE FROM logbook WHERE id=$id");

echo mysql_error();
header('Location:delete.php');

?>

Can someone see where I am going wrong?if you need any more info then please let me know. thanks.

Seth
  • 10,198
  • 10
  • 45
  • 68
tyra
  • 1
  • 1

3 Answers3

1

Currently you load delete.php, delete from the database, and then redirect to delete.php. When you reload delete.php, you delete from from the database, and then redirect to delete.php. When you reload delete.php, you get the point.

That is where the error "Redirect Loop" comes from; it will never end. Try commenting out your header() call or changing the location to another page like deleted.php.

Sam
  • 20,096
  • 2
  • 45
  • 71
  • You should read about SQL injection and how to avoid it. I'd be able to delete your whole table of data right now. – Sam Apr 16 '14 at 02:40
  • @tyra, to elaborate (since I was on mobile before), I am able to go to `delete.php?id=0 OR id IS NOT NULL`. With your current code, your SQL statement would evaluate to `DELETE FROM logbook WHERE id=0 OR id IS NOT NULL`; deleting your whole table. – Sam Apr 16 '14 at 15:04
1

You need to check to see if $_GET['id'] exists. Only then should you perform your query and then redirect.

if (!empty($_GET['id'])) {
    mysql_query("DELETE FROM logbook WHERE id=$id");
    header('Location:delete.php');
}

Once delete.php has no querystring parameter of id it won't reload and cause an infinite loop.

John Conde
  • 217,595
  • 99
  • 455
  • 496
  • Actually header makes no sense here, he doesn't need to redirect on delete.php for any reason whatsoever, its better to redirect to the source page instead. – Mr. Alien Apr 16 '14 at 02:31
  • How do you know it isn't the source page? You're making an assumption here. This page can list a bunch of items to be potentially deleted and also do the deleting. – John Conde Apr 16 '14 at 02:32
  • It is clear that he is using a separate page for a delete action, just like the beginner tutorials show, I can make that out because of the use of get var and the page name – Mr. Alien Apr 16 '14 at 02:33
  • Ummm see, he uses a separate page to delete the row, say just a link with the row id which points to delete.php, so if he has a separate page, and say the get is set for the first time with the row id, it will delete the row from the db, and will redirect back to the same page and that's where the page gets redirected to the white page, but wont cause an infinite loop because of the empty(), so am sure this is not on the same page – Mr. Alien Apr 16 '14 at 02:36
  • @tyra are you using a separate page just for deleting the row from the db – Mr. Alien Apr 16 '14 at 02:38
  • I don't see where it says he uses a separate page to delete the row. It just says he has a separate page for deleting. And really, this isn't even close to the biggest issue on the page. They're an amateur and this code is going to be basic at best. But I think this answer demonstrates why they have an error and how to fix it. If all they have to do is change where it redirects to then I think they can figure that out (especially after reading all of the comments for this answer). – John Conde Apr 16 '14 at 02:40
  • Yap if he is a super newbie, than comments should help, else we will get another question soon that why I'm getting a white page or how do I get back to source page after delete action – Mr. Alien Apr 16 '14 at 02:42
0

Your code has a logical and serious security problems. Use at least "mysqli", PDO, prepared statements. Read a bit and secure your web applications.

If that code is within the "delete.php" then you should redirect elsewhere or use some logic like below.

<?php
if($_GET['id'])
{
    include ("dbase.php");

    //Use method $_Get to retrieve data from url
    $id=addslashes($_GET['id']);

    mysql_query("DELETE FROM logbook WHERE id=".mysql_real_escape_string($id).")" or die(mysql_error());

    header('Location: delete.php');
}
?>
GTodorov
  • 1,993
  • 21
  • 24
  • addslashes()? Won't make anything better... his code is still vulnerable to sql injections – Mr. Alien Apr 16 '14 at 02:39
  • @Mr. Alien - it is vulnerable, but at least it won't crash with an error, if someone decides to enter single or double quote. And that's why you have "mysql_real_escape_string". The user can also use the build in PHP filter() function to sanitize or validate. – GTodorov Apr 16 '14 at 03:03