The Danger of Copy/Paste Echo Chambers

Echo Chambers as a Function of Well Intended Ignorance

“Echo chambers” are an oft-encountered downfall of developers when learning new skills or implementing unfamiliar systems. What begins as a simple question leads to a collection of up-voted knowledge perfectly ready to be copied into your project — a great thing for productivity (if you don’t mind getting it wrong).

I recently wrote an article discussing some of the open security risks facing new installs of the Apache HTTP Server. One thing I purposefully left out was condemning the usage of ETags, which you can find on plenty of other recent guides for securing Apache. Why?

Because ETags haven’t been a problem for Apache since versions 1.3.22 through 1.3.27 and only affected BSD. The underlying problem with Inode leakage was resolved through a fundamental change to the way the algorithm was implemented years and years ago.

And yet plenty of modern guides still recommend turning off ETags or removing Inode from the calculation method. Not because they’re right, but because that memetic morsel of “ETags are bad” has entered into a spiraling “echo chamber” of security group-think.

Let’s Pick on PHP (It’s So Easy)

With that example of echo chambers in mind let’s follow the path of a developer deciding to implement the Singleton pattern in PHP.

First, let’s put ourselves in the mindset of a developer wanting to write a Singleton: this is someone who either doesn’t understand the anti-pattern nature of Singletons, or understands it enough to still have valid use cases.

Perhaps we’re thinking like an inexperienced PHP developer, or maybe even an otherwise experienced systems programmers who needs to make a quick web project.

For our example we’ll say that we need a Singleton for a PDO Database handler and another one for a Configuration file, we’d prefer that we have an extendable class we could use for both.

That in mind let’s Google it:

Google ‘singleton in php’

Awesome! Plenty of links, surely the right answer is in here somewhere.

PHP Singleton: The Right Way?

Let’s visit the top result: Design Patterns — PHP: The Right Way

Immediately we’re presented with a very clean looking implementation, including some nicely formatted PHPDoc comments.

A Beautiful Looking Singleton Implementation

Scrolling down the author’s page we find an excellent description of the Singleton class implementation, including justification for tying off __clone() , __wakeup() , and __construct() .

It even specifically calls out the ability to inherit this Singleton and have a child class: perfect for our use case of needing child implementations for a Database and Config class.

Here’s a quick implementation copy/pasted from the example above, stripped of comments for readability.

class Singleton

{

private static $instance; public static function getInstance()

{

if (null === static::$instance) {

static::$instance = new static();

} return static::$instance;

} protected function __construct() {}

protected function __clone() {}

protected function __wakeup() {}

}

Now I just need some child classes for my Database and Config handlers:

class Config extends Singleton

{

public $config; public function setConfig($json)

{

$this->config = json_decode($json);

}

} class Database extends Singleton

{

public $handler; protected function __construct()

{

$config = Config::getInstance();

$this->handler = new PDO($config->dsn,

$config->user,

$config->pass);

}

} /* Some dirty tests */ // Fill the config with some JSON

$config = Config::getInstance();

$config->setConfig(

'{"dsn":"mysql:host=localhost;dbname=test","user":"test","pass":"test"}'

); // Init the db connection (which depends on the Config singleton)

$db = Database::getInstance(); // Query a made up table

$sth = $db->handler->prepare("SELECT * FROM testtable");

$sth->execute();



Note: the above classes are all in a single file for the sake of this demonstration, including the Singleton class definition itself.

OK! Now let’s just run php test.php and we’ll see that… oh.

PHP Fatal error: Cannot access property Config::$instance in /home/ec2-user/test/test.php on line 8

Line 8?

The compiler very unhelpfully talks about Config::$instance but the error itself is way back up in the Singleton class definition, not in the Config class definition! Not exactly an easy error to interpret for junior developers.

It does, of course, turn out that the static variable $instance is set to private despite the future code example from the author explicitly showing off its ability to be inherited — so much for thinking about the children.

Changing it to protected solves this initial issue.

