PHP exercise to read or write database values

Question

I just came up with the code below but I'm wondering if there is room for improvement? I.e. better readability and or performance, better structure, less lines of code?

The script should parse a simple HTTP GET or POST request. The request can contain the following parameters:

• action (required, possible values are "read" and "write");
• mykey (required)
• id (only required when action equals "read")

Depending on a read or write request the script should print an array containing:

• status;
• id (only after writing / inserting a value into database);
• value (in case of a read request);

Sample requests:

• api.php?action=write&mykey=12345&value=im%20fine
• api.php?action=read&mykey=12345&id=13

The script should also log the content of the request to file. (append)

Code:

<?php
 include("opendb.php");
 $logging = true;
 $log = serialize($_REQUEST) . PHP_EOL;
 $reply = array("id" => null, "status" => null);
 if(isset($_REQUEST["action"]) && isset($_REQUEST["mykey"])) {
 $action = strtolower($_REQUEST["action"]);
 $mykey = $mysqli->real_escape_string($_REQUEST["mykey"]);
 if($action == "read") {
 if(isset($_REQUEST["id"]) && is_numeric($_REQUEST["id"])) {
 $id = $_REQUEST["id"];
 // read message from database
 $sql = "SELECT * FROM data WHERE mykey = '$mykey' AND id = $id";
 if ($result = $mysqli->query($sql)) {
 $row = $result->fetch_assoc();
 $reply["id"] = $id;
 $reply["value"] = $row["value"];
 $reply["status"] = "success";
 } else {
 $log .= $mysqli->error . PHP_EOL;
 $reply["status"] = "error";
 }
 } else {
 $log .= "id missing" . PHP_EOL;
 $reply["status"] = "error (incorrect id)";
 }
 } elseif($action == "write") {
 // write message to database
 $value = $mysqli->real_escape_string($_REQUEST["value"]);
 $sql = "INSERT INTO `data` (`value`, `mykey`) VALUES ('$value', '$mykey')";
 if ($result = $mysqli->query($sql)) {
 $reply["id"] = $mysqli->insert_id;
 $reply["status"] = "success";
 } else {
 $log .= $mysqli->error . PHP_EOL;
 $reply["status"] = "error";
 }
 }
 } else {
 $log .= "invalid querystring" . PHP_EOL;
 $reply["status"] = "error (incorrect action, mykey)";
 }
 print_r($reply);
 if($logging) {
 file_put_contents('log/read_write_exercise.log', date('d-m-Y') . " " . print_r($log, TRUE), FILE_APPEND | LOCK_EX) ;
 }
?>

Answer 1

• Instead of using a big stack of if/elseif, since you're just checking the value of a single variable you should be using a switch/case instead.
• What if action and mykey are both set, but action is neither read nor write - Your user will get a blank response.. yet another reason to use a switch (with a default option).
• Don't use real_escape_string, use prepared statements instead.
• Don't SELECT * ... if you only need the value column, just select value ...
• I would suggest creating a function to handle errors.
• I would suggest holding your errors in an array and joining them rather than concatting to a string - this will be more efficient and manageable.
• There is no reason to use LOCK_EX in conjunction with FILE_APPEND - If you lock the file and the script is called simultaneously one instance will be blocked or denied while the other is writing. See here for more info.
• Is this not supposed to be an API? If it is you might consider outputting JSON instead of print_ring the results. I would suggest putting this in an output function as well.

---

include("opendb.php");
$logging = true;
$log = array(date('d-m-Y'));
$log[] = serialize($_REQUEST)
$reply = array("id" => null, "status" => 'success');
if(!isset($_REQUEST["mykey"])) error("No key provided.");
$action = empty($_REQUEST["action"]) ? "" : strtolower($_REQUEST["action"])
switch($action){
 case "read":
 if(!isset($_REQUEST["id"])) error('No ID provided.');
 $stmt = $mysqli->prepare("SELECT value FROM data WHERE mykey = ? AND id = ?");
 $stmt->bind_param('si', $_REQUEST["mykey"], $_REQUEST["id"]);
 $stmt->execute();
 $stmt->bind_result($value);
 if(!$stmt->fetch()) error("No results found.");
 $reply["id"] = $_REQUEST["id"];
 $reply["value"] = $value;
 output();
 case "write":
 if(!isset($_REQUEST["value"])) error('No value provided.');
 $stmt = $mysqli->prepare(INSERT INTO `data` (`value`, `mykey`) VALUES (?, ?));
 $stmt->bind_param('ss', $_REQUEST["value"], $_REQUEST["mykey"]);
 if(!$stmt->execute()) error($mysqli->error);
 $reply["id"] = $mysqli->insert_id;
 output();
 default: error('Invalid action.');
}
function error($msg){
 $GLOBALS['log'][] = $msg;
 $GLOBALS['reply']['status'] = 'error';
 $GLOBALS['reply']['error'] = $msg;
 output();
}
function output(){
 if($GLOBALS['logging']){
 file_put_contents('log/read_write_exercise.log', implode(PHP_EOL, $GLOBALS['log']), FILE_APPEND) ;
 }
 header('Content-Type: application/json');
 echo json_encode($GLOBALS['reply'], JSON_PRETTY_PRINT);
 exit;
}

• Your Common Sense: Nothing personal but this is a texbook example of the worst use case for the global variables. It's barely aceptable to have read-only globals (to access global config etc) but making them writable is a support nightmare.
• I wrestled a bear once.: you are absolutely right but I'm not going to refactor his entire code into a class as the "correct" way to do this requires a hell of a lot more code rather than less code..
• john81: First of all thanks for your replies. @I restled a bear once: thanks for shring your implementation / correction, this is really appreciated. About the superglobals discussion: I rarely use the GLOBALS array to globally store variables but if I'm correct it's bad practise to use this array to write to, I suppose this is because of debugging and testing? Do you have any references to articles that discuss this topic about globals and best practice?
• I wrestled a bear once.: @john81 - First, you should know that Common is a bit of a drama queen, I believe that's why he's currently banned on Stackoverflow. Most of the time it's just easier to upvote his comment and agree than it is to try and argue. As far as superglobals go, if you aren't familiar with the code and you need to debug the $reply array and you do a search for it in the page and you can't figure out what is changing it's value it can be super frustrating. However, given that this is a super short script and the two functions using it are readily apparent, it's not really a problem in this case.
• I wrestled a bear once.: @john81 - truth is, if it was really that bad, the GLOBALS array wouldn't exist in the first place.