Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

An implementation of the singleton pattern in PHP using a trait (added in 5.4). Is there anything missing, any ways to create a second copy of the class?

trait Singleton {
    static function instance() {
        static $instance;
        if ( is_null( $instance ) ) {
            $instance = new static;
        }
        return $instance;
    }
}

A class can the use the trait with the following:

class P {
    use Singleton;
    protected function __construct() {
        echo 'Hello, parent!' . PHP_EOL;
    }
    final private function __clone() {}
}

class C extends P {
    protected function __construct() {
        echo 'Hello, child!' . PHP_EOL;
    }
}

echo get_class( P::instance() ) . PHP_EOL;
echo get_class( C::instance() ) . PHP_EOL;

Output:

Hello, parent!
P
Hello, child!
C
share|improve this question

2 Answers 2

up vote 1 down vote accepted

Contract Enforcement

There are a few ways to violate the singleton contract, but none need to be guarded against.

  1. Create a subclass with a public constructor. This obviously is pilot error.
  2. Use reflection to either make the constructor accessible (5.3.2) or instantiate the class without calling the constructor (5.5?).
  3. Serialization. I think private __sleep and __wake methods would block this.

However, these are active attacks, and the contract should only concern itself with preventing mistakes. So in that regard this looks sufficient.

Configurability

You may want to consider accepting arguments and passing them to the constructor (if this is even possible using new static).

For example, a database connection could accept the host, port, user, and password, allowing reuse in many applications. Otherwise each program must subclass the connection to supply its connection info.

Again, I don't know if this is even possible. I think the only way to pass an array of parameters to a constructor is via reflection, and I don't know if you can get the correct subclass name staticly.

Minor Tip

Since $instance is either null (falsey) or a valid instance (truthy), you can simplify

if (is_null($instance)) . . .

to

if (!$instance) . . .

Testability

This implementation will make testing difficult. Moving the static local variable to a private static property would allow clearing it using reflection in a unit test.

share|improve this answer

From the get-go, let's agree that static and the Singleton pattern are quite pointless in PHP. By design, PHP is a stateless language. The state of an object is not retained in between requests. Hence, static is often referred to as mere globals in OO-drag.
I would like to stress that, in PHP, Singletons are as pointless as a broken pencil.

Be that as at it may, a Singleton trait is an interesting thought. It's a good way to visualize, test and eventually understand inheritance precedence, when traits are involved. And it can also show how magic constants and certain keywords behave unexpectedly.
But it's nothing more than that: an academic example, an exercise and... an example of bad practices.

Don't abuse traits

What you do is very bad practice indeed. You take a concept, meant to do one thing, and abuse it to do something entirely different. You're using a trait, not to get around the (alleged) limitations of single inheritance to mimic multiple inheritance.

This is not what a trait is for

Ask yourself: "What is the point of a trait?", why were the introduced? They were created to solve (IMHO imagined) problems, posed by single inheritance (which actually is a good thing). Traits are an attempt to offer the benefits of multiple inheritance without all of the headaches. A trait is a unit of code that extends the functionality of a class. You are using a trait to dictate the behaviour and usage of a class.

Consider the syntax:

class Bar extends Foo{}

Ok, a class that builds on a fully-fledged (stand-alone) class. The extends keyword makes sense.
Now:

class Something implements Iterator{}

A class, that implements an interface. The interface dictates (part of) the behaviour and can be used to enforce a contract. A class that implements an interface has to conform. Just like JavaScript is an implementation of the ECMAScript standard, and compilers ship with an implementation of the C/C++ standard, so must this class be an implementation of the interface's contract.
Again, the keyword implements covers the load.

Look at the syntax for traits, though:

Class Something extends Base implements Iterator
{
    use DBGetters;
}

The use statement is part of the class, it does not belong to the class definition. A trait, then, has no business enforcing a contract in the way an (abstract) parent class does. Nor should it dictate the behaviour of a class, like an interface does.

