1
\$\begingroup\$

I'm currently reading a book and watching some tutorials about PHP with MySQL (PDO), but it is not clear where to put certain things - where to put some code, should I put the connection to the database within try-catch and so on.

So, I'll explain how I started my project and if you could tell me if this is wrong or is there a better way.

  1. In the directory includes I have config.php:

    <?php 
    defined('DB_DNS') ? null : define("DB_DNS", "mysql:host=localhost;dbname=photo_gallery");
    defined('DB_USER')   ? null : define("DB_USER", "root");
    defined('DB_PASS')   ? null : define("DB_PASS", "");
    
  2. In the directory includes I have database.php:

     try {
       $connection= new PDO(DB_DNS, DB_USER, DB_PASS);
       $connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
     } catch(Exception $e) {
        echo "Error: ".$e->getMessage();
        die();
     }
    
  3. In the directory includes I have user.php with a class named User:

     <?php
    
     require_once(__DIR__."/../includes/config.php");
     require_once(__DIR__."/../includes/database.php");
    
     class User
     {
       public function findAllUsers() {
          try {
            global $connection;
            $sql = $connection->prepare("SELECT * FROM users");
            $result = $sql->execute();
            return $result;
          } catch (Exception $e) {
             echo $e->getMessage();
             die();
          }
       }
       public function find_by_id($id = 0) {
          try {
            global $connection;
            $sql = $connection->prepare("SELECT * FROM users WHERE id = :id");
            $result = $sql->execute(array($id));
            return $result->fetch(PDO::FETCH_OBJ);
         } catch (Exception $e) {
            echo $e->getMessage();
            die();
        }
      }
    }
    

Similar to this, some authors do tutorials and courses that I watch.

\$\endgroup\$
2
\$\begingroup\$

Ok there are three distinct parts I'll discuss, but the most important one is the last one (the User class).

Config constants:

defined('DB_DNS') ? null : define("DB_DNS", "mysql:host=localhost;dbname=photo_gallery");
defined('DB_USER')   ? null : define("DB_USER", "root");

Things like this are sadly quite common. The problem with this setup is that things like username, password and even the default DB name (owing to it being part of the DSN) are all hard-coded in your project. The chances of these credentials being tracked by a SVC system like git are really quite high. As are the chances that the credentials may vary depending on the environment the code runs on (local/develop/testing/production). These are configuration parameters that you really shouldn't keep hard-coded in the code base

All things aside, it's also a lot shorter to write:

defined('SOME_CONSTANT') || define('SOME_CONSTANT', 'define here');

You're using a ternary to set a given value to null. Now a DSN can't be null, nor can the DB username. You should error in those cases, instead of setting the values to null.

try-catch-die

Code like this:

