Event/observer pattern broken by _beforeSave() method override, in several Magento classes that extend Mage_Core_Model_Abstract

Question

When trying to implement an observer in order to catch 'cataloginventory_stock_item_save_before' events, I've come to realise that the _beforeSave() method in Mage_CatalogInventory_Model_Stock_Item overrides that of Mage_Core_Model_Abstract without calling its parent, thus preventing the system from dispatching the usual 'model_save_before' and $this->_eventPrefix.'_save_before'events...

In Mage_Core_Model_Abstract:

protected function _beforeSave()
{
    //...
    Mage::dispatchEvent('model_save_before', array('object'=>$this));
    Mage::dispatchEvent($this->_eventPrefix.'_save_before', $this->_getEventData());
    return $this;
}

In Mage_CatalogInventory_Model_Stock_Item:

protected function _beforeSave()
{
    //...some other stuff, but no parent::_beforeSave()!
    return $this;
}

Although I'm very new to Magento, this looks weird to me, especially when looking at many other Magento classes which extend Mage_Core_Model_Abstract, override the _beforeSave() method, but do call parent::_beforeSave() (in various ways, as can be seen in the following examples).

For instance, in Mage_Catalog_Model_Product:

protected function _beforeSave()
{
    //...
    parent::_beforeSave();
    //no return in this one!
}

In Mage_Catalog_Model_Product_Compare_Item:

protected function _beforeSave()
{
    parent::_beforeSave();
    //...
    return $this;
}

Or in Mage_Catalog_Model_Abstract:

protected function _beforeSave()
{
    //...
    return parent::_beforeSave();
}

Etc.

Even weirder, Mage_CatalogInventory_Model_Stock_Item also overrides the _afterSave(), but this time does call the parent's method:

In Mage_CatalogInventory_Model_Stock_Item:

protected function _afterSave()
{
    parent::_afterSave();
    //...
    return $this;
}

My question is (intended for Magento gurus out there):
Do you understand whether there is any good reason for this omission of parent::_beforeSave() in Mage_CatalogInventory_Model_Stock_Item?
Or should it be listed as a bug?

The only fix I have thought of, in order to be able to catch 'cataloginventory_stock_item_save_before' events, is to copy the whole Mage_CatalogInventory_Model_Stock_Item class from core to local, and add a call either to parent::_beforeSave(), or directly to Mage::dispatchEvent('cataloginventory_stock_item_save_before', ...).
Isn't this solution a bad hack?

Having searched (just by curiosity) in the many other direct descendents of Mage_Core_Model_Abstract, I've found that alongside with Mage_CatalogInventory_Model_Stock_Item, only two other subclasses share the same problem, at least as far as the _beforeSave() method is concerned (I haven't checked other basic events such as _afterSave and so on): Mage_XmlConnect_Model_Application``andMage_Dataflow_Model_Batch`.
Which leads me to think that this omission in only three classes may not have been done on purpose...

So: is it a bug, or am I wrong?

Answer 1

Not sure what is meant by "meringue" in this context, but this is one of several examples in which the targeted Mage_Core_Model_Abstract auto-fired events...

• *_load_before
• *_load_after
• *_delete_before
• *_delete_after
• *_delete_commit_after
• *_save_before
• *_save_after
• *_save_commit_after

...are broken.

This occurs due to either the failure of a subclass to call the parent template method when overridden (as you mention) or due to the failure to override the _eventPrefix in the subclass. To me this has always seemed to be an omission rather than an intention.

In this case of Mage_CatalogInventory_Model_Stock_Item::_beforeSave() this seems to be an omission. Up through the 1.3.2.4 version of this class the method duplicates the targeted event logic (though it omits the dispatch of model_save_before). The 1.4.0.0-alpha2 version of this file sees the _eventPrefix property being added, though the failure to call the parent _beforeSave() method results in the loss of this targeted event.

The only intention I can see in this instance may be to force the developer to manipulate the cataloginventory object as the stock_item property in the context of the the product save process, but that would merit a code comment.