0

I have an a href which looks like that: <a href="delete-news.php?deleteID=11">Delete</a>

And file delete-news.php is as follow:

<?php 

if(isset($_GET["?deleteID='.$id."])) 
{

    $result = mysql_query("DELETE FROM 'news' WHERE id='$id'");
    echo mysql_error();
    if($result)
        echo "succces";
}
else { echo "GET NOT SET"; }

?>

But it is returning GET NOT SET. What I'm doing wrong?

USer
  • 13
  • 1
  • 2
  • @wiero - I have changes that but still `GET NOT SET` – USer Aug 12 '11 at 14:13
  • Do **NOT EVER** use a get request to do a delete operation. One webspider on your site and ALL of your data is gone: http://thedailywtf.com/Articles/The_Spider_of_Doom.aspx – Marc B Aug 12 '11 at 14:51

7 Answers7

5

Use this, and for god's sake escape your inputs.

if(isset($_GET['deleteID'])) {
    $result = mysql_query("DELETE FROM `news` WHERE id='".mysql_real_escape_string($_GET['deleteID']). "'");
    echo mysql_error();
    if($result)
        echo "succces";

} else {
    echo 'GET NOT SET';
}
Dunhamzzz
  • 14,682
  • 4
  • 50
  • 74
1

$_GET will have each element of the GET variables already broken down, so no need to include the URL data. So, in your example, the link ?deleteID=123 would produce $_GET['deleteID'].

Try using that, but also remember to sanitize the values you receive in from URLs. If it's going to be a numeric value, I suggest casting it:

$deleteID = (int)$_GET['deleteID'];
Brad Christie
  • 100,477
  • 16
  • 156
  • 200
  • It's still better to escape as you insert it into the query, what if OP overwrites the value of $id, or copies elsewhere and changes to a non-int variable? – Dunhamzzz Aug 12 '11 at 14:20
  • What if, what if, what if. There will always be exceptions, I can't prevent a [PICNIC](http://en.wikipedia.org/wiki/User_error). – Brad Christie Aug 12 '11 at 14:40
  • Well you could cater for all those by escaping in the query. – Dunhamzzz Aug 12 '11 at 14:43
1

Please also note that changes to the system should only happen via POST, and never GET. Otherwise (for example), you might get a spidering bot that deletes your whole site. See this post for more references:

https://stackoverflow.com/questions/679013/get-vs-post-best-practices

Community
  • 1
  • 1
Alex Howansky
  • 50,515
  • 8
  • 78
  • 98
0
<?php 

if(isset($_GET["deleteID"])) 
{
    $id = ($_GET['deleteID']);
    $result = mysql_query("DELETE FROM news WHERE id='".mysql_real_escape_string($id)."'");
    echo mysql_error();
    if($result)
        echo "succces";
}
else { echo "GET NOT SET"; }

?>

is correct one

genesis
  • 50,477
  • 20
  • 96
  • 125
  • This will return `You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''news' WHERE id='11'' at line 1` – USer Aug 12 '11 at 14:15
  • You're not supposed to wrap table names in `'`, use backticks instead (see my response below for more information). :-) – karllindmark Aug 12 '11 at 14:19
  • It's still better to escape as you insert it into the query, what if OP overwrites the value of $id, or copies elsewhere and changes to a non-int variable? – Dunhamzzz Aug 12 '11 at 14:20
  • @genesis that's actually worse – Dunhamzzz Aug 12 '11 at 14:23
  • @Dunhamzzz: 'I have an a href which looks like that: Delete' << op's post. But edited – genesis Aug 12 '11 at 14:24
  • that's irrelevant, I'm talking about `mysql_real_escape_strinng()`'ing `$id` as you put it into the query, just incase it's not a number. – Dunhamzzz Aug 12 '11 at 14:26
0

You need to check $_GET for just deleteID. Later, reference it as $_GET['deleteID']. Also, call mysql_real_escape_string() on $_GET['deleteID'] to retrieve your query parameter $id.

if(isset($_GET["deleteID"])) 
{
    $id = mysql_real_escape_string($_GET['deleteID']);
    $result = mysql_query("DELETE FROM `news` WHERE id='$id'");
    echo mysql_error();
    if($result)
        echo "succces";
}
else { echo "GET NOT SET"; }
Michael Berkowski
  • 267,341
  • 46
  • 444
  • 390
  • this will return `You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''news' WHERE id='11'' at line 1` – USer Aug 12 '11 at 14:18
  • @USer yes, single quotes on `news` converted to backquotes. See above – Michael Berkowski Aug 12 '11 at 14:18
0

Try this instead:

<?php 
    if(isset($_GET['deleteID'])) 
    {
        $id = intval($_GET['deleteID']);
        $result = mysql_query("DELETE FROM `news` WHERE id='$id'");
        echo mysql_error();
        if($result) echo "succces";

    } else { 

        echo "GET NOT SET";

     }
?>

Note that I'm making the given deleteID into an int, meaning that values other than some form of number will become 0.

Also, you can't wrap a table- and/or column name with ' - backticks are the way to go!

karllindmark
  • 6,031
  • 1
  • 26
  • 41
  • It's still better to escape as you insert it into the query, what if OP overwrites the value of `$id`, or copies elsewhere and changes to a non-int variable? – Dunhamzzz Aug 12 '11 at 14:19
  • You may want to ensure that `$id>0` before executing the sql statement. – Shoan Aug 12 '11 at 14:21
  • Well, yes, that's true. However, I assumed that OP actually knows what he's doing when he's doing it. :) Regarding the `id>0`: there could actually be a row that's got a negative value, but the query itself won't *fail* if no row is found. – karllindmark Aug 12 '11 at 14:22
0

You obtain GET NO SET, because the $_GET associative array does not contain ?deleteID='.$id.

In order for you to obtain the id, you need to so something like this:

$id = $_GET['deleteID'];

Also

$result = mysql_query("DELETE FROM 'news' WHERE id='$id'");

That is very unsafe as it allows SQL injections. Instead, do:

$query = sprintf("DELETE * FROM news WHERE id=%d",
         mysql_real_escape_string($id),
$result = mysql_query($query);

I hope this helped.

tsbelanger
  • 1
  • 1