PHP class architecture and dependency injection

Question

Let me start off by saying I am not new to PHP or PDO, but very new to OO PHP, and have been struggling to get a firm idea of best practices in my head regarding dependency injection and structuring my classes and their methods.

The code I am providing will clearly be built on once I know I am doing things the correct way, so imagine more methods etc. using the same techniques I have used below, and what needs to be changed to make the whole package efficient and stable.

Whilst I am working on other PHP projects at work, the code below is not for or from any project, simply to help me learn before using OO in a real project. I also would normally do far more error checking, checking variables are set etc but have fiddled with this code so much, I haven't done this yet.

Here are the issues I see with it:

1. It seems that I am not taking advantage of the extra efficiency prepared statements bring when re-using the same statement but with different values. By doing this in a method like below, I would be preparing the statement every time, even if I just wanted to change the values.
2. I am not using a dependency injection container. Whilst it may not be necessary for small classes like these, I feel it would be better to use on in the long run.
3. I am worried I am on the track to creating tonnes of methods for every different type of query - see the findById and returnObjectById methods - they are structured very differently - one calling a query method in the Database class, the other running the query within the User class - I have no idea which is best. But also I feel I could end up creating a new page, realizing I need ANOTHER type of query, and then having to either write one in that file as a single instance (which would make me think why don't I do this for all), or create ANOTHER method in the User or Database class.
4. Are my methods within the correct classes - should the returnObjectById for example, be in the database class?

Note: Classes are auto loaded with spl_autoload_register in the config file, where DB constants are also set.

class_Database.php

    class Database
    {
    private $conn;

    public function __construct() {
        $this->openConnection();
    }

    public function getConnection()
    {
        $conn = $this->conn;
        return $conn;
    }

    public function openConnection() {
        try {
            $this->conn = new PDO('mysql:host=' . DB_SERVER . '; dbname=' . DB_NAME, DB_USER, DB_PASS);
            $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        } catch (PDOException $e) {
            echo 'There was an error connecting to the database, Error: ' . $e->getMessage();
            die();
        }
    }

    public function standardQuery($sql, array $params)
    {
        $stmt = $this->conn->prepare($sql);
        $stmt->execute($params);
        return $stmt->fetch(PDO::FETCH_ASSOC);
    }
}

class_User.php

 class User
    {   
    public $id;
    public $first_name;
    public $last_name;
    public $username;
    public $password;
    public function __construct() 
    { 

    }
    public function findById(Database $db, $id) 
    {   
        $user = $db->standardQuery("SELECT * from users WHERE id = :id", array(':id' => $id));
        return $user;
    }
    public function returnObjectById(Database $db, $id)
    {   
        $stmt = $db->getConnection()->prepare("SELECT * from users where id = :id");
        $stmt->bindParam(':id', $id);
        $stmt->execute();
        return $stmt->fetchAll(PDO::FETCH_CLASS, "User");
    }
}

Essentially, answers to my points above would be great, but I'd like to know if I am in any way moving in the right direction, or if I should stop and start over with a different technique.

Answer 1

Alright, let's start with your concerns, and then move on to the actual review:

1. A prepared statement costs about 50 times a normal statement. However, that cost is on the database server's side (i.e. on MySQL's side), preparing a statement A, then clearing the statement object on PHP, then preparing the same statement A (with different parameters) will not incur a penalty, because the query is cached by mysql.
2. A dependency injection container has advantages and disadvantages. It simplifies the dependency wiring process at the cost of magic. A dependency injection container is completely optional, don't sweat about it (especially in small projects).

3. What are you abstracting? That's a question that you should ask yourself every time you create a class or a layer (which is a bunch of classes that have the same purpose). You seem to trying to abstract the PDO object, hiding it from the user. But why? PDO is an abstraction on its own, you don't really need to abstract it. What you should abstract is either

   • Storage itself. Make a mapper that takes an object and stores/fetches it from storage. Note that I didn't say MySQL because mappers are storage agnostic, then you can work seamlessly across different storage types.
   • The SQL language. Make a criteria based API (also commonly known as DSL). Something like this: $db->select()->from('table')->where(Database::EQUALS("column", "value"))->limit(10); It's up to you.

   Your current abstractions don't really add value to an application. Why would I use your class over plain 'ol PDO?

4. Yes, it should, and I'll explain. When you create an object, it should do one thing and one thing only. Your user object should be responsible for keeping track of data about the user. It's an object that represents a single user in your data model.

   Your user should not be aware of its own means of storage. A user specifically can come from many places. It can come from a register form, it can come from a database, it can come from a migration CSV file containing hundreds of users to be inserted at bulk.

   As a result, your user should be storage agnostic, it should not care where the data came from, it should just accept it in the constructor, and work with what it's got.

---

A few more points:

• Always keep your code properly and consistently indented.
• Don't catch the database exception and kill the script every time when there's an error. Let the caller decide! What if I can live without a database and will rely on alternative storage methods until I can get the database back online?

• Don't rely on constants for the database credentials! Pass the to the constructor!

  public function openConnection(string $host, string $dbname, string $username, string $password) {
  

• By default, refrain from having public fields. Everything should be as private as possible, and only what you want to expose to the outside world is public.

• Assuming your user object is bound to the database, a database object would be part of the object's state, it's bound to that database instance. Why not pass it in the constructor and keep it as a property?

• PHP7 added the ability to type-hint scalars (such as string), use that in your advantage.

• Your instance methods seem to return a new user object, rather than changing the current one, that seems counter intuitive, make changes to the current object. If the consumer wants a different user, they should create a new User.