Magento 1: use of observers and missing native core setter?

Question

Yet another best practice question.

So here's the thing, I have several modules that deal with setting different data on the core Magento block.

I don't like to rewrite entire blocks and I tend to use observers intead.

Here's an example:

The goal of the module is to set some rates for the following blocks:

• Mage_Checkout_Block_Onepage_Shipping_Method_Available
• Mage_Checkout_Block_Cart_Shipping

The easiest way of doing it would be to rewrite both blocks to add my logic in the getEstimateRates method but I don't really like that logic so here's what I've done:

Still rewritten the blocks

Just to add the following method that is missing from the original blocks:

public function setEstimateRates($rates)
{
 $this->_rates = $rates;
}

Use observers

Thanks to this new method, I can use an observer on core_layout_block_create_after to update the rates on the fly:

// Get the block
$block = $observer->getBlock();
// Get the OG block rates
$rates = $block->getEstimateRates();
if ()
{
unset($rates[self::$_KEY]);
}
// Set the new rates
$block->setEstimateRates($rates);

A second example of a similar problem I have faced a while ago can be found here: Category Product List : add a custom view mode via observer

So here are my questions:

• is that bad practice to add methods to native classes, especially setters ?
• is it better practice to do what I just did instead of rewriting a core class method ? pros/cons ?

Answer 1

In cases like these where there are no clear cut best solutions it will always be a tradeoff closely linked to what exactly you are trying to achieve.

The feedback that I would give you on your chosen approach is that I don't like the rewrites + still using a broad event core_layout_block_create_after. I would prefer to see a specific event getting dispatched in the rewritten block which you then observe.

The relevant code to retrieving the rates in your blocks is this

Mage_Checkout_Block_Cart_Shipping

public function getEstimateRates()
{
 if (empty($this->_rates)) {
 $groups = $this->getAddress()->getGroupedAllShippingRates();
 $this->_rates = $groups;
 }
 return $this->_rates;
}

and Mage_Checkout_Block_Onepage_Shipping_Method_Available

public function getShippingRates()
{
 if (empty($this->_rates)) {
 $this->getAddress()->collectShippingRates()->save();
 $groups = $this->getAddress()->getGroupedAllShippingRates();

which leads to a better place for you to inject yourself into the process Mage_Sales_Model_Quote_Address::getGroupedAllShippingRates()

I would rewrite the above and then dispatch a custom event:

public function getGroupedAllShippingRates()
{
 $rates = parent::getGroupedAllShippingRates();
 $transport = new Varien_Object();
 $transport->setRates($rates);
 Mage::dispatchEvent(
 'fooman_sales_quote_address_get_grouped_shipping_rates',
 array($this->_eventObject => $this, 'transport' => $transport)
 );
 return $transport->getRates();
}

One thing to aim for when extending to dispatch an event is to call the parent method so that you do not need to copy the core logic (== needing to maintain it).

• Raphael at Digital Pianism: Thanks for the answer, I like the idea of dispatching a custom event.

Answer 2

@Raphael at Digital Pianism

Re: bad practice to add methods to native classes: Yes, it's bad practice to directly add properties/methods to native classes. It's best to just extend the class and add your customizations there.

Re: better practice to do what I just did instead of rewriting a core class method: It's best practice to use observers over class rewrites, as the use of rewrites could potentially lead to conflicts, whereas you won't experience this with observers. Ben Marks would teach this in the Fundamentals of Magento Development class.

• Raphael at Digital Pianism: Thanks for your answer. Unfortunately your statements lead to opposite solution. As I need to add a method (bad practice) to be able to use the observer (good practice), I'm stuck with a dilemma.
• ryanF: @RaphaelatDigitalPianism touche' I was approaching answering your questions separately, not taking them both into account, so my mistake there. If you're able to use an observer to listen to the core_layout_block_create_after event, and use code similar to the sample you showed above, that seems perfectly legit to me. It's a decoupled approach that follows best practices.