PDO - configuration, database connection & and using 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.


In the directory includes I have config.php:

&lt;?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", "");

In the directory includes I have database.php:

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

In the directory includes I have user.php with a class named User:

 &lt;?php

 require_once(__DIR__."/../includes/config.php");
 require_once(__DIR__."/../includes/database.php");

 class User
 {
   public function findAllUsers() {
      try {
        global $connection;
        $sql = $connection-&gt;prepare("SELECT * FROM users");
        $result = $sql-&gt;execute();
        return $result;
      } catch (Exception $e) {
         echo $e-&gt;getMessage();
         die();
      }
   }
   public function find_by_id($id = 0) {
      try {
        global $connection;
        $sql = $connection-&gt;prepare("SELECT * FROM users WHERE id = :id");
        $result = $sql-&gt;execute(array($id));
        return $result-&gt;fetch(PDO::FETCH_OBJ);
     } catch (Exception $e) {
        echo $e-&gt;getMessage();
        die();
    }
  }
}



Similar to this, some authors do tutorials and courses that I watch.
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-&gt;setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(Exception $e) {
    echo "Error: ".$e-&gt;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 =&gt; 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 &lt;-- PSR
    /**
     * @var PDO
     */
    protected $connection = null;

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

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

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