Everything you have (Classes, interfaces and traits) are meant to address a specific issue you might encounter.
A trait adds functionality. It does not change the way a class functions.

Still, some core-review:

The issues up front:

  • try to adhere to the coding standards as much as possibel
  • always specify an access modifier, don't rely on the implicit public. Someone might come across some code, and treat the lack of an access modifier as an error, and make it protected.
  • static methods begin with an UpperCase
  • constructors take arguments
  • To ensure a method is not overwritten in child classes, use the final keyword
  • Initializing variables is a good habit. Though not required in PHP (default value is null), it's a habit that makes the transition to other languages easier/smoother. Initialize to null explicitly

Let's put all this to practice when writing your trait.

trait Singleton
{
    final public static function GetInstance(array $args = null)
    {
        static $instance = null;
        if ( is_null( $instance ) ) {
            $instance = new static($args);
        }
        return $instance;
    }
}

That, to my eye, looks a lot cleaner, but there are problems. With this code, your trait is implicitly enforcing a contract: the __construct methods will now be passed either null or an array, and so all constructors must be defined to conform to this contract.
The static variable that retains the instance is a static variable is also local to the method's scope. I know why you do this, but there are implications, and alternatives. One such implication is (owing to PHP's memory management system, which relies on simple ref-counts), that the instance will not be eligible for garbage collection (most static's aren't, for the same reason).
However, a regular singleton class, in PHP does allow for its ref-count to be set to 0, and can be garbage collected (in a hacky way):

class GCSingleton
{
    private static $Instance = null;
    //private constructor and such
    final public static function GetInstance()
    {
        if (self::$Instance === null)//or static::$Instance
            self::$Instance = new static;
        return self::$Instance;
    }
    public function selfDestruct()
    {
        self::$Instance = null;
        return null;
    }
}
$singleton = GCSingleton::GetInstance();
$singleton = $singleton->selfDestruct();

This only works, though, if no other variable references the instance in question, but no matter.

Of course, I completely understand why you did not create a property to which you assign the instance. A static instance is, after all, shared, so using a static property makes inheritance impossible, right?
Not quite: enter inheritance precedence. You can override a static property in a child class, with the same (or less) restrictions:

trait Test
{
    private static $Inst = null;
    final public static function GetInstance()
    {
        if (static::$Inst === null)
            static::$Inst = new static;
        return static::$Inst;
    }
}
class Foo
{
    use Test;
    public function __construct()
    {
        echo 'Created new instance of ', __CLASS__, PHP_EOL;
    }
}
class Bar extends Foo
{
    protected static $Inst = null;
    public function __construct()
    {
        echo 'Created instance of ', __CLASS__, PHP_EOL;
    }
}
$foo = Foo::GetInstance();//echoes Created instance of Foo
$bar = Bar::GetInstance();//echoes Created instance of Bar

If, however, you change the use of the keyword static to self, you loose the power of late static bindings, and Bar::GetInstance returns the instance of Foo, assigned to the private property of the trait.

Ok, now that's some under-the-hood info of how the properties are being resolved, and how the resources are managed. Let's move on to some actual code-review:

Omissions

The whole point of singletons is that they are there to make sure you never create more than one single instance of a given class. You are aware of the __clone magic method, but you're missing the __sleep and __wakeup, or even the __toString methods.
The best way to address this issue, IMO, is to define all of them (as final) in the trait, and instead of letting them fail silently, have them throw exceptions. The moment someone writes something like:

$copy = clone $singleton;

An exception should be thrown. You're just assigning null to $copy, which won't produce errors until someone uses this $copy variable as an object. This can lead to hours, wasted on debugging code, looking for that statement that reassigns $copy, that just isn't there. Instead, write something like:

final public function __clone()
{
    throw new LogicException(
        sprintf(
            'Error: cloning of %s instances is not alllowed',
             get_class($this)
        )
    );
}

Do this for all magic methods.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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