PHP OOP classes used for building web applications

Question

The following classes are used for web development. And I was wondering if I'm implementing them correctly specially the Base class.

This is a version 2.0 of the question the classes are as follows:

MyPDO
Session
Security
Base

Examples:

<?php
//using the autoloader
require "../classes/autoloader.php";

// checking if the user isn't logged in so he can loggin
$session = new Session;
if ($session->is_logged_in()) {
    Base::location();
}

// set session variables used by the class "logging" the user in
$session->initialize_user_session($user["admin"], $_POST["username"]);

//Redirect to another webpage and exit
Base::location();

//Encoding output to prevent XSS
$html = "<script>alert('XSS')</script>";
echo "<h1>". Security::clean_html($html) ."</h1>";

My doubts are should I use the constants and is base class any good since it only has two functions and they are not that related? The code is:

MyPDO class (made from phpdelusions.net) :

define("DB_HOST", "localhost");
define("DB_NAME", "root");
define("DB_USER", "root");
define("DB_PASS", "root");
define("DB_CHAR", "utf8mb4");


class MyPDO extends PDO
{
    public function __construct($dsn = NULL, $username = DB_USER, $password = DB_PASS, $options = [])
    {
        $default_options = [
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        ];
        $options = array_replace($default_options, $options);
        $dsn = $dsn ?? "mysql:host=".DB_HOST.";dbname=".DB_NAME.";charset=".DB_CHAR;
        parent::__construct($dsn, $username, $password, $options);
    }

    public function run($sql, $args = NULL)
    {
        if (!$args)
        {
            return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
    }
}

Session class:

/*
 * Session handling class
 */

define("ADMIN_VALUE_KEY", "admin");
define("LOGIN_VALUE_KEY", "logged_in");
define("USER_ID_KEY", "user_id");
define("CSRF_TOKEN_KEY", "csrf_token");
define("LOCATION_DEFAULT_DIR", "index.php");

class Session
{
    public function __construct()
    {
        session_start();
    }

    public function initialize_user_session($admin, $user_id) {
        $_SESSION[ADMIN_VALUE_KEY] = $admin;
        $_SESSION[LOGIN_VALUE_KEY] = true;
        $_SESSION[USER_ID_KEY] = $user_id;
        $_SESSION[CSRF_TOKEN_KEY] = Security::generate_token(64);//bin2hex(random_bytes(32));
    }

    public function logout(){
        session_destroy();
        Base::location();
    }

    public function is_logged_in() {
        return (!empty($_SESSION[USER_ID_KEY]));
    }

    public function is_admin() {
        return (!empty($_SESSION[ADMIN_VALUE_KEY]));
    }

    /*
     * Check functions
     */
    public function check_token($token, $dir = LOCATION_DEFAULT_DIR)
    {
        if (!hash_equals($_SESSION[CSRF_TOKEN_KEY], $token)) {
            Base::location($dir);
        }
    }

    public function check_login($dir = LOCATION_DEFAULT_DIR)
    {
        if (empty($_SESSION[USER_ID_KEY])) {
            Base::location($dir);
        }
    }

    public function check_admin($dir = LOCATION_DEFAULT_DIR)
    {
        if (empty($_SESSION[ADMIN_VALUE_KEY])) {
            Base::location($dir);
        }
    }
}

Security class:

class Security
{
    public static function generate_token(int $length) : string {
        return bin2hex(random_bytes($length/2));
    }

    public static function clean_html(string $html) : string {
        return htmlspecialchars($html, ENT_QUOTES, 'utf-8');
    }

    public static function clean_json(string $json) : string {
        return json_encode($json, JSON_HEX_QUOT|JSON_HEX_TAG|JSON_HEX_AMP|JSON_HEX_APOS);
    }
}

Base class:

define("LOCATION_DEFAULT_DIR", "index.php");

class Base
{
    public static function location($dir = LOCATION_DEFAULT_DIR)
    {
        header("Location: " . $dir);
        exit();
    }

