What's the issue with this PHP code?

General PHP Help
#1

Hi!
Somewhere in this code there is an error (I know this because it causes a HTTP 500 error on the page)
I have ran the code through a php syntax checker and it didn’t flag any issues. Please could someone help?
Many thanks!

if($_GET) {
 if($_GET["claim"] == "try") {
  if($usernamedata["weeklyclaim"] == "true") {
   header("Location: https://emailpass.tk/dashboard/buy-credits?claim=error");
  } else {
   $credits = int($usernamedata["credits"]);
   $newcredits = ($credits + 1);
   $sql = 'UPDATE `accounts` SET `credits`="'. $newcredits .'" WHERE `username`="'. 
   $_SESSION["loggedin"] .'";';
   $result = $conn->query($sql);
   $usernamedata = $result->fetch_assoc();
   header("Location: https://emailpass.tk/dashboard/buy-credits?claim=success");
  }
 } else {
 }
}

(thanks for the edit, I forgot about bb code)

#2

Have a look into the error log of your webserver. then comment out block by block until the error is gone.

1 Like
#3

After checking the log, as you said, I fixed the issue. It turns out you can’t have a Boolean in the table when using fetch_assoc(). Thanks for the help!

1 Like
#4

The error you got about a boolean was because there’s no result set for an UPDATE query. There’s nothing to fetch.

What’s the issue with this PHP code?

Let me count the ways -

  1. Use a post method form when updating, inserting, or deleting data (anything that causes an action on the server.)
  2. Put the form and form processing code on the same page, i.e. don’t redirect all over the place. This will simplify the code and provide a better user experience.
  3. Every header() redirect needs an exit; statement after it to stop program execution.
  4. If you are executing an update query just to modify a number column, don’t SELECT the value first, just modify the column directly in the UPDATE query, i.e. SET credits = credits + 1 …
  5. Data related to a user should be stored using the user’s id, not the username, for a few reasons. 1) It will use least amount of storage space, 2) It will result in the fastest queries, and 3) It will allow the username to be edited without breaking the relationship between the data.
  6. When keeping track of real, virtual, or credit/token based money, you should INSERT a row for every transaction that affects a user’s account value. This will give you an audit trail, so that if a programming mistake, duplicate form submission, or nefarious activity alters a value, you can correct the amount. By just UPDATEing a value in a column, you will never know if or what caused an incorrect amount. You would use SUM(amount) in a query to get the current total for the rows for a user.
  7. Use a prepared query when supplying dynamic values to an sql query statement. For the current query, which you will be changing to use the user id, if someone registered a username that contained sql, that sql would get injected into the query. I’m assuming you have seen this sql joke - https://xkcd.com/327/
  8. The semi-colon ; on the end of an sql query statement is not used when executing the query through php.
  9. You don’t have any apparent error handling for the database statements that can fail - connection, query, prepare, and execute. You should use exceptions for database statement errors and in most cases let php catch and handle the exception, where it will use its error related settings to control what happens with the actual error information.
1 Like
#5

Thank you so much for the detailed breakdown of the issues in my code, I will try my best to improve my code on all the pages of my website and also try to prevent SQL injection. I am new to PHP & SQL so responses like this mean a lot! Thanks :slight_smile:

Sponsor our Newsletter | Privacy Policy | Terms of Service