-1

I am trying to make a deactivate account so ...When I click a link I want the account status to be updated in the database so it will turn 0. Every time I click the link here nothings happens it just re direct me to another page this my code for the deactivating

<?php
    include "../includes/dbcon.php";
    if(isset($_GET['user_id']))
    {
        $result = mysql_query("SELECT user_id FROM users WHERE user_id = $user_id");

        while($row = mysql_fetch_array($result))
        {
            echo $result;
            $status = $row($_GET['status']);

            if($status == 1)
            {
                $status = 0;
                $update = mysql_query("Update users set status = $status");
                header("location: admin_manage_account.php");
            }
            else
            {
                echo "Already deactivated";
            }
        }
    }
?>
Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
Christian
  • 259
  • 3
  • 7
  • 21
  • 2
    `Update users set status = $status` is missing the `WHERE` clause and is trying to update ALL your rows. You should also use `mysql_affected_rows()` for *truthness*. Check for errors. – Funk Forty Niner Jan 16 '16 at 13:47
  • I'm not able to understand this line in your code `$status = $row($_GET['status']);` What you tiring to assign to your `$status` – Saty Jan 16 '16 at 13:47
  • 2
    Check your database. You will find that _All_ your users have been deactivated because your `WHERE` clause does not specify which user to change `status` for. You must review [How can I prevent SQL injection in PHP](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php). Your code is vulnerable such that if the called URL was `http://example.com/deactivate.php?user_id=0+OR+1%3D1%27`, all your users would be instantly deactivated even if you did fix the `WHERE` clause. – Michael Berkowski Jan 16 '16 at 13:49
  • 1
    as @Saty stated, this `$row($_GET['status'])` should most likely be `$row['status']` – Funk Forty Niner Jan 16 '16 at 13:49
  • @Fred-ii- I was writing a looong comment and you got in ahead again... – Michael Berkowski Jan 16 '16 at 13:49
  • 2
    @MichaelBerkowski I'm on my 2nd cup of coffee ;-) *Speedy Gonzales!* Arribah!! – Funk Forty Niner Jan 16 '16 at 13:50
  • I'll let you guys have a go on this. Too many things wrong with this code. – Funk Forty Niner Jan 16 '16 at 13:51
  • 1
    *My usual:* Add error reporting to the top of your file(s) right after your opening PHP tag for example ` – Funk Forty Niner Jan 16 '16 at 13:55
  • and sure enough, seems like my comments are serving "some" purpose but has tripped a bit. – Funk Forty Niner Jan 16 '16 at 13:56

3 Answers3

0

I don't know what is your problem exactly, but why do you select the id based on the id?

Why don't you do something like "UPDATE users SET status = $status WHERE user_id = $user_id" in the first place?

In your example you don't even have a condition in the update statement...

If you want to "toggle/flip" a value you can just do something like:

UPDATE users SET status = NOT status WHERE user_id = $user_id

This way, true become false, false become true, etc.

lucian.nicolaescu
  • 647
  • 3
  • 9
0

You code is not ok on many reasons. But the most serious problem is SQL Injection attack!

If attacker put non-expected value to your user_id param, your sql like that "SELECT user_id FROM users WHERE user_id = $user_id" and that "Update users set status = $status WHEREuser_id= '$user_id'" can cause very serious problems.

For example: user_id can be: "0; DROP TABLE users"

Be careful with your code, rewrite it

Dzianis Yafimau
  • 2,034
  • 1
  • 27
  • 38
-1

Too many things needed to be improved. You should not use MySQL either. Consider MySQLi or PDO. BTW here is an updated version of your own code which should work:

<?php

include "../includes/dbcon.php";



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

$user_id = $_GET['user_id']; // I assume you want to capture it from URL

$result = mysql_query("SELECT user_id FROM users WHERE user_id = $user_id");

while($row = mysql_fetch_array($result))
{

   // echo $result; why do you echo it? No need
   /* $status = $row($_GET['status']); should be: */
   $status = $row['status'];
    if($status  == 1)
    {
        $status = 0;

        $update = mysql_query("Update users set status = $status WHERE `user_id` = '$user_id'");
                  header("location: admin_manage_account.php");
    }
    else
    {
        echo "Already deactivated";
    }

    }

}
?>

Previously, you were not defining $user_id. Moreover, it wasn't correct the way you were getting the status ($status) of the user.

Rehmat
  • 4,681
  • 3
  • 22
  • 38
  • which "should" work. Care to "explain" it? Or have I done that already? Ah yes, I have. – Funk Forty Niner Jan 16 '16 at 13:57
  • 1
    You should NOT do a query just to get the previous value. You can do something like "UPDATE users SET status = NOT status WHERE user_id = $user_id". Also, your example is not containing the condition again!!! – lucian.nicolaescu Jan 16 '16 at 13:58
  • @lucian.nicolaescu If OP loves to retrieve the status before updating the database then its his choice. We aren't here to rewrite entire code. We can just fix what's programmatically incorrect. BTW what kind of condition you are talking about? – Rehmat Jan 16 '16 at 14:02
  • 1
    *"We aren't here to rewrite entire code."* - Well you should (somewhat), because what you have now, will update ALL their rows. – Funk Forty Niner Jan 16 '16 at 14:04
  • This one "Update users set status = $status" you update all the users, not just the "selected" user. – lucian.nicolaescu Jan 16 '16 at 14:04
  • @Fred-ii- Sorry for this ;) – Rehmat Jan 16 '16 at 14:05