Overall a great example of SEO optimized content, but not a great example of correct, usable programming knowledge.

Not Trying to be Rude

Now, before I go any further, I’m not purposefully picking on the “PHP: The Right Way” project or the author themselves: the takeaway here is that this kind of simple mistake from authoritative sources reinforces itself in an echo chamber when others, often meaning well, copy/paste these snippets as the gospel truth rather than trying them out for themselves.

If this particular example had not been a generic Singleton class meant to be inherited (such as a User class built as a Singleton, rather than extending a Singleton class) then the usage of private would have been perfectly valid.

On the other hand, a cursory Google search (after the fact) reveals that I’m not the only one who’s ever noticed this. There’s a detailed bug report (#585) from all the way back in 2015 that still awaits resolution on GitHub for PHP The Right Way’s problems with Singletons.

But Wait, There’s More!

Let’s take a tour of the top Stack Overflow answer for this same question, and let’s keep the “extendable” Singleton class in mind.

It looks like a lot of people have a lot of thoughts to share. At the time of writing the accepted answer has 168 votes, with at least two other posts pointing out its flaws.

An excellent point. If we’re using a single variable to store the Singleton instance then a second subclass will point to the same resource as the first subclass.

Just to prove it, let’s test our previous code:

PHP Notice: Undefined property: Config:$handler in /home/ec2-user/test/test.php on line 48 PHP Fatal error: Call to a member function prepare() on a non-object in /home/ec2-user/test/test.php on line 48

Sure enough, line 48 is actually $sth = $db->handler->prepare("SELECT * FROM testtable"); so we’re definitely using the same resource for the Database subclass as Config .

Singletons with Multiple Subclasses

The solution is deceptively simple: use an array for instances of classes (one instance per class).

Here’s a PHP5.3 version that handles our requirements:

abstract class Singleton

{

private static $instances = array(); protected function __construct() {} public static function getInstance()

{

$class = get_called_class(); if (!isset(self::$instances[$class])) {

self::$instances[$class] = new static();

} return self::$instances[$class];

} private function __clone() {}

private function __wakeup() {}

}

A quick breakdown:

abstract because you should never need to instantiate the Singleton parent class itself

because you should never need to instantiate the Singleton parent class itself private accessor for $instances : a complete reversal of the earlier need for protected in the single instance variable subclassing case

accessor for : a complete reversal of the earlier need for in the single instance variable subclassing case $instances as an array to store multiple subclasses (key is class name)

as an to store multiple subclasses (key is class name) protected function __construct() so you can provide a custom constructor (to initialize data, such as a database handler) if you need to

so you can provide a custom constructor (to initialize data, such as a database handler) if you need to $class as the key for the $instances array: this could be replaced by an inline static::class in PHP7

as the key for the array: this could be replaced by an inline in PHP7 __clone() and __wakeup() are tied down and unusable

Breaking the Echo Chamber

It’s at times like these that the weakness of tools like Stack Overflow and Google show themselves: chiefly that top results are often the most out-dated, and sometimes contain incomplete and factually incorrect information.

What can we do about it? Some thoughts:

Contact the author of the Stack Overflow post who voted for the top (incomplete) answer, and ask them to reconsider: http://meta.stackexchange.com/questions/93969/is-changing-the-accepted-answer-frowned-upon Use our Stack Overflow accounts to raise the vote count of the more useful and correct answer: http://stackoverflow.com/questions/203336/creating-the-singleton-design-pattern-in-php5#answer-15870364 Report the issue to the author of PHP: The Right Way for correction due to highly placed search results landing on a wrong answer. As noted above, there’s already a GitHub issue submitted for this that has gone unimplemented. Supply a canonical example in source code that’s properly documented with a clear use case.

Ergo, there’s no easy answer. Google and Stack Overflow aren’t really built for deciding on canonical, vetted answers.

If a Google search engineer is out there and listening, though, I certainly have some thoughts on how we can improve results for programmers!