    public static function check_input($required, $error)
    {
        foreach ($required as $field) {
            if (empty($_POST[$field])) {
                Base::location($error);
            }
        }
    }
}

slepic · Accepted Answer · 2019-12-03 18:19:54Z

Constants - glabal vs. class scope

Definig global constants should be considered a bad practice. You should move them to the classes above which you are defining them now.

You should also consider defining those constants private unless you need them elsewhere too (in which case they probably belong elsewhere themselves and the class in question should accept them as constructor arguments)

class Session
{
    private const ADMIN_VALUE_KEY = "admin"
}

Multipurpose methods

Methods like this one:

public function run($sql, $args = NULL)
{
        if (!$args)
        {
            return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
}

should also be considered a bad practice. Not talking about the fact that run is pretty bad name for this, IMHO anyway. What I mean is the method takes one argument and does something, or it takes two arguments and does something else. PHP does not have the feature of method overloading. And so you should define two separate methods for this (like PDO does). Well I know from certain angle you could say it is the same thing. And if you were forced to use two arguments because you are implementing an interface, then I would not object (except maybe the interface deserved some inspection).

Extending PDO

Extending PDO is not a very good idea as well. You are doing it for 2 reasons:

you wanted to simplify query with/without args but that is IMO useless. You always know if you want to pass arguments or not and so you can choose the right method to use.
you wanted to allow default constructor with all your credentials passed, but that is not IoC, it is not SRP, it is not flexible, it introduces extra dependency...

The simplest design pattern of all comes to rescue, the factory!

function createMyPDO(): \PDO
{
  return new \PDO(/* hardcode it, load it from config file, get it from class variables, load it from database (just kidding:)), ... */);
}

Type Hints

You should be as specific about the types of arguments as possible. Omitting them is, well yea, it is a minor performance gain, but it is a major readability drop.

As seen on the Security class, you definitely have access to the scalar typehints feature. So why not use it?

Also if you expect array why accepting null? Like in the already mentioned run method.

Single Exit Point

public static function location($dir = LOCATION_DEFAULT_DIR)
{
    header("Location: " . $dir);
    exit();
}

Avoid calling exit on multiple places, there should only be one exit in your application/page. And by exit I mean a call to exit or the natural end of program. Well, we all use (or used to use) var_dump (and alike) with exit/die for development purpose (take a look at xdebug and forget var_dump btw) but in production code, only one exit point should exist, because there may be task(s) that need to be done before exit. And even if there is none now, adding it in future is like nothing if you have only one exit point.

Code Style

This is definitely a matter of preference, but it looks weird to me to have classes starting with capital letter, then methods with snake case. So I would just recommend to stick with the most common way in PHP world, and that is:

PascalCaseClass::withCamelCaseMethods()

Base

The Base class looks like you didn't know what to do with those functions. They are what was left after some refactor and class splitting. But just because they are only two doesn't mean they belong together.

Base::location();

doesn't mean anything to me.

Response::redirect()

on the other hand tells me something about what it does.

Also you dont have to wrap every two lines in a function.

The below is absoutely ok to do and I would actually understand what it does

if (!Form::checkRequiredFields($required, $_POST)) {
    return Response::redirect($error);
}

unlike I would from

Base::check_input($required, $error);

A very good review with many valid points. Only a little correction: the purpose of the run() function is to simplify query with args in the first place which greatly reduces the amount of code to be written. the "without" part is indeed useless but that' s not the point — Your Common Sense
– Your Common Sense, Commented Dec 3, 2019 at 18:09
@YourCommonSense You can still call $pdo->query($sql) without args and $pdo->prepare($sql)->execute($args) with args. Not sure if it is worth simplifying to $mypdo->run($sql, $args), especially if that's the only reason to extend PDO class in the first place. — slepic
– slepic, Commented Dec 3, 2019 at 18:16
I am genuinely curious, would will be your approach, given run() is renamed to query_params() with the empty $args functionality removed? — Your Common Sense
– Your Common Sense, Commented Dec 4, 2019 at 13:20
@YourCommonSense First for the fetch. It's true you usualy need that as well, but the run method is not calling it in either case, so that is irrelevant here. For the query_params, I already said I don't think it is worth it. Anyway I didn't really propose that empty $args functionality is removed, what I wanted to say is that $pdo->prepare($sql)->execute([]) will also work. I actualy believe that PDO::query is just a shorthand for this. Anyway my approach would be to not use PDO directly, I would define interface with methods I care of and wrap PDO in an implementation of that interface — slepic
– slepic, Commented Dec 4, 2019 at 15:55

Stack Exchange Network

PHP OOP classes used for building web applications

1 Answer 1

Constants - glabal vs. class scope

Multipurpose methods

Extending PDO

Type Hints

Single Exit Point

Code Style

Base

You must log in to answer this question.

Linked

Hot Network Questions

PHP OOP classes used for building web applications

1 Answer 1

Constants - glabal vs. class scope

Multipurpose methods

Extending PDO

Type Hints

Single Exit Point

Code Style

Base

You must log in to answer this question.

Linked

Related

Hot Network Questions