SitePoint Sponsor

User Tag List

Results 1 to 20 of 20
  1. #1
    SitePoint Member
    Join Date
    Mar 2012
    Posts
    2
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

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

    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.

    PHP Code:
    class Database extends PDO
    {
        private static 
    $_instance null;
      
        private function 
    __construct()
        {
            
    parent::__construct(APP_DB_DSNAPP_DB_USERAPP_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?

  2. #2
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    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)

  3. #3
    SitePoint Member
    Join Date
    Mar 2012
    Posts
    2
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Cheers Tom,

    Quote Originally Posted by TomB View Post
    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.

  4. #4
    Foozle Reducer ServerStorm's Avatar
    Join Date
    Feb 2005
    Location
    Burlington, Canada
    Posts
    2,699
    Mentioned
    89 Post(s)
    Tagged
    2 Thread(s)
    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.


    PHP Code:
    // 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
    Last edited by ServerStorm; Oct 8, 2012 at 12:23. Reason: Fixed protected property on setter example
    ictus==""

  5. #5
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    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:

    PHP Code:
    $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.

  6. #6
    SitePoint Guru bronze trophy
    Join Date
    Dec 2003
    Location
    Poland
    Posts
    925
    Mentioned
    7 Post(s)
    Tagged
    0 Thread(s)
    Steve, is this a typo in your setter injection?
    PHP Code:
    $oSomething->db = new DbClass
    You can't do this because you declared db protected.

  7. #7
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,149
    Mentioned
    14 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by TomB View Post
    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.
    "First make it work. Then make it better."

  8. #8
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    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"?

  9. #9
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    4,812
    Mentioned
    141 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by TomB View Post
    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.
    Be sure to congratulate xMog on earning April's Member of the Month
    Go ahead and blame me, I still won't lose any sleep over it
    My Blog | My Technical Notes

  10. #10
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    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.

  11. #11
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    4,812
    Mentioned
    141 Post(s)
    Tagged
    0 Thread(s)
    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.
    Be sure to congratulate xMog on earning April's Member of the Month
    Go ahead and blame me, I still won't lose any sleep over it
    My Blog | My Technical Notes

  12. #12
    Foozle Reducer ServerStorm's Avatar
    Join Date
    Feb 2005
    Location
    Burlington, Canada
    Posts
    2,699
    Mentioned
    89 Post(s)
    Tagged
    2 Thread(s)
    Quote Originally Posted by Lemon Juice View Post
    Steve, is this a typo in your setter injection?
    PHP Code:
    $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.

    PHP Code:
    $oSomething->setDb(new DbClass); 
    Steve
    ictus==""

  13. #13
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    Quote Originally Posted by cpradio View Post
    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.

  14. #14
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    4,812
    Mentioned
    141 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by TomB View Post
    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).
    Be sure to congratulate xMog on earning April's Member of the Month
    Go ahead and blame me, I still won't lose any sleep over it
    My Blog | My Technical Notes

  15. #15
    SitePoint Member
    Join Date
    Feb 2009
    Posts
    17
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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.

  16. #16
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    4,812
    Mentioned
    141 Post(s)
    Tagged
    0 Thread(s)
    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.
    Be sure to congratulate xMog on earning April's Member of the Month
    Go ahead and blame me, I still won't lose any sleep over it
    My Blog | My Technical Notes

  17. #17
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    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.

  18. #18
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    4,812
    Mentioned
    141 Post(s)
    Tagged
    0 Thread(s)
    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.
    Be sure to congratulate xMog on earning April's Member of the Month
    Go ahead and blame me, I still won't lose any sleep over it
    My Blog | My Technical Notes

  19. #19
    SitePoint Guru bronze trophy TomB's Avatar
    Join Date
    Oct 2005
    Location
    Milton Keynes, UK
    Posts
    988
    Mentioned
    9 Post(s)
    Tagged
    2 Thread(s)
    Quote Originally Posted by cpradio View Post
    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.

  20. #20
    Hosting Team Leader silver trophybronze trophy
    cpradio's Avatar
    Join Date
    Jun 2002
    Location
    Ohio
    Posts
    4,812
    Mentioned
    141 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by TomB View Post
    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
    Be sure to congratulate xMog on earning April's Member of the Month
    Go ahead and blame me, I still won't lose any sleep over it
    My Blog | My Technical Notes


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •