PHP and design patterns : a smart way ?

Thanks to Jon Lemaitre’s excellent article “A Response to PHP the wrong way”, I recently discovered http://phpthewrongway.com self-exposed as the counterbalance of http://phptherightway.com. Then came the need to write an answer on the subject, my views on framework, OOP, standards and the rest but I realized I would end up with a very long article full of obvious arguments or at least statements many people already know. Boring. Seriously who wants to read another pros and cons on frameworks and OOP ? Is it really worth discussing it nowadays, knowing how many classes are offered by PHP itself and how its own syntax is object oriented ? I just think that if you don’t want to write OOP code with PHP, you’re free to use another language or to fork PHP 3.

The only subject I will cover in this post is the usage of design patterns. It could have been the wisest part of “PHP the wrong way” but the author failed to provide relevant arguments by focusing on a philosophical point of view. My goal here is to bring a more objective point of view with technical facts.

The wrong way: Looking for a pattern to solve a problem

True, this is a dogmatic approach we must stay away from. Patterns were designed to implement common needs like separation of responsibilities, open/close principle and to save some precious time finding design solutions. However, it has some drawbacks and the use of design patterns requires skills and common sense.

The absolute anti-pattern : the Golden Hammer

** With the Golden Hammer in my hands, everything looks like a nail. **

Want to access the database ? There’s a singleton for that. Want to write a log message? There’s another singleton for that. Want to to send an email? Singleton. Want to access the user’s session? Another singleton is there for you. Want to access user’s permissions ? This time it’s a “multiton” with the user’s group in getInstance() arguments. Want to send a postal card to your mom ? No problem, we’ll write a singleton. Want a glass of Whiskey ? Guess what, there’s a single malt for that (wait no, I’m going off-topic). Want to unit test your application ? Certainly not, PHPUnit is not a singleton ! Most of the time, a beginner was left alone on the project and he did his baby steps with the only toy he knew : singletons. Let’s just hope the project was not really important for the company otherwise a full re-writing is the only solution.

The Golden Hammer is the tool you use to solve everything. That’s why with the Golden Hammer everything looks like a nail : because I am obsessed with this tool and I pretend to have it under control in every circumstances, I will use it for everything. Try to build a house with only a good hammer, tons of nails and planks then send me pictures of it. With a good focus on the bathroom please, I want to see how watertight the shower cabin is.

You’ll have the same issue with software development : you can’t use the Golden Hammer to build everything, whatever the hammer is (observers, singletons, factories, etc …). Again, the best way to stay out of trouble is to work on the architecture and to spend the necessary amount of time on design before writing the code.

## Choosing Bad patterns : the evil of Singleton

I used the singleton to make my point on purpose because it’s a an easy pattern and almost all developers have already used it once. It’s designed to keep a single instance of an object in memory, which is useful when you need to manage sensitive resources such as database connection or the user’s session. Pretty simple and useful, however it has some issues.

Dependency management

With its own dependencies

By design, dependencies cannot be injected into a singleton. Take this one :

class Foo
{

    /**
     * @var Foo
     */
    static protected $instance;

    /**
     * @var \Monolog\Logger
     */
    protected $logger;

    /**
     * @return Foo
     */
    static public function getInstance()
    {
        if ( is_null(self::$instance) ) {
            $class = static::class;
            self::$instance = new $class;
        }

        return self::$instance;
    }

    /**
     * Foo constructor.
     */
    protected function __construct()
    {
        $this->logger = new \Monolog\Logger('main');
    }

    /**
     * @return int
     */
    public function get()
    {
        $this->logger->addInfo('call to get');

        return 42;
    }

}

This class is tightly coupled with its dependency, Monolog. You cannot call Foo::getInstance() without creating a new Monolog\Logger instance. Besides, this Logger instance can’t be configured outside Foo::__construct(), one solution is to access application’s configuration to build the Logger.

    /**
     * Foo constructor.
     */
    protected function __construct()
    {
        $config = Config_Adapter::getInstance()->get('log');

        $this->logger = new \Monolog\Logger('main', $config);
    }

See ? Another Singleton to handle configuration ! Now Foo has two dependencies : Monolog and a Config_Handler which needs to be initialized before the first call of Singleton::getInstance().

