Magento makes use of design patterns, or at least an interpretation of design patterns. One particularly pernicious one, is Mage::getSingleton().
A Singleton, if you've not heard the term before, was popularised in the Design Patterns book by the Gang of Four (Erich Gamma, Richard Helm, Ralph Johnson and John Vlissides). To be very succinct, a Singleton is a way to ensure there is only ever one instance of a class in an Object Oriented design. To put it in even simpler terms , it is an Object Oriented version of a global variable.
It's used heavily in Magento (in the app/code/core directory, 2261 times in fact!). But anyway, why is it considered harmful? There are a number of arguments for why and why not. Herb Sutter's Once is not enough gives a pretty good (and fun to read) overview of them, or you can read Kenton Varda who looks in-depth at the topic. I generally think though, that in Object Oriented software, you're seeking to create abstractions around complexity. The Singleton is a (too) convenient escape hatch from encapsulation and can lead to the attendant issues you get with global variables.
In the Magento/PHP land a more implementation specific problem with Singletons, is memory consumption. Today I was revisiting a Magento Promotions extension I had written and trying to figure out why it was suddenly obliterating PHP's memory_limit.
This extension basically piggybacks on the existing Promotions/Coupons system, but generates an index of products that match coupon codes, the price before and after the promotion is applied and some other metadata.
In order to determine what products have a promotion associated with them, I run through all the products, and match SalesRule conditions against them. I create a synthetic quote for the products that match and then pump them through the SalesRule validator. This effectively applies the promotion to the product and let us see what the savings are.
It's fairly basic, it doesn't look at multi product combinations but it works well enough for simple cases.
/**
* Apply pricing rules to a synthetic quote to calculate discounted price
*
* @param string $couponCode
* @param Mage_Catalog_Model_Product $product
* @return float
*/
public function applyToProduct($couponCode, $product)
{
$quote = Mage::getModel('sales/quote');
$item = Mage::getModel('sales/quote_item')
->setQuote($quote)
->setProduct($product)
->setQty(1)
->setBaseDiscountCalculationPrice($product->getPrice())
->setDiscountCalculationPrice($product->getPrice());
$validator = Mage::getSingleton('salesrule/validator')
->init(1, 1, $couponCode);
$validator->process($item);
return $product->getPrice() - $item->getDiscountAmount();
}
Now when I wrote this code, it seemed sensible to use the validator as a Singleton, after all I only needed one copy of it. It didn't, at the time, seem to make sense to create and then destroy the validator a couple of thousand times during indexing. Indeed when this code was first deployed, everything ran smoothly.
Recently the user of this extension added a whole bunch of sales rules - and this caused that product/salesrule index loop to detonate.
That Singleton Validator, which was written as some sort of optimization, started happily hosing over a gig of ram.
Changing getSingleton() to getModel() took ram usage down from 1100MB to about 80MB.
My suspicion is that PHPs garbage collection wasn't cleaning up adequately after each validation attempt. As the validator is effectively static, it never gives up its references for PHP to clean up. When you use getModel(), the validator loses all its references after each loop. While it means it also has to be constructed after each loop, but that allows PHP to free the memory it is using.
The Singleton is already a controversial pattern these days, but Magento developers should be particularly wary of it's implementation and its scope to hose memory.