znaji Posted January 24, 2016 Share Posted January 24, 2016 Hi Would like to rewrite this patch of MySQL update query better can someone please help. $sqlStatment = "UPDATE products SET " . "catid=" . $_REQUEST['cat'] . ", cattxt='" . $catText . "', ptitle='" . $_REQUEST['ptitle'] . "', saleoffer='" . $_REQUEST['saleoffer'] . "', url='" . $_REQUEST['url'] . "', tag=" . $_REQUEST['tag'] . "', oldprice=" . $_REQUEST['oprice'] . ", nprice=" . $_REQUEST['nprice'] . ", availability=" . $_REQUEST['availability'] . ", shortdesc = :shortdescription" . ", pdescription = :description" . ", meta_title =" . $_REQUEST['meta_title'] . ", meta_keyword =" . $_REQUEST['meta_keyword'] . "', meta_description=" . $_REQUEST['meta_description'] . ", last_modified = NOW()" . " WHERE id=" . $_REQUEST['pid']; try{ $query = $db->prepare($sqlStatment); $query->execute(array(':shortdescription'=>$_REQUEST['shortdescription'], ':description'=>$_REQUEST['description'])); } catch(PDOException $e) { die($e->getMessage()); } Thank you for any help. Quote Link to comment Share on other sites More sharing options...
Barand Posted January 24, 2016 Share Posted January 24, 2016 Use placeholders for all the variables and not just two of them. Quote Link to comment Share on other sites More sharing options...
maxxd Posted January 25, 2016 Share Posted January 25, 2016 (edited) Barand is right - use your prepared statement correctly. Right now you're wide open to SQL injection attacks and your single quotes are irregular and incorrect in several spots. A prepared statement will take care of those issues for you. Also, I'd recommend against using $_REQUEST. You should know where your data is coming from, and look only there for it - $_GET, $_POST, or $_SESSION. $_REQUEST is a catchall superglobal and can open you up to more security issues. Edited January 25, 2016 by maxxd 1 Quote Link to comment Share on other sites More sharing options...
ginerjm Posted January 25, 2016 Share Posted January 25, 2016 (edited) I agree strongly with Maxxd on the use of $_REQUEST. You KNOW whether it is a post or get so USE that array name. I also agree with the comments on using a fully prepared query, ie, use parms for all of the query's values, not just for a couple of them. This would also help alleviate the injection possibility that you are creating by using un-sanitized input values. Lastly you could read up on how to use single and double quotes and braces for your query string. Consider this: sqlStatment = "UPDATE products SET catid={$_REQUEST['cat']}, cattxt='$catText', ptitle='{$_REQUEST['ptitle']}', saleoffer='{$_REQUEST['saleoffer']}', ... ... WHERE id='{$_REQUEST['pid']}'"; A much simpler statement to construct and to read. Of course when you SWITCH to a properly prepared statement it will be even easier: sqlStatment = "UPDATE products SET catid=:cat, cattxt=:cattext, ptitle=:ptitle, saleoffer=:saleoffer:' ... ... WHERE id=:pid"; Edited January 25, 2016 by ginerjm Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.