Add To Basket Script

[chibineku]

As mentioned in another thread, I'm writing a script to let users place items in their baskets. It is very complex so far, and I would like to talk it through with someone to see if they can suggest better design or tidy the current design up. Here is the code I have, which doesn't work very well (i.e. has errors that I haven't yet resolved), with comments to show my intentions:<?phpsession_start();include_once("db_include.php5");doDB();if(!$_GET["productid"] || !$_GET["qty"]) {//the user has entered the address directly into their address bar, send them away (if=1 to let me know where the script branched) header("Location:index.php5?if=1"); exit();}//do select query to verify item id is valid, in case they entered data into the query string or the item has been removed from db$check_sql = "SELECT * FROM aromaProducts1 WHERE id='".$_GET["productid"]."'";$check_res = mysqli_query($mysqli, $check_sql) or die(mysqli_error($mysqli));if(mysqli_num_rows($check_res) == 0) {//item doesn't exist, redirect user header("Location:index.php5?if=2"); exit();} else if(mysqli_num_rows($check_res) != 0) {//item exists //do select query to check for item id already in basket - if this item is already in the table associated with the user's session id (which will be added every time an item is), then we want to change the quantity only $duplicate_sql = "SELECT qty FROM sessionBasket WHERE product_id='".$_GET["productid"]."' AND usersessid='".session_id()."'"; $duplicate_res = mysqli_query($mysqli, $duplicate_sql) or die(mysqli_error($mysqli)); if(mysqli_num_rows($duplicate_res) != 0) { //item in basket - add another - fetch current quantity and add new quantity $basketInfo = mysqli_fetch_array($duplicate_res); $currQty = $basket_info["qty"]; $add_sql = "UPDATE sessionBasket SET qty='".($_GET["qty"]+$currQty)."' WHERE usersessid='".session_id()."'AND product_id='".$_GET["productid"]."'"; $add_res = mysqli_query($mysqli, $add_sql) or die(mysqli_error($mysqli)); if($add_res !== TRUE) { //wasn't updated for some reason - this is where my script currently breaks header("Location:basketfailredirect.php5?error=add"); exit(); } else if($add_res === TRUE) { //was updated - send them away header("basket.php5?res=add"); exit(); }} else if(mysqli_num_rows($duplicate_res) == 0) { //no existing items in basket, so we want to add the current item info associated with the user's id/session id $info = mysqli_fetch_array($check_res);//fetch product id, passed in query string from the product info page $productid = $_GET["productid"]; //sanitize possible inputs, if set - notes is a field added to the product info page for custom products, and we want to sanitize it if it's set $notes = isset($_GET["notes"])?trim(mysqli_real_escape_string(check_chars_mailto($_GET["notes"]))):"";//if the user is logged in, their userid is stored in the session variable $userid = $_SESSION["userid"]?$_SESSION["userid"]:"";//not sure about the keep alive option - i.e. store basket contents even if the user doesnt register/sign in, but keeping the option there $alive = $_SESSION["alive"]?$_SESSION["alive"]:"no"; //insert query $insert_sql = "INSERT INTO sessionBasket (userid, usersessid, date_added, keep_alive, product_id, qty, notes) VALUES ( '".$userid."', '".$_SESSION["PHPSESSID"]."', now(), '".$alive."', '".$productid."', '".$_GET["qty"]."', '".htmlspecialchars($notes)."')"; $insert_res = mysqli_query($mysqli, $insert_sql) or die(mysqli_error($mysqli)); if($insert_res === TRUE) { //success header("Location:basket.php5?res=add"); exit(); } else if($insert_res !== TRUE) { //fail header("Location:basketfailredirect.php5?error=add2"); exit(); } }}?>It's pretty complicated for me - I want to allow empty fields, add the userid if it's available (which is missing from my UPDATE query)... Is this a million miles from a good design, or what? Edit: also, when I try to add items to the basket, I now get an error: internal server error 500. I suspect this is due to bad coding because my search results and product view pages work and they use the same server and the same database as this script.Edit 2: I changed $_SESSION["SESSID"] for session_id() which works better. I can now add items to the basket, but if I add an item and then add the same item, the update qty branch doesn't work - I get the error 500 again. If the user is signed in, their userid gets added correctly and adding new items is fine - so it's 90% there.

[chibineku]

Oh, and uh *bump*

[chibineku]

Hm, okay, now it updates. I had put ' round the number to set the qty as, typecasting it as a string even though I had called it an integer. Now it updates, but I still get a server error, which is strange. But it's progress!It now looks like:

<?phpsession_start();include_once("db_include.php5");doDB();if(!$_GET["productid"] || !$_GET["qty"]) {  header("Location:index.php5");  exit();}//do select query to verify item id$check_sql = "SELECT * FROM aromaProducts1 WHERE id='".$_GET["productid"]."'";$check_res = mysqli_query($mysqli, $check_sql) or die(mysqli_error($mysqli));if(mysqli_num_rows($check_res) == 0) {  header("Location:index.php5?if=2");  exit();} else if(mysqli_num_rows($check_res) != 0) {  //do select query to check for item id already in basket  $duplicate_sql = "SELECT qty FROM sessionBasket WHERE product_id='".$_GET["productid"]."' AND usersessid='".session_id()."'";  $duplicate_res = mysqli_query($mysqli, $duplicate_sql) or die(mysqli_error($mysqli));    if(mysqli_num_rows($duplicate_res) != 0) {	//item in basket - add another	$add_sql = "UPDATE sessionBasket SET qty=qty+".(integer)$_GET["qty"]." WHERE usersessid='".session_id()."'AND product_id='".$_GET["productid"]."'";	$add_res = mysqli_query($mysqli, $add_sql) or die(mysqli_error($mysqli));		if($add_res !== TRUE) {	  //wasn't updated	  header("Location:basketfailredirect.php5?error=add");	  exit();	} else if($add_res === TRUE) {	  //was updated - send them away	  header("basket.php5?res=add");	  exit();	}} else if(mysqli_num_rows($duplicate_res) == 0) {	//no existing items in basket  $productid = $_GET["productid"];    //sanitize possible inputs, if set  $notes = isset($_GET["notes"])?trim(mysqli_real_escape_string(check_chars_mailto($_GET["notes"]))):"";  $userid = $_SESSION["userid"]?$_SESSION["userid"]:"";  $alive = $_SESSION["alive"]?$_SESSION["alive"]:"no";  //insert query  $insert_sql = "INSERT INTO sessionBasket (userid, usersessid, date_added, keep_alive, product_id, qty, notes) VALUES (  '".$userid."',  '".session_id()."',  now(),  '".$alive."',  '".$productid."',  '".$_GET["qty"]."',  '".$notes."')";  $insert_res = mysqli_query($mysqli, $insert_sql) or die(mysqli_error($mysqli));  if($insert_res === TRUE) {	//success	header("Location:basket.php5?res=add");	exit();  } else if($insert_res !== TRUE) {	//fail	header("Location:basketfailredirect.php5?error=add2");	exit();  }  }}?>

[justsomeguy]

First, make sure you always develop with maximum error reporting:error_reporting(E_ALL);ini_set('html_errors', 1);ini_set('display_errors', 1);Instead of doing this:if(!$_GET["productid"] || !$_GET["qty"]) {you're going to get a notice if those aren't set, so use empty to check instead:if (empty($_GET['productid']) || empty($_GET['qty']))that won't give an error if they aren't set.Second, never use anything from $_GET, $_POST, or $_COOKIE in a SQL statement without sanitizing the data first. That's the #1 way that websites get hacked. If the value is a number value, use intval or floatval to convert it. If it's a string value, use mysql_real_escape_string to escape it. Either way, don't trust the raw data. If you want to learn more about that, look up SQL injection attacks.Since you have the script exiting if a query has an error, you don't need to check for true or false. With this, for example:

$add_res = mysqli_query($mysqli, $add_sql) or die(mysqli_error($mysqli));if($add_res !== TRUE) {

$add_res will never be false, because if it was the script would have already quit and printed the error. For what it's worth, I use an error log instead of printing the error message. Users don't know what the errors mean, and I don't want them seeing database details. So I catch the errors by checking the return values and printing a generic error message for the user, and I send the database error to an error log that I can check later.As for 500 errors, that doesn't give you any information. That's just the browser telling you that an error happened. IE has an option which shows "friendly" errors instead of giving the actual error message from the server. Either disable that option in IE to get it to show the actual error message, or use a different browser to test.

[chibineku]

Ah, for some reason I forgot that people can fudge input even if it's supposed to come from a select element. I will sanitize.I also hadn't thought about the error thing. The problem with the error logs is that I'm using a remote browser and don't know how to check the logs there. I understand that I can use any file I specify, but I don't know how yet. As for the error 500, I am not testing on IE, I'm using FF, so I should be getting more explanatory error messages, right? I mean, usually it tells me very good messages regarding PHP and MySQL syntax.It is strange that it's breaking after making the query successfully.

[justsomeguy]

This is what I use to write to an error log:error_reporting(E_ALL);ini_set('error_log', dirname(__FILE__) . DIRECTORY_SEPARATOR . 'error.log');ini_set('html_errors', 0);ini_set('log_errors', 1);ini_set('display_errors', 0);That uses a file called error.log in the same directory as the PHP script. You may need to give it permissions to write to that file. I usually download the log over FTP, you could also just point your browser to it though. If you have a script that has an error but you aren't able to get it to print or log the errors, try setting the error options first and then including the problem file:

<?phperror_reporting(E_ALL);ini_set('error_log', dirname(__FILE__) . DIRECTORY_SEPARATOR . 'error.log');ini_set('html_errors', 0);ini_set('log_errors', 1);ini_set('display_errors', 0);include 'file.php';?>

[chibineku]

Ah, noted - so, instead of having or die(mysqli_error($mysqli)), it all just filters directly into that file? And then, what, just send location headers on event of failure? Have you any idea why my script might break after the UPDATE query? I will go and do the error logging thing, but just in case you have any ideas that are faster.

[justsomeguy]

Since mysql_query won't report an error automatically, can use error_log to send the error to whatever log was set up. e.g.:

$add_res = mysqli_query($mysqli, $add_sql);if (!$add_res){  error_log(mysqli_error($mysqli));  // print message or redirect}

I'm not sure what's going on with the query, but print it out to see what it looks like.

[chibineku]

Ha, I get the time and date in the error log file, but no details.

[justsomeguy]

What do you see if you put this after the query:var_dump($add_res);or whatever variable the return is in.

[chibineku]

bool(true)

[justsomeguy]

It's true, but it's failing a test like this:if (!$add_res)?

[chibineku]

Seems that way. Ah, now an error:PHP Warning: Cannot modify header information - headers already sent by (output started at /homepages/6/d203988372/htdocs/sinaesthesia/do_additem.php5:38) in /homepages/6/d203988372/htdocs/sinaesthesia/do_additem.php5 on line 51That's here: if(!$add_res) { //wasn't updated error_log(mysqli_error($mysqli)); header("Location:basketfailredirect.php5?error=add"); exit(); } else if($add_res) { error_log(mysqli_error($mysqli)); //was updated - send them away header("aromatherapy.php5?res=add"); exit(); }Apparently writing to the error log counts as sending headers? I moved the var_dump line to the else if($add_res) branch and it showed up, so the test is working right, but it's complaining about the headers when I try to send it away to the line in bold.

[justsomeguy]

Using error log doesn't send output, but using var_dump does. For this case:} else if($add_res) {error_log(mysqli_error($mysqli));If $add_res is true, there's not going to be an error, that's probably where you're writing a blank message to the error log.

[chibineku]

Well, even without the error_log line or var_dump, it still breaks, but as always makes the query successfully.

[justsomeguy]

Well, then you need to figure out where it's breaking. Add error_log lines farther and farther down the page until you don't see them showing up in the error log any more. Then you can at least narrow down which line is causing it to fail. You can use this to just print the line number:error_log(__LINE__);Are you sure the error isn't happening after the redirect?

[chibineku]

*sigh* I'm too embarassed to tell you what the problem was.

[chibineku]

hm, now that that's all working, and I've added a query that updates the userid field each time, in case the person has since signed in, I have another little problem of expectation: if you have items in a basket, and you leave a site, then come back, you don't really expect the items to still be there, right? But is it nice if they are? I could write a query that finds items associated with the current userid but not the same (i.d. previous) session ids. It would be fairly complex and annoying to write. I'm not sure if it's worth doing. It depends what I do when the user is ready to buy - do I move everything for their order from the sessionBasket table to an orderTracker table so that I can safely remove all records in the sessionBasket table that are past a certain date without worrying that I'm deleting a record of an order? I reckon that sounds like the best idea. There will also be paper copies of orders, or at least a digital copy of the paperwork each customer will recieve in the mail with the order, so we can archive those. But what if you sign in, add items to the basket, sign out, close the page, come back and sign in...you expect basket items to still be there. Egads it's complicated!

[justsomeguy]

But what if you sign in, add items to the basket, sign out, close the page, come back and sign in...you expect basket items to still be there.

I don't think that's the expectation, I think if someone logs out and closes the page they don't expect their session to be saved. Most people don't know the technical details, they just assume that if they log out and close the page they'll have to put everything they wanted in their cart the next time they log in.