PHP MASTER: singleton, trait and registry examples throw fatal error

[soupytwist]

I've been working my way through the PHP MASTER sitepoint book and have found an issue that affects the singleton, trait and registry sections of the design patterns chapter of the book.

class Database extends PDO
{
    private static $_instance = null;
  
    private function __construct()
    {
        parent::__construct(APP_DB_DSN, APP_DB_USER, APP_DB_PASSWORD);
    }
    ...
} 

This throws a fatal error: Fatal error: Access level to Database::__construct() must be public (as in class PDO)

I'm ok with the principle that the lesson is teaching, but I'm just curious as to what others do to create a db/model class. Is it better to create the PDO object and pass it into the database object, assigning it to a private property for use?

[TomB]

Singletons are considered bad practice. The reason for the error is that you're trying to change the visibility of the constructor as it is defined in PDO.

There are several options. If you do need to extend the functionality of PDO, do that and pass your database object around. If you don't, just pass PDO around. While singletons, registries and service locators work, they go against several good practice design considerations.

When it comes to dependencies, the general consensus is that the best method is passing dependencies into the constructor and an instance of the exact class that's required rather than a service locator/registry (see Flaw: Digging into Collaborators)

[soupytwist]

Cheers Tom,

The reason for the error is that you're trying to change the visibility of the constructor as it is defined in PDO.

Yeah, I was a bit surprised that Sitepoint would publish a book with such an error, considering the error affects numerous pages and code examples.

Thanks for the answer and links, which I'll have a read through.

[ServerStorm]

Hi,

Below are some of the simpler possibilities. You will find those in the OOP community that don't like Static functions and the registry approach. It is Singleton like and that smells bad to many. You can find lots of 'Why Singletons are Evil' discussions here and via search engines.

Setter Injection seems nice as you don't have to wire up an object in a specific order, but this is also a weakness when Objects get more complicated it can be hard to remember what needs to be wired for each object and also complicates debugging.

Constructor Injection has a benefit in that it naturally enforces wiring up object with the dependencies in the correct order.

There is one further approach DI (Dependency Injection). Most of the time you will not need to use DI; however when you have objects with lots of dependencies then a DI container can be really helpful; this includes database dependencies A DI container is an object that knows how to configure and instantiate your objects. You can look at some of the smaller php containers like Bucket, Pimple or you can write your own. You may also want to read Do you need a Dependency Injection Container? - Fabien Potencier
article.

// Setter Injection
class Something {
  protected $db;
  public function setDb($db){
    $this->db = $db; 
  }
}
$oSomething = new Something;
$oSomething->setDb( new DbClass);

// Constructor Injection 
class Something { 
  function __construct($db){...}
}
$oSomething = new Something(new DbClass);


// Registry Pattern
class Registry {
  static function getDb(){
    return new DbClass;
  }
}

class Something {
  protected $loader;
  function __construct($loader) {
    $this->loader = $loader;
  }
  function doSomething {
    $db = $this->loader->getDb();
    ...
  }
} 

Steve

[TomB]

Setter Injection seems nice as you don't have to wire up an object in a specific order, but this is also a weakness when Objects get more complicated it can be hard to remember what need to be wired for each object and also complicates debugging.

I'd just like to expand on this a little. From an OOP purist perspective, setter injection is a bad idea. In OOP an object, once it's constructed, should be able and ready to fulfill its responsibilities. Setter injection means an object can exist in an intermediate state where it cannot and it's methods must be called in a specific order. This goes against the principle of encapsulation because the client code must be aware of the implementation details (That it's not fully constructed after being constructed!) of the object in question. There is a very good read here: http://misko.hevery.com/code-reviewe...oes-real-work/ for more information on this topic.

edit: To demonstrate this:

$user = new User;
$user->setDb($db);
$user->findById(123); 

If setDb() is not called then findById() simply will not work.

In complex systems, it's more than likely the $user object will be passed around. The code using the $user object throughout the system will not know whether its dependencies have been correctly set. By using constructor injection this ambiguity is removed. Any variable that is an instance of "User" will be complete and can be guaranteed to be able to fulfill its responsibility.

[Lemon Juice]

Steve, is this a typo in your setter injection?

$oSomething->db = new DbClass; 

You can't do this because you declared db protected.

[Jeff Mott]

From an OOP purist perspective, setter injection is a bad idea.

From an OOP pragmatist perspective, constructor injection is best for required dependencies, and setter injection is best for optional dependencies.

[TomB]

What would you describe as an "optional dependency"? It's obviously a requirement for some functionality within the object. If it's only needed by one method it should be passed to the method not a setter. Do you have any examples of what you mean by "optional"?