try {
    $connection= new PDO(DB_DNS, DB_USER, DB_PASS);
    $connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(Exception $e) {
    echo "Error: ".$e->getMessage();
    die();
}

is again all too common. But it's smelly for a number of reasons:

  • Exception's indicate something Exceptional has happened, and requires special attention by a piece of code written especially for such an event, often this is an Exception-handler. A basic handler will check some parameters to see if it can/should output details about the error, or hide them from the client, and will almost always write the full stack trace to some log, so you can diagnose the problem later on. What you're doing here is squelching that information, echo an often cryptic error message and then die. That's not handling the exception, that's giving up
  • PDO never throws a "regular" Exception instance, it throws PDOExceptions, your catch block should reflect this. This becomes especially useful when you're working on larger projects where a single method call can throw any number of different exceptions, and your calling it from a place where you can handle some of these exceptions, but can't handle others. In that case, you'll write something like catch (ExceptionICanHandle $e) {} and let other exceptions propagate to code that can handle it
  • Your DSN string is missing the ;charset=utf8 bit at the end (required to protect against injection)
  • You're calling setAttribute after constructing the instance. You needn't do that, the PDO constructor takes a fourth argument (array of attributes).

In short, here's what I'd write:

//parse or "obtain" config in some way
$dsn .= ';charset=utf8';//in case the charset is still missing, fix the config. I'm adding it here as a reminder
$connection = new PDO($dsn, $user, $pass, [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);

No try-catch nonsense, no die and no setAttribute. Just 1 line that does it all.

The User class

Your class suffers from a number of issues. The short list:

  • global is code-smell, a sign of bad design. Don't use it unless you really have to. And you really never need to use it (I've not seen a valid use case in 10 years +)
  • Again: try-catch-die, I've explained what is wrong with this
  • echo in a method is code-smell: SRP (Single Responsability Principle) is one of the core principles to follow if you want to write good code: A class should have only one job. User is a data object, it represents a DB record that represents a user of sorts. That's its job, nothing more, nothing less. Taking care of output is the job of a template/view.
  • Coding standards are important. Your class does not conform to the most widely adopted standard: PHP-FIG (PSR)
  • This is perhaps personal, but for the same reasons (SRP), I don't like data models to have find or save methods defined. That's the job of a DB mapper, or in doctrine-speak, an entity manager. Still, it's not unheard of to have data models with DB connections, so this is just a matter of opinion

In short, and assuming you're to keep the find_by_id method, here's what I'd have the User class look like:

class User
{//next line <-- PSR
    /**
     * @var PDO
     */
    protected $connection = null;

    /**
     * @var array
     */
    protected $statments = [];

    /**
     * @param PDO $connection
     */
    public function __construct(PDO $connection)
    {
        $this->connection = $connection;
    }

    /**
     * @param int $id
     */
    public function findById($id)
    {
        if (!is_numeric($id) || $id < 1) {
            throw new InvalidArgumentException('ID must be positive integer');
        }
        $method = __METHOD__;
        if (!isset($this->statements[$method])) {
            $this->statements[$method] = $this->connection->prepare('SELECT * FROM user WHERE id = :id');
        }
        $stmt = $this->statements[$method];
        $stmt->execute([':id' => $id]);
        $data = $stmt->fetch(PDO::FETCH_ASSOC);
        $stmt->closeCursor();
        if (!$data) {
            return null;
        }
        $found = new User($this->connection);//pass connection
        //load data model
        $found->setId($data['id'])
            ->setName($data['name']);
        return $found;
    }
}

The other method, I'd write in the same way, but I'll leave that for you to write. I'll have a look to previous reviews I've posted here that do roughly the same thing (query the DB, setup a data model etc...), because this review was written in a hurry, the code is not tested, and I might have left some things out

related review - links to 4 other reviews

\$\endgroup\$
  • 1
    \$\begingroup\$ @PeraMika: Configuration is best kept in a data file-format of choice (ini, yaml, json or XML files. Those files should then be read/parsed/loaded into a data structure (most commonly a class) that holds all config parameters that you can access along these lines: $config->get('dsn'); \$\endgroup\$ – Elias Van Ootegem Oct 27 '15 at 7:55
  • 1
    \$\begingroup\$ @PeraMika: I'm passing the connection because, in its current form, the User class has to have a PDO instance passed to its connection. I'm also returning an instance of User instead of the array $data because that's what I'd expect a method called User::findById to return: a User, not an array of data. It makes the whole code truly object oriented: calling methods that return the requested data in a normalized form (ie an object), which can be passed to other methods that can then type-hint: public function updateUser(User $user, array $data) \$\endgroup\$ – Elias Van Ootegem Oct 27 '15 at 9:51
  • 1
    \$\begingroup\$ As for the fatal error: The code I posted here is incomplete. You'll have to declare properties for each field in the user table (id, email, name, etc...), and have a getter and a setter for each of these properties (check one of the linked reviews, there is bound to be a more complete example in one of them). I'm using __METHOD__ to create a unique key so I don't have to call PDO::prepare each time User::findById is called. I can re-use the prepared statement if I've already created it. It's a basic way of saving on repeated prepare calls \$\endgroup\$ – Elias Van Ootegem Oct 27 '15 at 9:54
  • 1
    \$\begingroup\$ @PeraMika That would be fine, but in that case I'd change the method name from findById to loadUserById($id), because I'd expect a find method to return what it finds, null if nothing was found. If you want to populate the current instance, that's fine but think about how you handle an empty result-set: realisticly, the only thing you can do is throw an exception. If you don't, you have to hope the caller will bother to check if $user->loadById(1); actually set the properties correctly. Something that gets even harder if $user wasn't empty to begin with... \$\endgroup\$ – Elias Van Ootegem Oct 27 '15 at 11:59
  • 1
    \$\begingroup\$ @PeraMika: That method should indeed return an array of User instances. using PDO's FETCH_CLASS is something I generally advice against. It actually bypasses the constructor, or any getters/setters you may define. It assigns the class' properties directly, bypassing any code that validates/filters/formats the data itself. Besides: all you really need to do is write a simple loop, that's not that hard \$\endgroup\$ – Elias Van Ootegem Oct 27 '15 at 12:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.