REST API PHP find by id

Question

Description: The web server retrieves data from file database (pdf, doc, 3d file, zip and rar) and financial program such as inventory, price, indexes, order status and so on. Communication diagram

I'm doing REST API PHP for the first time, I've never used this API with PHP, I am an absolute beginner to REST.

Code returns data to cooperate with a www site.

First, I will test for a product that exists.

Output code 200:get_id2.php?id=39

id "39"
caption "product01"
filename "http://localhost/photo_gallery/public/files/images/image01.jpg"
description "testing Api"

Next, I will test for a product that does not exist.

Output code 404:get_id2.php?id=99999

message "No product."

I need your opinions.

1. Are there any code smells (can it be done better)?
2. Better to use prepared statement or not?
3. Use htmlspecialchars and urlencode for output?
4. What is good practice for REST API (PHP)?

Thank you in advance for your opinion.

File: get_id2.php

header("Access-Control-Allow-Origin: *");
header("Access-Control-Allow-Headers: access");
header("Access-Control-Allow-Methods: GET");
header("Access-Control-Allow-Credentials: true");
header("Content-Type: application/json; charset=UTF-8");
 include_once('config_setup.php');
 //$id = $_GET['id'];
 $id = isset($_GET['id']) ? $_GET['id'] : die();
 $sql = "SELECT * ";
 $sql .= "FROM photographs ";
 $sql .= "WHERE id='" . db_escape($db, $id) . "'";
 $result = mysqli_query($db, $sql);
 confirm_db_connect($result);
 $url = 'http://localhost/photo_gallery/public/files/images/';
 $allowed_tags = '<div><img><h1><h2><p><br><strong><em><ul><li><table><td><tr><th><tbody>';
 $photo = mysqli_fetch_assoc($result);
 $id = $photo['id'];
 $caption = $photo['caption'];
 $filename = $photo['filename'];
 $description2 = $photo['description2'];
 if ($photo['caption'] != null) {
 // create array
 $product_arr = array(
 "id" => u(h($id)),
 "caption" => h($caption),
 "filename" => $url . h($filename),
 "description2" => strip_tags($description2, $allowed_tags)
 );
 // set response code - 200 OK
 http_response_code(200);
 echo json_encode( $product_arr );
 } else {
 // set response code - 404 Not found
 http_response_code(404);
 // tell the user product does not exist
 echo json_encode(array("message" => "No product."));
 }
 mysqli_free_result($result);
 db_disconnect($db);

Functions

function h($string="") {
 return htmlspecialchars($string);
}
function u($string="") {
 return urlencode($string);
}
function confirm_result_set($result_set) {
 if (!$result_set) {
 exit("Database query failed.");
 }
 }
function db_escape($connection, $string) {
 return mysqli_real_escape_string($connection, $string);
 }
function db_disconnect($connection) {
 if(isset($connection)) {
 mysqli_close($connection);
 }
 }

Answer 1

• get_id2.php is expected to produce a json string, so make sure that it always does that.
• Don't use die() because then you are not providing an informative json response.
• Don't be too informative when the expected id is missing or invald -- be clear about the problem, but also vague.
• Validate the incoming id using isset() then ctype_digit(), if it passes those checks, then your script may advance to the querying step.
• I prefer object-oriented mysqli syntax since it is less verbose, but you can keep using procedural if you like.
• Calling confirm_db_connect($result); after executing the query doesn't make any sense. If you are going to bother checking the connection, you should be doing that before trying to execute the query.
• You only need four columns in your result set, so I advise that you list those columns explicitly in your SELECT clause so that you don't ask for more than you need.

• I don't see any value in declaring single use variables from the result set.

  $id = $photo['id'];
  $caption = $photo['caption'];
  $filename = $photo['filename'];
  $description2 = $photo['description2'];
  

  Just use the $photo[...] variables directly when sanitizing/preparing the data.

• Those single character sanitizing/preparing functions are a bad idea. Developers that need to read your script in the future (including yourself) will have no chances of instantly understanding what these calls do. This will require manually jumping to another file to look up what action is being executed. Use more meaningful function names and don't try to save characters at the cost of losing comprehensibility. In fact, it only hurts your code to write a custom function that replaces a single native function call -- just use the native call and your script and developers will be happier.

