Observer pitfalls of building serious modules in Magento
Unlike good old WordPress that “every kid in the block” knows how to create a plugin for, Magento is a whole new system. It requires extensive knowledge of OOP, ORM’s, MVC, and few other stuff. This is why not “every kid in the block” can write a module for Magento, and this is why I love it. However, unlike WordPress, Drupal and other community driven systems out there who keep in mind backward compatibility things with Magento things are a bit different.
One of the things that caught my eye and made me wonder about the consequences is the Observer and override functionality that probably most of the serious custom made Magento modules will utilize. Utilizing Observers enables you to observe and execute some action inside the “customer work flow” or even some admin stuff. Such functionality is heavily known is systems like WordPress and it’s called “hooks”. Besides hooking, another useful an very powerful feature which is actually related more to the OOP concept itself is the overriding. Personally i find the combination of observers and overrides to be the coolest and most powerful stuff in Magento module development.
So where is the issue? As of time of this writing, the latest Magento version is 1.3.1. Let’s look at the previous “major” release prior to that one, version 1.2.1.2. Here is a prepareForCart method signature from app/code/core/Mage/Catalog/Model/Product/Type/Abstract.php (line 239, Magento version 1.2.1.2):
public function prepareForCart(Varien_Object $buyRequest)
And here is the same method signature in Magento version 1.3.1
public function prepareForCart(Varien_Object $buyRequest, $product = null)
Notice the difference? Although this might not look like “that big of a deal” suppose you had implemented override functionality like
public function prepareForCart(Varien_Object $buyRequest)
{
parent::prepareForCart($buyRequest);
$product = $this->getProduct();
if ($buyRequest->getcpid()) {
$product->addCustomOption('cpid', $buyRequest->getcpid());
}
return array($product);
}In the above code, we override the prepareForCart method to to some custom stuff for us. This peace of code would work perfectly fine in version 1.2.1.2, but when client decides to upgrade the shop to 1.3.1 or newer he would get the error like
Strict Notice: Declaration of SomeClassOfMine::prepareForCart() should be compatible with that of Mage_Catalog_Model_Product_Type_Abstract::prepareForCart() in /app/code/community/MyCompany/MyModule/Catalog/Model/Product/Type/Simple.php on line 3
The issue itself is resolved by adding missing parameters to function we are overriding like
public function prepareForCart(Varien_Object $buyRequest, $product = null)
{
parent::prepareForCart($buyRequest, $product);
//$product = $this->getProduct();
if ($buyRequest->getcpid()) {
$product->addCustomOption('cpid', $buyRequest->getcpid());
}
return array($product);
}To me, stuff like this is a serious downside towards building advanced modules. Changing core files and core functionality can have serious impact on custom made modules each Magento upgrade. Therefore, one should keep an eye open on what modules he will throw into the shop. Personally I consider online stores very serious and have strong feelings about each store owner having somewhat of dedicated developer that will be in charge even for stuff like adding new modules.
Just consider the financial loss for a store owner of any more serious web shop if he decides to download the and install module himself just to realize that for incompatibility reasons this new module made his shop fully nonfunctional.
12 comments
Hi,
Thanks to share this.
This is an extension to Josh Ribakoff’s solution, i.e. copy Oxid Ecommerces override model.
In a nutshell, each class definition extends a none-existing pseudo class. E.g., two classes extending Mage_Core_Model_A
class Foo_Bar_Model_A extends Mage_Core_Model_A_Foo_Bar
class Much_Kiz_Model_A extends Mage_Core_Model_A_Much_Kiz
Now, when the class instances are created, magento would create the missing pseudo classes on the fly
class Mage_Core_Model_A_Much_Kiz extends Foo_Bar_Model_A
class Mage_Core_Model_A_Foo_Bar extends Mage_Core_Model_A
Of course, each module class would have to stick to the API and call the parent’s method when it’s done.
In practice this would not solve all issues, but it would help to eliminate a load existing conflicts, and it would create a path for modules to be created in a more compatible way in future.
Vinai
http://joshribakoff.com/?p=81
I was having the same problem and found this post. I found the solution to merge code unacceptable and so I wrote a extension that fixes it. Full technical details on my hacks are on my blog, along with the extension’s download URL.
I was pondering this issue yesterday, as I was creating a custom serial number module for Magento. I had to override the Mage_Sales_Model_Quote class, and I thought to myself: “this seems like a pretty core class to be overriding; what if another module is installed in the future and needs to override this class?”
Of course events are a wonderful way to avoid having to override a method, but there are many things that go on in the system that don’t trigger an event. I’ve made a couple of requests for events to be added to the Magento core. If any of you have suggestions, I’d encourage you to post them here: http://www.magentocommerce.com/wiki/customizing_magento_using_event-observer_method
One thing that I’ve seen a number of OS applications do with their plugins: force the module developer to update their module to state that it works with a certain version. This requires the module developer to test the module with all the newest versions of the application and then upload a new version of the module to the code repository. I could see Magento doing something like this, but it would have to be something that would be able to be disabled. The benefit of this setup is that there would be more incentive for Magento module developers to be proactive in testing their module against the latest versions of Magento.
@FOOMAN I don’t know definitively how the class overloading precedence is determined, but I’m pretty sure that the most recently loaded config takes precedence over any other config file. So if two modules overload the same class, the module whose config file was loaded last would take precedence.
I would suggest (from Magento’s perspective for future releases) having parameters passes as arrays of values. The methods always accepts an array, then values are pulled from the array. It actually can be create slighter faster running code also.
Great post. My hope is that Varien is able to stabilise their code at some stage in the near future. Maybe even extending the API to a point where it would be used from within Magento. All API changes would then require a new API entry point. In the meantime querying the version number of the module we override might be a good start, hoping that Varien is consistent when bumping the numbers (0.0.+1 = would not break existing extensions / 0.+1.x will break stuff)
I was wondering the other day as well if it was possible to override an override. But it seems that it only works for the Mage namespace. Has anyone figured out yet when two extensions override the same class who actually gets preference?
For an existing collision between the Canonical_URL and my Speedster extension we had to create a work around: Since the Canonical_URL gets preference it was rewritten to extend my class. It works but obviously not ideal.
I stand just at the beginning of module development for MAgento however I have already notify the difference. Of cause OO Methods are greate in comparison to hookes and api approaches. However you have to know how the system works to achieve first results in Magento development. Unfortunately there is not much development documentation on the Internet. So thank you for blogging about Magneto. I’ll do it in the future to as far I can.
Branko,
High quality article. Great observations, and it brings up a whole new venue utilizing the event observers as we progress into customizing this application in more appropriate ways.
Thanks for the thoughts that encourage growth.
Lee
How do you insert PHP code here without breaking it? :-\
I’ve solved this in Unirgy_Giftcert by creating 2 class files for 1.2.x- and 1.3.x+ 🙂
This observer changes configuration when needed:
public function controller_front_init_before($observer)
{
if (version_compare(Mage::getVersion(), ‘1.3.0’, ‘setNode(‘global/catalog/product/type/ugiftcert/model’, ‘ugiftcert/product_type12’);
}
}
To be honest at this point I’m not entirely sure what the best course of action would be here.
Adding more events into the core would most probably only scatter the issue, but still it would be here.
My first “happy moment” with observers was when I discovered the possibilities of adding numerous new events by utilizing override functionality and injecting the new event definitions in the new overrided methods. My thoughts were like “Cool, I’ll simply add tens of new events trough overwrite of some core methods then I’ll use my Observers to observer those new Events”, something like:
core method:
public function someCoreMethod()
{
//Some stuff here
}
override method
public function someOverrideMethod()
{
// Dispatch some new event here
parent::someCoreMethod();
// And dispatch some new event here
}
I haven’t done any testing to see how nicely this would play with entire system. This however does not resolve the “only one of changes will actually take effect” issue. 🙂
Great article, this sort of thing is a big issue when it comes to module development and is something I’ve been pondering myself lately.
What are your thoughts on the issue of multiple modules overriding the same class? Currently, if two modules override the same block (eg. sales_order_grid), only one of changes will actually take effect. This is obviously going to be a huge problem as more and more development is done and users are installing many modules to enhance the functionality of their store.
Perhaps the solution here is to intelligently add a lot more events into the core code so Observers are used more often than overrides?
What are everybody’s thoughts?