Legacy Refactor Question: What About Singletons?

In my talk and my book on refactoring legacy PHP code, one of the early steps is to start removing globals and replace them with dependency injection (not a container, just injecting dependencies by hand.) I addressed the use of the global keyword and the $_GET (et al) superglobals, but I did not address singletons directly. I had a Reddit correspondent ask about singletons recently, and how to refactor away from them; I answered on Reddit, where you can read the full conversation.

Are you stuck with a legacy PHP application? You should buy my book because it gives you a step-by-step guide to improving your codebase, all while keeping it running the whole time.

7 thoughts on “Legacy Refactor Question: What About Singletons?

  1. The reddit answer seems to be gone.

    I bought the book; it has helped a lot! My codebase has five singletons all of which use superglobals. I would love to know how you suggested refactoring away from them.

    Can this answer be added to the electronic version of the book?

    • Hey Rob — I’m so glad the book has helped! I’ll add the Reddit answer to the post later, and delete this comment thereafter. In the mean time …


      In my talk and my book on refactoring legacy PHP code, one of the early steps is to start removing globals and replace them with dependency injection (not a container, just injecting dependencies by hand.) I addressed the use of the global keyword and the $_GET (et al) superglobals, but I did not address singletons directly.

      My correspondent asks, regarding a legacy project:

      I have guided my junior developers via code review to “inject your database connection into your consuming classes”. Roughly moving away from …

        class Foo
        {
            public function __construct()
            {
                 $this->db = /* database connection, perhaps using constants
                declared within this class */
            }
        }
      

      … to:

        class Foo
        {
            public function __construct($db)
            {
                $this->db = $db;
            }
        }
      

      … in the name of testability. No container is being used here, so we’re just talking manual constructor injection without any resolution out of a DI container or Service Locator.

      So today, a junior developer tried to do the above, except in the invocation code I saw this:

        $db = Database::singleton();
        $foo = new Foo($db);
      

      To me, this doesn’t make it any more testable. It doesn’t give you any confidence that a mocked or stubbed database object when Foo is the system under test is going to be representative of real world behavior because the singleton is susceptible to action at a distance.

      On a legacy application where we have to deal such horrors, is it better to inject singletons? Or should we just accept that a singleton is global state and carry on using the singleton inside our classes?

      Even though we are using a singleton for the injection, the fact that we are injecting it makes Foo much more testable. (The script using Foo and the singleton Database remain as testable/untestable as they were before.)

      How can it be that Foo is more testable, when a singleton is being injected? Think of it like this: if we were to create a new Database instance instead of the singleton, would Foo be more testable or less than when using a singleton?

      The answer is that, because $db is now injected, Foo does not know or care how $db is created. This makes it more testable. As long as Foo gets a $db object, it will work just fine.

      In the test for Foo, we can create any Database instance we want and inject it into Foo. This has the effect of decoupling the Foo class from the singleton, and thereby making Foo more testable. You’re still stuck with a shared instance of $db in production, but then, you would have a shared instance even if that instance was managed by a container instead of a singleton.

      So yes, it is much better to use dependency injection, even if the injection is created via singleton.

      Does this mean we should use singletons? Not if we can avoid them. But some legacy codebases make wide use of them. Taking the baby-step of injecting a singleton instance, rather than calling the singleton from inside a class, will make it much easier to test the class using the injection. As an added benefit, converting all singleton calls to dependency injection will make it much easier to refactor to using to a DI container later on.

      • Sorry my message was sent too soon. I have singleton Foo that requires singleton Bar. I have decided to send singleton Bar into the function, so basically:

        Foo::getInstance()->myFunction(\Bar::getInstance());

        Does that seem reasonable for now before I start Chapter 6 “Replace new With Dependency Injection” ?

Leave a Reply

Your email address will not be published. Required fields are marked *