• As of PHP7.4, strip_tags() allows an alternative declaration of allowed tags. So your second parameter could look like this:

  ['div', 'img', 'h1', 'h2', 'p', 'br', 'strong', 'em', 'ul', 'li', 'table', 'td', 'tr', 'th', 'tbody']
  

  Admittedly, when using proper spacing between values, the expression ends up being longer; but it can be sensibly broken into multiple lines to reduce overall width.

• I prefer to write the negative response before the positive responses in my projects for consistency. This just means moving the No product. outcome earlier in the code.
• It is not necessary to manually free the result nor close the db connection when this script resolves; php will do this clean up for you automatically.
• write json_encode() just once and pass a variable to it -- for the sake of DRY-ness.

My untested suggestions:

header("Access-Control-Allow-Origin: *");
header("Access-Control-Allow-Headers: access");
header("Access-Control-Allow-Methods: GET");
header("Access-Control-Allow-Credentials: true");
header("Content-Type: application/json; charset=UTF-8");
if (!isset($_GET['id']) || !ctype_digit($_GET['id'])) {
 $response = ['message' => 'Missing/Invalid identifier provided'];
} else { 
 include_once('config_setup.php');
 $sql = "SELECT id, caption, filename, description2 FROM photographs WHERE id = " . $_GET['id'];
 $result = mysqli_query($db, $sql);
 if (!$result) {
 $response = ['message' => 'Please contact the dev team.'];
 } else {
 $photo = mysqli_fetch_assoc($result);
 if (!$photo) {
 $response = ['message' => 'No product.'];
 } else {
 $url = 'http://localhost/photo_gallery/public/files/images/';
 $allowed = '<div><img><h1><h2><p><br><strong><em><ul><li><table><td><tr><th><tbody>';
 $response = [
 'id' => urlencode(htmlspecialchars($photo['id'])),
 'caption' => htmlspecialchars($photo['caption']),
 'filename' => $url . htmlspecialchars($photo['filename']),
 'description2' => strip_tags($photo['description2'], $allowed),
 ];
 }
 }
}
http_response_code(isset($response['message']) ? 404 : 200);
echo json_encode($response);

Granted, some developers will not wish to write three nested condition blocks and might prefer to write early json exits. I am choosing not to do this because my script is not excessively wide and it makes only one call of http_response_code() and json_encode() at the end. "To each their own." Restructure the snippet however you like.

An alternative structure with multiple returns to avoid arrowhead code:

function getResponse() {
 if (!isset($_GET['id']) || !ctype_digit($_GET['id'])) {
 return ['message' => 'Missing/Invalid identifier provided'];
 }
 include_once('config_setup.php');
 $sql = "SELECT id, caption, filename, description2 FROM photographs WHERE id = " . $_GET['id'];
 $result = mysqli_query($db, $sql);
 if (!$result) {
 return ['message' => 'Please contact the dev team.'];
 }
 $photo = mysqli_fetch_assoc($result);
 if (!$photo) {
 return ['message' => 'No product.'];
 }
 $url = 'http://localhost/photo_gallery/public/files/images/';
 $allowed = '<div><img><h1><h2><p><br><strong><em><ul><li><table><td><tr><th><tbody>';
 return [
 'id' => urlencode(htmlspecialchars($photo['id'])),
 'caption' => htmlspecialchars($photo['caption']),
 'filename' => $url . htmlspecialchars($photo['filename']),
 'description2' => strip_tags($photo['description2'], $allowed),
 ];
}
$response = getResponse();
http_response_code(isset($response['message']) ? 404 : 200);
echo json_encode($response);

• Roland Illig: What about putting the whole code into a function and return when you're done? That avoids the deeply nested conditions.
• mickmackusa: Sure, I could just as easily support a return based approach. 3v4l.org/g9V0T My current boss actually doesn't like multiple returns in a function; I must say I prefer them.
• bumperbox: I like returns based approach too, although i often use multiple returns as guard clauses at the start of the function, but if i end up putting returns all over the place, the chances are the function could be refactored into multiple functions or simplified in other ways
• mickmackusa: I think my employer's stance is about speed of readability. The argument being that it is simpler for the human eye to follow indentation versus colored returns (amidst all other syntax which is colored by the IDE). Again, I am happy to adapt my code writing style to conform with specific projects or company/team policy.
• mickmackusa: The same can be said regarding my decision to not implement a prepared statement. The incoming value is suitably validated as an integer before being injected into the sql string. It isn't necessary, but if the project is using prepared statements for 100% of the db interactions, then I am happy to endorse conformity/consistency and implement a prepared statement.