[cpradio]

Singletons are considered bad practice. The reason for the error is that you're trying to change the visibility of the constructor as it is defined in PDO.

Maybe there is a better technique, but I've always used singleton patterns when I need to rely on a process that isn't thread safe. For example, maybe you store documents in a third party storage system, but the driver is flaky at best and really only runs smoothly if a single thread/process is ever created. A singleton makes it easy to control that one thread/process is started and all requests honor that. Yes, you can definitely cause slow downs with this, but it does help.

Can it be done without a singleton pattern, probably, but is usually much more complex.

[TomB]

There's always edge cases with everything, but the simplest way to do that would be a simple static property in the class which stored whether a thread was running or not. It's not good practice but it's a workaround for something that already isn't working as it should be! The more robust the foundation's you're building on and the more robust your code can be.

The problem you have with the singleton in that scenario is that it's then ingrained in your code. What if someone releases another driver that works better and is threadsafe? Or the driver you had gets updated to do so. If you used a singleton, you're stuck with it. If you'd delegated object creation to a factory and imposed the "Only one instance" limit there, you can adjust your factory to allow multiple instances and it'll just work. If you used a singleton, you need to find every piece of code which has called it and update that.

Singletons always limit flexibility and make code very difficult to test. I can't think of any real examples where they are the best choice.

[cpradio]

In my particular situation, that driver was never updated for well over the 6 years I was at that company, so the singleton pattern we implemented worked well. We also developed an API to it, so as long as you utilized the API in your projects you were golden if we ever needed to update it to a different pattern.

[ServerStorm]

Steve, is this a typo in your setter injection?

$oSomething->db = new DbClass; 

You can't do this because you declared db protected.

Yup so I changed it so the setter example now uses a set method.

$oSomething->setDb(new DbClass); 

Steve

[TomB]

In my particular situation, that driver was never updated for well over the 6 years I was at that company, so the singleton pattern we implemented worked well. We also developed an API to it, so as long as you utilized the API in your projects you were golden if we ever needed to update it to a different pattern.

But whatever API the object has, it's accessed using class::getInstance() which is the sole problem with the singleton pattern. It ties your code to a very specific implementation. You can't substitute it for a subclass of class and you can't substitute it in testing without manually creating a mock version of the class. It also creates hidden dependencies in the client code which limits the portability of the client code--you can't copy it to another project without also copying the singleton-- and, worse, it's not immediately obvious that the dependency even exists. Anyone looking through the code will need to read through every line to work out what dependencies there are. With dependency injection they only need to look at the class API, and ideally, only the constructor.

The sole benefit of enforcing a single instance is heavily outweighed by the drawbacks of using static methods and violating the single responsibility principle. When you consider that the same benefit can be achieved in a couple of lines of code (probably fewer than the singleton!) in several different ways, there's no argument for using it. Yes it works, but so would a global variable. It doesn't mean it's a good solution.

[cpradio]

But whatever API the object has, it's accessed using class::getInstance() which is the sole problem with the singleton pattern. It ties your code to a very specific implementation. You can't substitute it for a subclass of class and you can't substitute it in testing without manually creating a mock version of the class. It also creates hidden dependencies in the client code which limits the portability of the client code--you can't copy it to another project without also copying the singleton-- and, worse, it's not immediately obvious that the dependency even exists. Anyone looking through the code will need to read through every line to work out what dependencies there are. With dependency injection they only need to look at the class API, and ideally, only the constructor.

The whole point of wrapping a third-party implementation IS to HIDE it. The project using it shouldn't need to know what is under the hood, they just need to know here is the wrapper/API for you to use, if we need to switch drivers, implementations, use a different third-party, you don't have to do a single thing! I just update the wrapper/API (leaving the methods and arguments in tact) and you are none the wiser that we moved from a Content Manager from IBM to one from Oracle or to a internally built system.

In these situations, if my underlying code would need to reflect a singleton, I usually make that (the singleton) a smaller class, and the wrapper/API would then hide that class. This hides the singleton pattern, so all other code would never know the difference but the limitation is maintained to only permit a single thread/instance.

The sole benefit of enforcing a single instance is heavily outweighed by the drawbacks of using static methods and violating the single responsibility principle. When you consider that the same benefit can be achieved in a couple of lines of code (probably fewer than the singleton!) in several different ways, there's no argument for using it. Yes it works, but so would a global variable. It doesn't mean it's a good solution.

