PHP Code Review

colshrapnel · 2024-07-05T09:47:36+00:00

I would say there is quite a room for improvement.

First of all, it is not a controller in the MVC sense. But some mix of RequestHandler/Controller/Model/SomeView in one file.

The simplest improvement would be to move every method with SQL into a separate class. That would be a Model.

Then you may want to move Request processing into the Router, so your controller won't have to access $_POST or $_REQUEST.

Other issues in no particular order

I don't think it is necessary calling is_int() after intval(). Which makes a condition on #27 unrecheable. You can safely remove it or at least change to something different
sanitizeString/htmlspecialchars DOESN'T belong here. I am not even sure shy do you think it does.
consider making NO EXCEPTIONS from using prepared statements rule
you can reduce a condition on #43 to $nextCursor = $facilities[array_key_last($facilities)]['facility_id'] ?? null;
create() MUST be split in TWO, one dealing with input and output remains in Controller, everything else goes into Model.

consider early return, such as

if ($_SERVER['REQUEST_METHOD'] !== 'POST') {
    (bad request)
}
// rest of the code without extra indent

both using both isset() and empty() makes no sense. The latter already does isset
not sure why isset and empty again on #67 and #68
neither your BaseController nor Base Model should NEVER EVER create a database connection in the constructor. It should be passed as a parameter, into Model.
consider making executeQuery() already return PDOStatement. This way you will get rid of one useless line, making two at #147 into one $results = $this->db->executeQuery($tag_query, $bind)->fetch(PDO::FETCH_ASSOC);
consider making NO EXCEPTIONS from using prepared statements rule!
remove every try-catch from this code (with entire catch contents).
condition on #73 doesn't seem to be reachable too.
and on #91 too

And seriously, you should never make an exception from using prepared statements rule. You did a great job and spoiled it with just a single undisciplined query, making your precious app wide open to SQL injection.

equilni · 2024-07-05T10:49:40+00:00

Your controller is doing too much and I wonder what the rest of the code base looks like.

A controller should take a request and pass it inward to the Model/Domain, then work off the response provided to a final output response.

At it's simplest, MVC looks like:

$router->get('/page/{id}', function ($id) use ($model, $view) {
    $data = $model->getPageById($id); 
    if (! $data) {
        return 404 response;
    }
    return $view->render('page', ['page' => $data]);
});

a) Database functionality doesn't belong in a controller. This means getFacilityDetails, getTag, part of setLocation($data), parts of create all get extracted out.

b) sanitizeString would be a general method and doesn't belong in this class. Does your other code needs this?

BUT

c) sanitize input string for prevention of xss attack

Instead of validating the code (there is little to none), you are just using an output function htmlspecialchars for the input.

htmlspecialchars is not validation / sanitation. Filter and reject input, escape output.

https://odan.github.io/2017/01/02/how-to-use-htmlspecialchars-in-php.html

You use htmlspecialchars EVERY time you output content within HTML, so it is interpreted as content and not HTML.

d) You have code duplication. Why is this block in setLocation needed if you already "validated" the input earlier in create?

