Code auditing and code fixing in Magento projects
We often encounter the challenge of fixing problems or implementing new features in a working Magento store instead of being assigned the task of developing a new ecommerce shop from scratch. Here the question is how to start and what steps to follow to decide whether to take over an existing Magento project that was originally developed by someone else.
First step in Magento code audit
It is worth checking if some hard-coding took place in the project. Are there any modifications that were carried out in the core files and not in rewritten in config or transferred to the community/local codepool and modified there.
To be able to check this, we need a Magento project of the same kind (Community or Enterprise) with the same version number that we can compare it with.
For this we use PHPStorm, but this comparison can be done smoothly with other types of programmes as well. Right click on the app/code/core directory, “Compare With…” (Ctrl + D), and then select the same directory in the “co-environment” (in a blank Magento environment).
After you’ve accepted it (OK), a window pops up with the changes, here you can switch off “Show new files on left side” and “Show new files on right side” options because it would only cause some confusion. Having this done, only the “Show difference” option remains active, which lists the differences.
Unfortunately I have not found such an option that does not take comments into account, and although the code parts are the same, more files than differences are shown because of the comments.
Apart from that, in our case, there is no difference between the two projects. If in your case it is not so, i.e. there are differences in the system, the files which differ from the original ones, need to be restored and the modified ones need to be transferred.
1) Copying the core file from app/code/core/…/File.php to app/code/local/…/File.php and then reverting (setting) the original file into its original state (before any modifications were made) 2) Overwriting the file by config.xml with a file of our own module. Here too, the original file needs to be set to the original state and this original file should be extended by our file 3) As a third possibility, functional differences can be solved with an Observer
1) Copying the core file from app/code/core/…/File.php to app/code/local/…/File.php and then reverting (setting) the original file into its original state (before any modifications were made)
2) Overwriting the file by config.xml with a file of our own module. Here too, the original file needs to be set to the original state and this original file should be extended by our file
3) As a third possibility, functional differences can be solved with an Observer
In my opinion, the first one is the fastest and the last one is the finest.
Having this done (the app/code/core files are reset to their original state and the modifications are “transferred” to their proper locations (e.g. the appropriate observer method or under app/code/local in the duplicate of the original file).
The next step is comparing the content of the directories app/design/[frontend/adminhtml]. Unfortunately it often happens that a developer does not create a separate template or layout file, but modifies the original file in the base/default/[template/layout] directories.
Fixing this can be done in a similar way as in the case of the code. We copy the modified files in the same structure under our own theme and we revert the original files to their original state. E.g. we copy-paste the app/design/frontend/base/default/template/catalog/product/list.phtml to the app/design/frontend/rwd/theme/template/catalog/product/list.phtml directory and revert the original one.
In case of layout files it is enough to copy the variable part, there is no need to transfer the whole layout file. An even finer solution is that we refer to a defined block with a reference.
In case of templates it is also worth considering whether we can change them from layout according to our preferences. If, however, the new elements / blocks / templates in question do not appear in the appropriate locations after modifying them from layout, the method mentioned above (transferring, changing the template) offers a reliable solution.
Let’s suppose that everything went fine in all these locations, the template and layout files were modified in the proper locations. If so, then it looks a sound project, probably there won’t be problems with it later on.
Now the quality of the existing modules and template files need to be checked. This we can perform according to the following aspects, to be detailed later one by one:
- Code redundancy – code repetition
- Code relevancy – storing certain codes in appropriate locations
- Installer Scripts – quality and reliability
- Template files – minimizing object calls
- Block / Controller check – load and execution
Code redundancy – code repetition
Unfortunately it is quite hard to detect code redundancy, since the code is not an exact property, so we can run tests in several different ways. Therefore it may take more time to analyse and find code redundancy than to fix it.
The basic notion is to make developments in a manner that the right elements are located in the right places, there is no need for repetitions so that changes can be made in one single location for different operations.
Code relevancy – storing specific codes in proper locations
There are codes that have obvious locations, such a property is a sub-page and action of a new module that must be placed in the controller because otherwise it does not work. There are codes, however, that can either be in the Block, the Helper or even in the Model, no matter what function they have. But these also have their proper locations.
Codes related to general operations most often need to be placed in the Helper in order to assist cross-functioning. Typically such codes are smaller methods checking a module’s state and/or that, either directly or indirectly, work with config values.
Additionally, checks for current end users, not present in Magento by default, can be placed here, e.g. checking a certain type of user after an extended registration process ‒ $helper->isStudent(); $helper->isTeacher() etc. (These can also be implemented using an Observer.)
Operations that sometimes “get lost” or “stray” in the process of implementation, can also be mentioned here. A good example is the fields of a Model to be exported and also its CSV headers that should not be placed in a Helper or Block since in any place where the Model gets called, these may be needed, so it is simpler and more logical to store these in the given Model.
I also mention here, with the Model, the queries of parent-child elements, i.e. if a given Model has a connection of 1-* with another Model, it can be placed in the “main” Model with a $model->getChildSomethings(); method.
Installer Scripts – quality and reliability
While writing installer scripts, we tend to forget about that they, for some magic, indefinable reason (maybe someone deletes an attribute, field, value and runs the script again), sometimes run twice and thus we encounter unexpected errors.
Eliminating this needs a certain developer attitude: although we are likely to create and run our script based on the data of our own environment, it is worth preparing it for all possible cases that may occur, and writing the script so that it is started with a condition. If we add an attribute to it, we need to look for the attribute first and adding it should be based on a condition.
When modifying the fields of a table, it is wise to check its current state. We insert the $installer->endSetup(); code with a 100% secure run, if the installer operation is placed in a try-catch, endSetup() should be at the end of the try (not outside of it) ensuring that our module will change (upgrade) the version number only if it works properly.
Template files – optimizing object calls
When writing template files (.phtml) we need to pay attention to clearness and the reusability of the code.
What does it mean?
- No static values are implemented
- No functions or methods are declared here
- No greater number of methods are called than truly necessary
- We develop values/configurations that comply with layout requirements as much as possible (to support the work of the frontend developer).
- Every function which is called in addition to requests, should be declared in Blocks
- Depending on the complexity of the templates, the proper solution is to make smaller or sub-templates (e.g. with 2 different product/user types)
It is also important, basically in all aspects of development, to mention that we never identify a value (0, 1, 2) with its value, but it is very much recommended (if not compulsory) to place in a constant and thus with an “expressive” variant name, make its value clear. A bunch of nice examples can be found in Core Magento, e.g. defining product status values, xml config paths and certain minimum/maximum values.
In this case these are not only “more straightforward”, but using it everywhere else, it is easier to search them (fewer, more focused results) and we are able to modify them in one location.
Block / Controller check – load, implementation
When auditing Blocks and Controllers two aspects need to be taken into account, which strongly correlate: the load it needs to endure and how implementation is carried out. The better the implementation, the smaller the load.
If the getCollection() method of the Block is called several times from a template file, it is a good way to cache in the Block, thus true database operation takes place only the first time, any further operation gets only to the cached object list. For this, we also find Magento Core examples that can be used as guides.
In the case of Controllers, it is worth taking a look at the size of the different actions. I do not want to suggest font sizes or number of rows, as to what limits there are for an action, but we can agree that it needs to look all right, both in terms of appearance and volume, just by looking at it.
There is one more thing to add.
In my opinion, the Block creations inside the controllers are not necessary, this can be carried out inside another Block or from layout. Thanks to this, we reduce its size (number of code lines).
All this does not seem to be a lot of stuff. We have examined only 5 problem factors in detail, and 2 that are rather independent from them. But if you consider that a larger group of developers (more than 4) can work parallel on the same project, creating or modifying tens of files each for one module at the same time, then the whole thing looks much more complex.
If there is even only one minor error in each of these files, in the end it may result in a smaller refactoring before or after launching the project. Going through all the points I have mentioned in this article, we get a more detailed overview about how many and what type of problems our project has that need to be handled.