The reason why singleton is a “problem” with PHPUnit


Singleton is a common design pattern. It’s easy to understand and we can easily implement it with PHP:

<?php
class Foo
{
    static private $instance = NULL;

    static public function getInstance()
    {
        if (self::$instance == NULL) {
            self::$instance = new static();
        }
        return self::$instance;
    }
}

Maybe this pattern is not as useful as it is in J2EE world. With PHP everything dies within each request, so we cannot persist our instances between requests (without any persistent mechanism such as databases, memcached or external servers). But at least in PHP we can share the same instance, with this pattern, in our script. No matter were we are, we use Foo::getInstance() and we get our instance. It useful when we work, for example, with database connections and service containers.

If we work with TDD we most probably use PHPUnit. Imagine a simple script to test the instantiation of our Foo class

class FooTest extends PHPUnit_Framework_TestCase
{
    public function testGetInstance()
    {
        $this->assertInstanceOf('Foo', Foo::getInstance());
    }
}

All is green. It works.

Imagine now we want to “improve” our Foo class and we want to create a counter:

    public function testCounter()
    {
        $foo = Foo::getInstance();
        $this->assertEquals(0, $foo->getCounter());
        $foo->incrementCounter();
        $this->assertEquals(1, $foo->getCounter());
    }

Now our test fails, So we change our implementation of the class to pass the test and we add to Foo class

    private $counter = 0;

    public function getCounter()
    {
        return $this->counter;
    }

    public function incrementCounter()
    {
        $this->counter++;
    }

It’s green again. We are using a singleton pattern to get the instance of the class and we are using TDD, so what’s the problem? I will show you with an example. Imagine we need to improve our Foo class and we want a getter to say if counter is even or not. We add a new test:

    public function testIsOdd()
    {
        $foo = Foo::getInstance();
        $this->assertEquals(FALSE, $foo->counterIsOdd(), '0 is even');
        $foo->incrementCounter();
        $this->assertEquals(TRUE, $foo->counterIsOdd(), '1 is uneven');
    }

And we implement the code to pass the test again:

    public function counterIsOdd()
    {
        return $this->counter % 2 == 0 ? FALSE : TRUE;
    }

But it doesn’t work. Our test is red now. Why? The answer is simple. We are sharing the same instance of Foo class in both tests, so in the first one our counter start with 0 (the default value) but with the second one it start with 1, because the first test has changed the state of the instance (and we are reusing it).

This is a trivial example, but real world problems with this issue are a bit nightmare. Imagine, for example, that we use singleton pattern to get the database connection (a common usage of this pattern). Suddenly we break one test (because we have created a wrong SQL statement). Imagine we have a test suite with 100 assertions. We have broken one assertion (the second one, for example), and our PHPUnit script will say that we have 99 errors (the second one and all the following ones). That happens because our script can’t reuse the database connection (it’s broken in the second test). Too much garbage in the output of the PHPUnit script and that’s not agile.

How to solve it? We have two possible solutions. The first one is avoid singletons as a plague. We have alternatives such as dependency injection, but if we really want to use them (they are not forbidden, indeed) we need to remember to write one static function in our class to reset all our static states and call after each test. PHPUnit allow to execute something at the end of each test with tearDown() function. So we add to our test:

class FooTest extends PHPUnit_Framework_TestCase
{
...
    public function tearDown()
    {
        Foo::tearDown();
    }
}

And now we implement the tearDown function in our Foo Class

    public static function tearDown()
    {
        static::$instance = NULL;
    }

And all works again. All is green and green is cool, isn’t it? :)

Note: Take into account that “problem” is between quotes in the title. That’s beacause singleton is not a “real” problem. We can use it, it isn’t forbiden, but we need to realize the collateral effects that its usage can throw to our code.

About these ads

About Gonzalo Ayuso

Web Architect specialized in Open Source technologies. PHP, Python, JQuery, Dojo, PostgreSQL, CouchDB and node.js but always learning.

