PHP OOP: Configuring MySQL DB & query w/ Twig

Question

I decided to up my PHP game and learn some OOP. I am re-building my website from procedural to OOP, but since I don't want to use a full framework I fiddle with some components. As a router I use https://github.com/skipperbent/simple-php-router As templating engine I use Twig.

Before I start do more coding I just wanted to ask for your feedback whether my code is considered "good practice" or I would run in some issues while my project grows.

This is my folder structure:

    - my project/
     - config/
       config.php # config incl. db details
     - public/
       index.php # Frontcontroller
     - src/
       - Controllers/
         controller.php
       routes.php
     - templates/
     ...

This is my front controller, public/index.php:

    require __DIR__.'/../vendor/autoload.php';
    require __DIR__.'/../config/config.php';
    /* Load external routes file */
    require __DIR__.'/../src/helpers.php';
    require __DIR__.'/../src/routes.php';
    require __DIR__.'/../src/Controllers/controllers.php';
    
    use Pecee\SimpleRouter\SimpleRouter;
    SimpleRouter::start();

In the config file, I save the details to connect to the MySQL database in a Class, config/config.php:

    class Database {
        protected $username = 'username';
        protected $password = 'password';
        protected $servername = 'localhost:3307';
        protected $database = 'dbname';
    }

In the controller file src/Controllers/controller.php I have my classes:

    function load_twig() {
        $loader = new \Twig\Loader\Filesystemloader('../templates');
        $twig = new \Twig\Environment($loader);
        return $twig;
    }
    
    class db_connection extends Database {
        public function __construct() {
            try {
                $this->dbh = new PDO("mysql:host=$this->servername;dbname=$this->database", $this->username, $this->password);
                $this->dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            } catch(PDO_EXCEPTION $e) {
                echo "Connection failed";
            }
        }
    
        public function close_db_connection() {
            $dbh = null;
        }
    }
    
    class Faq {
        public function get_faq() {
            $db = new db_connection();
            $query = 'SELECT * FROM faq';
            $statement = $db->dbh->prepare($query);
            $statement->execute();
            $row = $statement->fetchAll(PDO::FETCH_ASSOC);
            $db->close_db_connection($db);
            return $row;
        }
    }

In the src/routes.php I define the routes for the website, e.g. for faq-section:

    SimpleRouter::form('/faq', function() {
        $data = new Faq();
        $twig = load_twig();
        echo $twig->render('faq.html', ['faq' => $data->get_faq()]);
    });

So, when someone navigates to FAQ-Section on the website, the class Faq with method get_faq() is called. Within get_faq() I call db_connection Class with __constructor method, which establishes a new connection to the MySQL database, returns a query and the results are stored in $row variable. Afterwards connection to database gets closed. I then pass $row to Twig-Template and data are parsed in my template.

It works fine, but this is probably the easiest query I'll have on my website and I want to make sure this is the right & secure way to do things. I am the only developer and probably no one will ever see the code of the website, but performance might in the future get important and I want to make sure, to do things right from the start.

I wonder if it would make more sense to establish the db connection in the front-controller?

Thanks for your feedback.

Answer 1

You really should use an autoloader. Either Composer autoloader or the PHP built-in one.

---

Your close_db_connection method is useless, you can safely remove it. You have no reason to prematurely close the connection so you don't even need to bother fixing this method.

---

echo "Connection failed";

This is not a useful error message to the user. A user should just see a generic 500 error and the real database error should be logged. I can understand that you want to catch the exception and rethrow your own one to prevent logging credentials, but you must be able to trace what went wrong. You can't just ingore the exception in catch.

---

In the controller file src/Controllers/controller.php I have my classes:

You should have only one class per file. Please learn PSR-4 and follow it accordingly.

---

What you are doing in this code isn't really OOP:

    class Faq {
        public function get_faq() {
            $db = new db_connection();
            $query = 'SELECT * FROM faq';
            $statement = $db->dbh->prepare($query);
            $statement->execute();
            $row = $statement->fetchAll(PDO::FETCH_ASSOC);
            $db->close_db_connection($db);
            return $row;
        }
    }

Use dependency injection and don't create the connection multiple times. That FAQ class should really be in your model.

---

What is the purpose of this code?

    class Database {
        protected $username = 'username';
        protected $password = 'password';
        protected $servername = 'localhost:3307';
        protected $database = 'dbname';
    }

It seems utterly useless, but if this is your idea of a config file, I need to tell you that there are better ways. Database credentials should not be stored in properties. Also, inheriting in this way class db_connection extends Database is really confusing. Just by looking at the names, one can never know what each class does. How is db_connection a specialization of Database?

---

There's no need to call prepare and execute separately if you have no variables to bound. You can use query() which is a shorthand for prepare & execute.

---

You really should consider making use of your database abstraction class. At the moment, your db_connection is just a useless wrapper around PDO. You can add helper methods and make $dbh a private property. You should also declare that properly instead of relying on dynamic properties.

---

PDO_EXCEPTION doesn't exist. You are looking for PDOException.

---

Please define your connection charset. You will have problems later if you don't.

---

Consider an improved version of your code:

class Database
{
    public function __construct(array $config)
    {
        try {
            $this->dbh = new PDO(
                "mysql:host={$config['servername']};dbname={$config['database']};charset=utf8mb4",
                $config['username'],
                $config['password'],
                [
                    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
                    PDO::ATTR_EMULATE_PREPARES => false,
                ]
            );
            $this->dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        } catch (\PDOException $e) {
            throw new \PDOException($e->getMessage(), (int) $e->getCode());
        }
    }

    public function fetchAllRows(string $query, array $params = []): array
    {
        $statement = $this->dbh->prepare($query);
        $statement->execute($params);
        return $statement->fetchAll(PDO::FETCH_ASSOC);
    }
}

class Faq
{
    public function __construct(private Database $database)
    {
    }

    public function get_faq(): array
    {
        return $this->database->fetchAllRows('SELECT * FROM faq');
    }
}

It's far from production-ready, but it at least resembles OOP. Each class should be in a separate file. The dependency is always provided in the constructor. A class is responsible for only one thing. The database abstraction class is responsible for connecting and executing SQL. The FAQ class belongs in your model and is responsible for everything related to your business logic.