I could never agree to that, it is my firm belief that there are situations where these patterns are absolutely a necessity, otherwise their existence wouldn't be needed. Static methods exist to serve a purpose, it is up to the developer to ensure the purpose is met when using it and understanding what the intent is of that ability. I'll never be a person to say "there's no argument for using it". I guarantee there is, the question is, does your situation merit the use of it.

I've seen plenty of times where someone is building a utility class and are not making their methods static even though it serves no purpose as an instantiated class. It doesn't have internal properties, it doesn't need to track any information, each method is self contained and not reliant on other properties. They do it because some one told them static is bad. So you see $utility = new Utility(); $utility->GetRootFolder();, instead of Utility::GetRootFolder(); (the latter being faster and consuming less resources).

I believe there are scenarios that make using a singleton pattern as the best option over an instantiated object (especially in other languages where your assembly may be cached in memory to be used by multiple applications/threads in a service type setup -- getting multiple requests simultaneously).

[pbyrne84]

It really comes done to if you want to do any testing. Using static is effectiveley copying and pasting implementation into the current scope and with it the tests for the logic that will need to cover that that bound implementation. Unless you rely on hackery with autoloaders and hard coded mocks included before the tests run to seam in the mock( similar to the process used in the book Working with Legacy Code http://www.informit.com/articles/art...59417&seqNum=3 where including a mock directly to nullify the autoloader is the equivalent of the #ifdef TESTING bit in the preprocessor ).

There is also partial mocking to override tested code by wrapping static calls( including the new keyword ) in internal protected methods which achieves the similar effect to the previous page but partial mocking leads to tests that are less intelligible.

If you don't have to do unit testing and projects never really grow beyond a certain complexity then the benefits of not using any direct statically bound implementation( again using the new keyword in a consumer instead of injection is also statically binding implementation ) it is harder to show business gain as the debate really becomes procedural/non SOLID OOP versus SOLID OOP( particularly Liskov substitution principle/Dependency inversion principle ). Single Responsibility Principle is harder to define and explain concrete reason in discussion as the size/ shape of a responsibility can mean different different things to different people so is a bit more of a zen thing like KISS( simple is different to different people ) and DRY( cognitive recognition of repetition starts at different points with different people ).

What Tom is saying about Singletons is very subtle. The single state is not the problem it is that a class decides for itself that it can be the only one. Separating the singleton creation and management to another class achieves the same result as a singleton and allows injection of a mock into the instance wrapper or allows it to be overriden at the configuration stage. Using dependency containers they can manage all single instances, the consumer does not require any knowledge that they are using a single state object, in fact the symfony service container that sort of design choice is done by configuration.
http://symfony.com/doc/2.0/book/service_container.html

Singletons went from pattern to anti pattern for a reason and that reason was not they always don't work but that they have a much higher troublesome weighting over alternate methodologies. It is based on probability of being a future hassle due to the probability of certain future unknown requirements. It is more akin to betting on horses due to previous form and picking an outside horse.

Things like using static for reason of micro optimisation can cost far more human time than processor time. Human time has to be included in the equation as it can rack up to be far more costly, unfortunately such metrics are hardly analysed at the same micro level as running a profiler can achieve.

[cpradio]

I do agree that testing singleton's is difficult, but if you work with a third party element that really "only" works properly if things are called in specific manners, and it can only be a single instance, it is difficult to utilize anything but a singleton, but I do give you a valid argument there. And to be fair, that is a very valid argument. My tests were terribly ugly and in the end we relied more on the API tests than the singleton class tests strictly because we cared that the API worked properly more than verifying all aspects of the singleton were working (since the API was the only thing being exposed) and the API wasn't a singleton, so we could mock it.

So with that said, I still stand by my statement, that I'll never agree that there is no argument for using a particular language feature, but rather, your chances for the need may be extremely rare or a very very specific use case where other patterns (or anti-patterns) would not be as appropriate.

[TomB]

It really comes done to if you want to do any testing

I'm not sure I agree with that. It's generally the case that unit testing is the first place you will identify problems caused by things like static methods but the flexibility issues caused by hardcoded dependencies and global state often cause problems as a project grows and code is re-used in a manner it may not have originally been intended or with other components it wasn't expecting.

Once you realise that the object can't be re-used without some hacky code because it's tied to global state or you can't use it with a substituted dependency then the problems become painfully apparent.

What Tom is saying about Singletons is very subtle. The single state is not the problem it is that a class decides for itself that it can be the only one. Separating the singleton creation and management to another class achieves the same result as a singleton and allows injection of a mock into the instance wrapper or allows it to be overriden at the configuration stage. Using dependency containers they can manage all single instances, the consumer does not require any knowledge that they are using a single state object, in fact the symfony service container that sort of design choice is done by configuration.

Yep! Thanks, you worded it far more eloquently than I did. I should have made it clearer I was talking about the client code using the singleton that's problematic rather than the singleton itself.

I do agree that testing singleton's is difficult, but if you work with a third party element that really "only" works properly if things are called in specific manners, and it can only be a single instance, it is difficult to utilize anything but a singleton, but I do give you a valid argument there.

There is plenty of discussion around the web about why singletons are bad practice, this isn't a new concept either, that initial post I linked to was from 2004. Bringing edge-cases into a discussion really is a red-herring because even here there's no real reason to use a singleton. The same thing can be easily achieved without littering your codebase with foo::getInstance(). I'm only arguing this point so heavily because people

I've seen plenty of times where someone is building a utility class and are not making their methods static even though it serves no purpose as an instantiated class. It doesn't have internal properties, it doesn't need to track any information, each method is self contained and not reliant on other properties. They do it because some one told them static is bad. So you see $utility = new Utility(); $utility->GetRootFolder();, instead of Utility::GetRootFolder(); (the latter being faster and consuming less resources).

This is a perfect example of a bad example. In reality, $utility; would be injected rather than created. "new" is static, after all. Those two examples are almost the same, as you say, the benefit is minimal because it's still making a static call. $utility cannot be substituted in this code so it's pointless not using static. This highlights the reason object creation should be separate from the utilisation of that object and is not an argument to use static methods. Your example code is tied to a specific implementation of "utility" either way which is far from ideal.

[cpradio]

I really enjoyed the answer here as to when a Singleton "could be" used (granted, I know you still think you should make it an instantiated object, but I digress, there are valid scenarios for using one, if the meet the following criteria)

A Singleton candidate must satisfy three requirements:

• controls concurrent access to a shared resource.
• access to the resource will be requested from multiple, disparate parts of the system.
• there can be only one object.

Source: http://stackoverflow.com/questions/2...-the-singleton

My utility was a bad example. Was just trying to make a quick point, that if your methods are not maintaining any state, the using an instantiated object only benefits integration testing (as you can mock or di it -- I do a lot of this today), but unit testing can be used to verify the returned result is expected (I do this for my statically typed or singletons as integration testing is much harder to accomplish).

But I agree to disagree I haven't found a case for a Singleton in several years, but when the scenario arises that requires the above 3 qualifications, I'd go with a singleton instead of playing around with control via an instantiated object. Primarily because it will be much clearer to the other developers from a readability standpoint; versus having to look in the instantiated object and figure out why the behavior seems to be limited -- which it is uncharacteristic of an instantiated object. But that is okay, I don't mind having differing opinions on the subject especially on a rarely (or what should be rarely) used language part.

[TomB]

I really enjoyed the answer here as to when a Singleton "could be" used (granted, I know you still think you should make it an instantiated object, but I digress, there are valid scenarios for using one, if the meet the following criteria)

Source: http://stackoverflow.com/questions/2...-the-singleton

I still don't agree with that. But I don't think we'll ever agree :P While it's a good definition of where singletons are most worthwhile, it's not at argument for their use over alternatives. Even the logger example in that post is flawed. While it avoids the issue of global state, hidden dependencies are bad however you use them and what if you wanted to enable logging for specific parts of the system and not others? What if you want to log to a database instead of the filesystem?

But I agree to disagree I haven't found a case for a Singleton in several years, but when the scenario arises that requires the above 3 qualifications, I'd go with a singleton instead of playing around with control via an instantiated object. Primarily because it will be much clearer to the other developers from a readability standpoint; versus having to look in the instantiated object and figure out why the behavior seems to be limited -- which it is uncharacteristic of an instantiated object. But that is okay, I don't mind having differing opinions on the subject especially on a rarely (or what should be rarely) used language part.

Yeah, I think that was my biggest issue regarding their use. They're taught as a commonplace pattern so get used very frequently for things they really shouldn't be and their downsides are not taught along with the pattern itself. When you consider the advantages they give are redundant as they can be achieved in better ways, the inclusion of the pattern in any educational material is detrimental to the teaching exercise they're part of.

[cpradio]

Yeah, I think that was my biggest issue regarding their use. They're taught as a commonplace pattern so get used very frequently for things they really shouldn't be and their downsides are not taught along with the pattern itself. When you consider the advantages they give are redundant as they can be achieved in better ways, the inclusion of the pattern in any educational material is detrimental to the teaching exercise they're part of.

Yay! Something we can agree on