inifinite loop with singleton and PDO

Question

I created a singleton pattern for PDO, the problem is that when I use it, I have an infinite loop, for example, making this:

$db=Db::fetch_instance(); 
$product = $db->query(<query>);

I have this error:

Fatal error: Maximum function nesting level of '100' reached, aborting!

My class is this one:

class Db {

    //START OF SINGLETON PATTERN

    private static $PDOInstance;

    public static function fetch_instance() {
        try {           
            if(empty(self::$PDOInstance)){                      
                self::$PDOInstance = new Db();                  
            }
            return self::$PDOInstance;
        } catch (Exception $e) {
                     <something>
        }
    }

    private function __construct() {
            return new PDO("something");
    }

    //START OF DECORATOR PATTERN

    public function beginTransaction() {
        .......
    }

    public function query($statement) {
        return self::$PDOInstance->query($statement);
    }

      ........

Why this loop?, I dont see any loop.

Shouldn't the `return self::$PDOInstance;` be outside the if statement. — Musa, Apr 03 '13 at 21:45
yes!, I saw, I fixed it, but continue the loop. I updated the code with your hint — francis, Apr 03 '13 at 21:47
__construct() method can't have return value as far as I know? — , Apr 03 '13 at 21:51
Please, stop using that antipattern. You might find this somewhat relevant: http://stackoverflow.com/a/11369679/727208 — tereško, Apr 04 '13 at 05:58

Alexandre Jacob · Accepted Answer · 2013-04-03T22:07:05.407

The error is pretty clear to me, your method query is calling itself.

public function query($statement) {
    return self::$PDOInstance->query($statement);
}

This is because you have a mistake in your singleton, you should change the code to :

public static function fetch_instance() {
    try {           
        if(empty(self::$PDOInstance)){                      
            self::$PDOInstance = new PDO("something");
            return self::$PDOInstance;
        }
    } catch (Exception $e) {
                 <something>
    }
}

private function __construct() {

}

Be careful, constructors are not meant to return anything, you were assigning self::$PDOInstance to an instance of a new Db object!

UPDATE

By the way here is something closer to what you want to do :

class Db {

    // START OF SINGLETON PATTERN

    private static $instance;

    private $PDOInstance;

    public static function fetch_instance() {
        try {
            if (empty(self::$PDOInstance)) {                      
                self::$instance = new Db();
            }

            return self::$instance;
        } catch (Exception $e) {
            // <something>
        }
    }

    private function __construct() {
        return $this->PDOInstance = new PDO("something");
    }

    // START OF DECORATOR PATTERN

    public function beginTransaction() {
        // .......
    }

    public function query($statement) {
        return $this->PDOInstance->query($statement);
    }

}

$PDOInstance is private and bound to your singleton, $instance is static and will contain the unique instance of the Db class. Your constructor is here in charge of initializing your $PDOInstance you can then use it as an attribute with $this->PDOInstance

THank you!, I don't know why but with your code I saw it very clear. THanks. — francis, Apr 03 '13 at 21:55

score 2 · Answer 2 · answered Apr 03 '13 at 21:54

2

__construct() shouldn't have return value.

Remove it and make your fetch_instance() like this:

public static function fetch_instance()
{
    if (!self::$PDOInstance) {
        self::$PDOInstance = new PDO();
    }

    return self::$PDOInstance;
}

answered Apr 03 '13 at 21:54

1

Indeed but it made no sense since constructor always returns class instance anyway. – Apr 03 '13 at 21:59

score 0 · Answer 3 · answered Apr 03 '13 at 22:06

Your singleton pattern is wrong. What you should do if you want instances of the class "Db" to be singleton is to store the one and only instance of "Db" inside the Db::fetch_instance() into a static private property. This instance should then have an instance of PDO.

class Db {

//START OF SINGLETON PATTERN

private static $dbInstance;

private $PDO;

public static function fetch_instance() {
    try {           
        if(empty(self::$dbInstance)){                      
            self::$dbInstance = new Db();                  
        }
        return self::$dbInstance;
    } catch (Exception $e) {
                 <something>
    }
}

private function __construct() {
        $this->PDO = new PDO("something");
}

//START OF DECORATOR PATTERN

public function beginTransaction() {
    .......
}

public function query($statement) {
    return $this->PDO->query($statement);
}

  ........

Note that there really is no need to use singletons.

On a general level, singletons are more and more considered an antipattern. They do make code badly testable and should be avoided.

Second: There is always more than one instance in PHP because every request runs inside it's own variable space. So there is one instance per concurrent request, not one instance per server. Which means that all these instances are connected to the database simultaneously.

Third: The mysql extensions in PHP prevent programs from connecting multiple times to the same database with the same credentials.

Fourth: Although there might be no current need, there will be a time where there is more than one database at the same time. This is the time where the creation of the database connection as a singleton will hurt the most.

So I would really recommend to not use the singleton pattern for database connections. You gain nothing, but lose very much.

And while we are at it: The decorator pattern wraps around an instance of an object of my choice. Your implementation does not allow to choose. It is no decorator. — Sven, Apr 03 '13 at 22:08

inifinite loop with singleton and PDO

3 Answers3