we can’t use Foo before initialising the Config_Handler
the Constructor loads a Monolog\Logger instance. If you already use it elsewhere, you’ll have two different instances.
get() triggers Monolog\Logger::add(), that means you’ll have a new log entry whenever you want it or not
last but not least, we need to bootstrap a fake application to run unit tests on this class.

The best thing to do in that case is to remove all the code from __construct() and to use a setLogger() method in order to initialise the Logger. But you’ll face another issue : the need to test if $this->logger is not null each time you need to use it. As a consequence you won’t be able to predict if Foo will log something or not while using its get() method. Which is an issue in another circumstances if the underlying dependency does important operations such as permissions checking or things like that.

As a dependency

Remember the first example we saw, and now read the following lines of code :

class Action
{

    /**
     * @return $this
     */
    public function run()
    {
        $bar = Foo::getInstance()->get();

        // do some stuff

        return $this;
    }
}

Singleton often comes with a terrible anti-pattern : hidden dependencies. Each time you call Action::run(), Foo triggers the Logger. How can we guess that while we are writing the Action class ? Without reading Foo’s code we can’t. It’s a bad surprise because it will again enforce to bootstrap a fake application to unit test Action.

Another case :

class Action
{

    /**
     * @return $this
     */
    public function run()
    {
        $bar = Foo::getInstance()->get();

        // do some stuff

        return $this;
    }

    /**
     * @param Entity $entity
     * @return $this
     */
    public function add(Entity $entity)
    {
        $entity->status = Entity::ADDED;
        $this->entity = $entity;
        Foo::getInstance()->store($entity);

        return $this;
    }
}

How to test it :

class ActionTest extends PHPUnit_Framework_TestCase
{
    public function testAdd()
    {
        $entity = new Entity;
        $entity->setName('foo');

        $action = new Action;
        $action->add($entity);

        $this->assertEquals(Entity::ADDED, $entity->getStatus());
    }
}

And boom ! I’m screwed. Because what I didn’t know is : Foo::store() performs a SQL query to the database. Which I didn’t setup before writing the unit test of course. Great, something else to add in my bootstrap, how clever ! I’m a good boy full of good will so I decide to refactor Action in order to inject its dependency through __construct() :

class Action
{

    protected $foo;

    /**
     * Action constructor.
     * @param Foo $foo
     */
    public function __construct(Foo $foo)
    {
        $this->foo = foo;
    }

    /**
     * @return $this
     */
    public function run()
    {
        $bar = $this->foo->get();

        // do some stuff

        return $this;
    }

    /**
     * @param Entity $entity
     * @return $this
     */
    public function add(Entity $entity)
    {
        $entity->status = Entity::ADDED;
        $this->entity = $entity;
        $this->foo->store($entity);

        return $this;
    }
}

Now I can mock Foo to write a valid unit test :

class ActionTest extends PHPUnit_Framework_TestCase
{
    public function testAdd()
    {
        $entity = new Entity;
        $entity->setName('foo');

        $foo = $this->getMock('Foo');
        $foo->method('store')->will($this->returnValue(null));

        $action = new Action($foo);
        $action->add($entity);

        $this->assertEquals(Entity::ADDED, $entity->getStatus());
    }
}

And I can use a dependency injection tool like Pimple in the application to handle Foo’s instance :

$container = new Container();

$container['foo'] = function () {
    return Foo::getInstance();
};

$action = new Action($container['foo']);

Good news : Pimple can hold a single instance for each class you make it handle. Foo doesn’t need to be a Singleton anymore. In fact, you don’t need to use the pattern anymore, which relieves your class from a superfluous responsibility : holding a single instance of itself.

** The smart way : when you want to write a singleton, drink a single malt instead. And think about it twice. **

A last word on Design patterns

Don’t get me wrong, I’m not against design patterns. I actually disapprove thoughtless decisions like trying to use patterns for everything, or choosing an easy one without thinking about the consequences. Because this kind of bad decision involves risks, difficulties and undesirable costs. Use design patterns wisely and you may have a good result, like a well structured application spared from duplications or excessive complexity in the code. This result depends on technical decisions, developers' skills, management, and many other factors. Not OOP, design patterns or a framework.

Final thoughts on PHP the wrong way.

If there’s a wrong way, “PHP the wrong way” uses it from the beginning to the end of the front-page : statements without any fact nor demonstration. It’s just a troll like the one we used to read on slashdot or linuxfr.org in the good old days. Not worth the reading.