Add colleciton method "addAfterLoadCallback"

[colinmollenhour]

This patch adds a new method to Varien_Data_Collection_Db which allows one to queue up callbacks which will be called after the collection is loaded. This is important since some event observers (e.g. Mage_Review's product list observer) act on the loaded collection, but it may be that other event observers need to first modify the query (e.g. add a filter or join). By using callbacks the event observers that need to act on a loaded collection can just add a callback and leave the query unloaded so that other event observers can still modify the query.

[magento-team]

@colinmollenhour Thanks for the contribution. You're suggesting to change a super class of high abstraction level (there are approximately 200 collections currently in Magento application). Could you please cover the changes in the 3 affected classes with tests?

More details in https://github.com/magento/magento2/wiki/Contribution-Guide

Also see comments on the diff.

[magento-team]

It makes an already complex implementation even more complex. And in general callbacks are hard to comprehend and debug.
Also you can pass whatever callback you want (literally anything), and the interface is limited to one argument. How reliable is this? How would you document this? Kind of hard to test, too.

[magento-team]

I believe the root cause of this contribution is a design problem in the collection: it doesn’t encapsulate DB-select object and in most cases adding a filter/limitation through collection API would modify the select object instantly.
Often if you call this method (that modifies select object) twice, there is a crash hazard – and that’s the most common problem that observers have when they touch collections.

So the solution for this design problem is this:

• Don’t allow access DB select object through collection API
• Refactor collections in such a way that any filter or limitation methods would alter collection object state, rather than DB-select. Utilize lazy load pattern to not only load data, but actually create and prepare SQL-query at the last possible moment.

A simplistic solution could be illustrated in:
Mage_Catalog_Model_Resource_Product_Link_Product_Collection::_beforeLoad()
Mage_Paypal_Model_Resource_Payment_Transaction_Collection::_beforeLoad()

In both cases whenever a filter is set, it just records a variable value to the state of collection object, however doesn't mess with DB select until the last moment. Which allows setting/changing filter multiple times before loading.

Just so you know.

[colinmollenhour]

The solution to the design problem you proposed appears to severely limit flexibility and would therefore require overriding classes. It is far better to offer more flexibility even if it is potentially hazardous than to require users override classes which IMO is far more hazardous.

Here is the design problem: there is one event dispatched but two potentially very different use-cases for this event. Those are: 1) modify the query before it executes (e.g. addFieldToFilter()) and 2) modify the loaded items (e.g. appendReviewSummary()). The big problem is the ordering of these cannot be easily controlled. For example, Mage_Review causes the collection to be loaded so no other observer can modify the query.

An alternate solution would be to dispatch two events back-to-back instead of one, like:

Mage::dispatchEvent('catalog_block_product_collection_load_before', array('collection' => $collection));
Mage::dispatchEvent('catalog_block_product_collection_load_after', array('collection' => $collection));

I'd be fine with this solution as well but as it stands currently, adding a simple filter to the product collection (or any other query change) requires a core class override.

[colinmollenhour]

Besides, I thought this addAfterLoadCallback() was consistent with Mage_Core_Model_Resource_Abstract->addCommitCallback()?

How are callbacks any harder to debug than event observers?

[magento-team]

Observer pattern is not designed to guarantee order of executing subscribers. Also in the observer pattern, for good, the sender doesn't care how subscribers use obtained information (and won't change its state because of subscribers).

Indeed, the addAfterLoadCallback() is consistent to addCommitCallback() in current framework (despite all the hazards they bring). So as it was mentioned before, we can accept this contribution if it is supplied with tests.

[colinmollenhour]

I just pushed what I think is a bit simpler solution, but it doesn't add a potentially very useful feature/method. Instead I just made there be two event observers instead of one and they are clearly labelled. The review observer was changed to observe the "after" event. See: 14b6b5ac8c3c073ed0974899b7376146e8ab261c

[verklov]

@colinmollenhour , our team investigated further on this issue and the resolution is that this change does not make sense since it can be done through using the plugin.