When working with Magento, the need to extend and alter core functionality is something that comes up frequently. Fortunately Magento provides a fairly robust rewrite system to facilitate this, so creating a class that extends a core Magento object, and making Magento load your class in place of the original, is trivial. There are plenty of resources for rewriting core Magento functionality, so I’m not going to dwell on that mechanism here.
So what’s the problem?
Where things can get ugly is when the change you need to make happens to be a single line in the middle of a 200 line method, so you extend the base class, copy the entire method from the parent class into your own, and make a change. But then you realise that the method you’ve copied relies on a few private methods from the base class, so you copy those in as well. Before long you have several hundred lines of core code in your class, when all you wanted was a single line change.
But at least it works, right? Well, yes it does until you commit your work and break the build because you’ve introduced a number of PHPCS and PHPMD violations that were present in the core code (and never mind that the chances of being able to unit test that code are going to be very slim). So now you have a number of options. You can either:
- Ignore the issues by adjusting the thresholds in your build tool
- Ignore the issues by configuring the build process to ignore your new class
- Use annotations to suppress the static analysis tools
- Create custom rulesets to match the core coding standard
- Fix the core code in your class
- Make your change to the core Magento object instead
Of course, there are issues with each of these approaches.
- The biggest issue here is related to Broken Windows. The basic theory of Broken Windows is that as soon as a small element of disrepair is allowed to exist in your project, it will generate apathy and gradually the level of disrepair will grow until the project is no longer recoverable. For this reason I think that projects should strive for nothing less than perfection in build statistics, including test results, coding standards and static analysis.
- If the build process is ignoring your class you have no visible broken windows. Instead you have termites. Your code will gradually rot from the inside, and without the protection of your build tools it will go unnoticed. Changes made over time to ignored files can increase the complexity, and so lower the performance of your application.
- Whilst being marginally better than point 2, suppressing your analysis tools with annotations effectively gives the same problem. Your class now contains documentation stating which issues exist, but you still have the potential for the scale of the issues to increase unnoticed.
- Whether or not you want to adopt a coding standard that matches the Magento core is your own business, but allowing the same looseness in your complexity detection is bad. Complex code containing many paths is less performant, and should be avoided for that reason if no other. Altering your configuration to allow complex code is just point 2 all over again.
- Fixing the core code is a trap that I’ve fallen into myself. The first problem with this approach is that you might inadvertently break some unnoticed core functionality. This is especially likely when breaking the code up to resolve complexity issues. The second problem is maintainability. When you come to upgrade your version of Magento it will be very difficult to determine what the actual change to the core behaviour was if you have rewritten it beyond recognition.
- Change core? Everyone knows you don’t change core, right? RIGHT?
Magento occasionally release patches. These are usually shell scripts that you can execute against your code base to fix bugs or security holes in the core software without having to update your entire core version. On the project I work on, we have a policy of storing every applied patch with our source files. This means that someone new to the project can easily see what changes have been made to core, and that when we upgrade Magento, we know which changes we need to consider re-applying.
We can also create patches ourselves, using our version control tool of choice. Here’s how to do it in the most common version control tools:
I’m sure that it is possible to use Subversion to create a patch of the last commit, but that is left as an exercise for the reader!
Safely changing core
The main reason cited for not changing the core of any framework is that your changes are at risk of being overwritten when a framework update is applied. This is especially likely with Magento, where the recommended approach to an upgrade is to start with a vanilla copy of your target version and add in your customisations. By creating patch files of your changes, you are protecting yourself, because when you update Magento you can go through your list of patches, applying them one at a time. Some will apply cleanly, others will require some work, or might not be needed at all – but you have the opportunity to assess each one.
It is important not to get carried away. If you change core, you risk changing the behaviour of your application in unexpected ways, and since you will not be covering the Magento core with unit tests, the best protection you can hope for is a robust set of integration tests (you have an integration test suite, I’m sure). This means that your core changes should be as small and unobtrusive as they can be.
I think it is best to restrict yourself to only two possible changes: either dispatch an event, or create a placeholder method. Both of these techniques ensure that, when patched, the Magento core will continue to behave exactly as it did before. Furthermore these changes are both trivial to make. In order to change the behaviour of the application it is necessary to write code in the local codepool that makes use of your modification to the core.
Deciding on the type of change to make is a fairly straightforward choice based on what your extension is trying to achieve.
Add an event
Magento comes with an event system that is easy to use. With a single line change to the core you can insert a point at which any number of modules can hook in and execute code. Obviously there are a great number of events already dispatched by Magento, but sometimes creating a new one can lead to cleaner code, rather than trying to use an event that is not quite in the right place.
Dispatching an event in Magento is simple:
Modules can register Observer objects to events, declaring methods that are to be called when the event is dispatched.
A simple example of when to use this approach could be in Mage_Customer_AccountController::createAction. The code for this method is:
If we wanted to perform some kind of activity in the middle of this method we could go to all the trouble of setting up a rewrite to allow us to extend this controller and overwrite this method. If, however, we want our activity to occur after the check for the user being logged in, we are going to have to copy the whole method into our own class verbatim, then add more code in the middle to satisfy our requirements.
Obviously this isn’t a huge issue because the createAction method is very small and easy to understand. Had I instead decided to paste in all 100 lines of Mage_Core_Model_Url_Rewrite::rewrite this would be much harder to read, but it may have better made the point.
So instead of doing the rewriting and the extending, how about we just place an event in the middle of the method, right there in the core codepool? Now it looks like this:
That’s a simple, safe, one line change to the Magento core that is easily reproducible in other versions. This event can now be leveraged from as many custom modules as you care to create, with no further need to edit the core or rewrite and extend the account controller.
Notice the use of ‘namespace’ in the event name. If you namespace your new events, ‘sessiondigital_new_account’ for example, then you are ensuring that you will not create event name collisions within core. It will be up to you to ensure that you don’t create any collisions with your own modules that may be using the same namespace.
Add a placeholder method
An alternative to dispatching an event is to use a placeholder method in the core class to create a clean point of extension. I find this approach to be most useful when the requirement involves manipulating data within a method. In such a case I prefer not to use an event because it can abstract the manipulation away from the rest of the code in a way that makes the intention less obvious. Consider the following protected function from Mage_Customer_AccountController:
The above shows it passes an empty string as the $backUrl option to Mage_Customer_Model_Customer::sendNewAccountEmail. If you wanted to change that behaviour, you would have to extend this controller and copy in this entire method, with all its logic. That then becomes logic that you are responsible for, and it contributes to the technical debt of your application.
Instead of doing that, a cleaner approach is to change core to add a placeholder method. The principle here is to make the smallest change possible, so we can simply replace the empty string with a call to a new protected method that just returns an empty string.
We are now free to rewrite the account controller as normal, but instead of copying in several lines of core logic, we only have to re-implement _getBackUrl() with whatever custom logic we require. The next person to come to the project can see the rewritten class that contains only the customisation without any surrounding noise, and will very quickly be able to understand its purpose.
How patches are managed is something that will be slightly different for everyone. The approach that the Session team currently uses is to have a directory in the project root that holds all the generated patches, and any that were generated elsewhere and applied to the project. This makes it easy to perform upgrades to Magento, because each patch can be assessed against the new version and reapplied as appropriate.
For companies that manage several projects, it may be beneficial to keep a separate repository of all the created patches, so that they can easily be shared between projects.
This approach is not for every team, project or situation. As with any other tool it is to be kept in the toolbox and only brought out when it is appropriate. The greatest gains are likely found when used in conjunction with static analysis tools like PHPCS, PHPMD or Scrutinizer. Certainly if you want to ensure that your Magento code is of a manageable quality, in terms of readability and maintainability, patching additional events and placeholder methods into core can greatly assist you.
Changing the core of a framework is still not something to be taken lightly, but with the correct tools and discipline it is a viable option, and one that I am turning to with increasing frequency.
About the author
Shane has been working in web application development since 2004, primarily with PHP, but also with large portions of ActionScript and front-end technologies. Shane is particularly interested in open source software, and when not spending time with his family or his archery club he contributes to several open source projects. Find him on Twitter @shanethehat or his Github.
This article was originally published under Session Digital, which unified with Inviqa in June 2016. For more information about the unification visit https://inviqa.com/new-era.