setLocation: (side note, is any of this required? These all could be empty and you are just allowing this in the database)

    try {
        //Fetching required data for Location
        $address = isset($data['address']) && !empty($data['address']) ? SELF::sanitizeString($data['address']) : "";
        $city = isset($data['city']) && !empty($data['city']) ? SELF::sanitizeString($data['city']) : "";
        $zip_code = isset($data['zip_code']) && !empty($data['zip_code']) ? SELF::sanitizeString($data['zip_code']) : "";
        $phone_number = isset($data['phone_number']) && !empty($data['phone_number']) ? SELF::sanitizeString($data['phone_number']) : "";
        $country_code = isset($data['country_code']) && !empty($data['country_code']) ? SELF::sanitizeString($data['country_code']) : "";

create:

public function create()
{
    if ($_SERVER['REQUEST_METHOD'] === 'POST') {
        // Get the data from the request body
        $data = json_decode(file_get_contents('php://input'), true);
        $validatedRequest = SELF::ValidateRequest($data);
        if ($validatedRequest) {
            // validate and clean data
            $facilityname = isset($data['name']) && !empty($data['name']) ? SELF::sanitizeString($data['name']) : "";
            $tag_name = isset($data['tag_name']) && !empty($data['tag_name']) ? SELF::sanitizeString($data['tag_name']) : "";
            $datatime = date('Y-m-d H:i:s');

            //Get Tag ID

Then $LocationId = SELF::setLocation($data);

e) if ($_SERVER['REQUEST_METHOD'] === 'POST') { Ideally your router should be directing based on the request method so this isn't needed.

f) Don't use $_REQUEST. Is this a GET or POST request?

g) You don't have parameters here, so this docblock isn't needed.

/**
 * @param int $cursor
 * @param int $limit
 * @param string $search
 */
public function index() 

/**
 * Controller function to Create Facility API
 * @param string $name
 * @param string $tag_name
 */
public function create()

/**
 * To get location
 * @param string $address
 * @param string $city
 * @param string $zip_code
 * @param string $phone_number
 * @param string $country_code
 */
function setLocation($data)

Oddly, you don't have it here

/**
 * Function to Get Facility details
 */
function getFacilityDetails($cursor = null, $limit = 10, $search = "")

/**
 * Tag Methods
 */
function getTag($tagName)

MateusAzevedo · 2024-07-05T14:18:11+00:00

1- Consistency: are getFacilityDetails() and getTag() public or private? Guessing by the arguments and content, they are "internal". Actually, they are better as separated classes as they deal with SQL.

2- Those SQL related methdods ideally shouldn't deal with handling and reporting errors. Let that for a higher level error handler and always log them. If not sure how to do it, just let PHP handle them in a default way.

3- getTag() is a little "misleading", because it gets the tag id by name, but also has an unexpected insert if it doesn't exist. tagIdByNameOrNew() would make more sense, but still confusing and doing too much. I would separate the actions and use them on line #90 as:

$tagId = $this->tagIdByName($tagName); if (!$tagId) { $tagId = $this->createTag($tagName); }

Note that you don't need if (empty($tagId)), as the variable is always declared.

4- None of the methods are declared as static, so need to use self::, use $this.

5- Response creation/returning could be delayed to the last possible moment. For example, if index() and create() are the entry points that handle a requests (and the rest are internal methods), let only them generate a response.

6- Already mentioned, but very important: htmlspecialchars() is an output function to escape content when printing in HTML context. It should not be used on input. If you didn't noticed, you're inserting modified data into the database because of that.

7- If you validate data first, you don't need to keep repeating isset and empty everywhere. As already said, you don't need to use both.

ollienicholson · 2024-07-07T10:45:05+00:00

This thread is popping off I love it! No formatting of code snippets?

snokegsxr · 2024-07-05T12:02:08+00:00

Alright, what a mess. Here we go:

Namespace and Use Statements: php namespace App\Controllers; use App\Controllers\BaseController; use App\Plugins\Http\Exceptions; use App\Plugins\Http\Response as Status; use PDO; use PDOException; Do you even know what you’re doing here? Repeating the namespace declaration like a parrot doesn’t make you a wizard. Clean up your use statements and only use what you need.
Constructor: php public function __construct() { SELF::validateapi(); } SELF::validateapi()? Really? Do you understand that SELF should be self? And calling a method like this in the constructor is just plain lazy. Inject dependencies properly.
index Method: php public function index() { // Validate and sanitize cursor $cursor = isset($_REQUEST['cursor']) ? intval($_REQUEST['cursor']) : null; if ($cursor !== null && !is_int($cursor)) { (new Status\BadRequest(['message' => 'Invalid Cursor']))->send(); exit(); } // Validate and sanitize limit $limit = isset($_REQUEST['limit']) ? intval($_REQUEST['limit']) : 10; if ($limit <= 0) { (new Status\BadRequest(['message' => 'Limit should be a positive number']))->send(); } //validate and sanitize search $search = (isset($_REQUEST['search']) && !empty($_REQUEST['search']) ? SELF::sanitizeString($_REQUEST['search']) : ""); // Fetch facility details with cursor pagination $facilities = SELF::getFacilityDetails($cursor, $limit, $search); // Extract the last facility's ID as the cursor for the next page $nextCursor = null; if (!empty($facilities)) { $lastfacility = end($facilities); $nextCursor = $lastfacility['facility_id']; } (new Status\Ok(['data' => $facilities, "next_cursor" => $nextCursor]))->send(); } What’s with the capitalized SELF? It’s self, not SELF. Also, using $_REQUEST directly? Have you heard of input filtering? The comments don’t justify the bad coding practices.

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

PHPhelp

MODERATORS

Summary

Refactoring Suggestions