Posted on September 24, 2012, in php, PHPUnit, TDD, Technology and tagged , , . Bookmark the permalink. 13 Comments.

  1. some ppl get really religious about singletons or any static methods and don’t use it at all. Hope they read your post

    • Static states are one kind of magic, and I don’t like magic. I know there’s a lot religious discussions about it. I was a big fan of singletons but after Fabien Potencier’s speach about OO and design patters, I realize the problems with static states. The usage of TDD enlarges the problem. Whitout using TDD those can of problems exists, but they are “hide” (but waiting to hit our face as soon as they can:) )

  2. So, the class Foo is made responsible of having a method which is totally unrelated to its function and only used in testing?

    Not only that but, as Foo::tearDown is public, any other code can (for unknown reasons) do a

    // We need a fresh instance
    Foo::tearDown();
    Foo::getInstance();

    Kind of… meh, isn’t it?

    • Yes and no. Let me explain it a little bit:

      The method tearDown can be private. It’s possible to test private methods with a bit of reflection magic, but the problem will be the same. We are adding one method only for testing purposes and it smells. TDD is a tool that helps us in development process, but if it adds “extra” problems, is it good?. I asked myself this question months ago and finally I realized that those kind of things are not “extra” problems because of TDD. They are “real” problems. OK maybe they are hard to see if we don’t use TDD, but they exists. Static states can be helpful, but they also can have side effects difficult to see. They are “magic” and even if we create a new instance of one object we must ensure what are the states of static properties.

      In my oppinion:

      • tearDown() must be public. If developer need, he can use it. But I undestand your point of view
      • We must try to avoid static states. Static functions are good as factories but static states adds one kind of “magic”, and I don’t like magic. With PHP we have the advantage that everything dies within each request, so we reset all at the end of the script (not the same in J2EE applications)
      • I understand your point of view, but really, Foo::tearDown break “the singleton-icity” of Foo. If you are just relaying on the good behavior of other programmers not to call tearDown, you may as well just ask them to pretty please not instantiate new Foo’s and simply not even bother making Foo::__construct private.

        I’m sorry but I don’t really see the point.

      • Agree with you, but the only way of testing properly singletons is with Foo::tearDown(). We can do tearDown() private or even we can reset static states with reflection in the test (without the funcition), but we need to do it (in one way or another). I’m agree with you. Public __construct and singletons together are not good friends. If we are strict we need to choose between them. singletons

        But we also can choose “avoid singletons as a plague”. I’m not 100% convinced about that (but I’m close :))

  3. Michael R├╝fenacht

    `return $this->counter % 2 == 0 ? FALSE : TRUE;` classical code smell ( `return $this->counter % 2 != 0;` )
    I tend to say that sometimes its even necessary to use singletons to make code testable, especially when the code is heavily function based (see wordpress).

    • I use singletons too :), but IMHO if we can avoid them, better. If we really need them perfect. The problem is when we use them without knowing the side effects (I use to do it in the past).

      By the other hand I agree with you. $this->counter % 2 != 0 is better. Avoid problems (such as swap FALSE and TRUE). Maybe we also can enclose it into a function to make it more readable.

  4. Thanks for sharing your insights Gonzalo! We were happy to include this article into a digest of the most interesting PHP news and updates: http://www.zfort.com/blog/php-digest-september-29-zfort-group/

  5. it is an old post, but anyway if somebody comes here from google for a reference:

    public function tearDown()
    {
    $refProperty = new ReflectionProperty(‘Foo’, ‘instance’);
    $refProperty->setAccessible(true);
    $refProperty->setValue(Foo::getInstance(), null);
    }

  6. Sorry, above comment can be removed (it is not working).

  1. Pingback: Links of the week 30.09.2012 « chombium's blog

  2. Pingback: PHP Digest from Zfort Group | Zfort Group Blog

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 972 other followers

%d bloggers like this: