Review of a bad Magento 2 extension
Periodically, I do code reviews of other extensions, for use in training or just for fun. One extension bumped up on my radar after I asked on the Dutchento Slack about bad modules. I did a quick review and the amount of bad code was staggering. Even worse, this extension was listed on the Magento Marketplace. This blog lists all points I found for the purpose of spreading better practices with others.
Writing a good third-party extension for Magento 2 is challenging at times. It requires a sharp sight, a fair amount of common sense and advanced knowledge. While coding standards like PSR-2 only set the baseline, Magento also has its own additional rules and practices. It is a lot to digest.
However, a third-party extension is spread across numerous places. Not sticking to the coding standards potentially leads to extension conflicts, more costs for modification and more time spent on hard-to-trace bugs. A third-party extension developer, therefore, should set higher requirements to her or his code then an in-house developer should.
Using Helper classes
Here we go. The extension was making use of
Helper classes, while in general,
Helper classes are nowadays frowned upon. They are often created when the creator doesn't have enough fantasy about what else to name the class. I have to admit, some of my own extensions still contain helpers, but refactoring should get rid of them.
Too much logic in PHTML templates
There was too much going in PHTML templates. Collection filters were added, the
ObjectManager was called upon (which I will address in a later point separately) and in a lot of places, the PHP code outnumbered the HTML code.
A template should only be about templating. Ideally, the template only calls upon a
Block to get something (
$block->getFoobar()) or perhaps a
ViewModel is used for this (
$viewModel->getFoobar()). Any advanced logic (apart from ifs, loops and simple output) should be moved to the underlying classes (
Code was not cleaned up
There were numerous classes that were created but never used. There were also classes that were used while only extending a parent class and not adding anything on top of it - the parent should be used instead.
There were events listed to using observers while the observers didn't do anything. There was even a pointless PHP class in there with broken PHP code, so that linting (
php -l) of the extension folder failed. Sometimes dependencies were injected in the constructor, but not used at all. There were numerous namespaced classes imported in the top (
use A) but not used in the code - it popped up in my PHPStorm editor right away.
module.xml were lacking in documenting the extension its dependencies. There were dependencies with PHP 5.5, while I don't recall that Magento 2 was ever compatible with that PHP version. But the Magento Framework itself and some other modules (
Magento_Checkout) were missing from the
require section in composer. And the modules were not listed in the
sequence section of the
Personally, I'm using my own Yireo_ExtensionChecker a lot to detect what the requirements are.
Coding standard stuff
The code was definitely not compliant with the official Magento Coding Standard (which is kind of new, but really useful). There were warnings in there with up to severity 9. The code was also far from compliant with PSR-1 or PSR-2, which basically showed that the creators never bothered to use simple formatting by their IDE.
There were silly things in there. Prefixing internal variables with an underscore - well, the Magento core does so itself, even though it pops up with PSR-2 scans, but this is just a small little thing. There were also long methods in the code (largest method had about 400 lines of code) with far too many features so that the code was definitely not SOLID. Also, there were frequently too many indents in the code, for instance up to 12 indents in a single class, which shows that applying the Object Calisthenics could definitely benefit here.
Yes, the ObjectManager, always the ObjectManager
ObjectManager is used in the wrong places, it is always a sign that quality is lacking. So obviously, this extension had numerous issues with it: Classes were calling upon the
ObjectManager::getInstance() singleton directly, while the needed dependencies could have been injected easily in the constructor instead. This resulted in numerous parts of the code not being extendable when needed.
ObjectManager was also called upon within PHTML templates, even though in places this was harder to detect because it was hidden way in a
factory() method of the
Block class. In one place, the ObjectManager was used to
get() a singleton instance and then still the usage was via a custom singleton method (while actually
create() should have been used).
Too many dependencies
DI was also used in the wrong way: Too many dependencies were injected, leading up to one class with 25 dependencies (only the legacy
Product model outnumbers this). And I mentioned earlier already that sometimes dependencies were injected but then not used.
Preferences instead of plugins
In multiple occasions, preferences were used to rewrite original core classes, while actually the purpose of the rewrite was a perfect scenario to choose a plugin instead, or by adding a Virtual Type, or by using
Type arguments, or by simply using composition. I really got the impression here that the developers didn't understand the DI toolbox that Magento gives them.
Plugins with around() instead of before() and after()
Plugin classes is always open for debate. In the code, I found multiple instances of plugins that used
around() where actually
after() were just as efficient.
Manual SQL code
Manual SQL queries were used to write to the Store Configuration instead of using
\Magento\Framework\App\Config\Storage\WriterInterface or similar classes. There were also SQL queries in the
Setup\Install class to insert default configuration values, while actually, a simple
default section in
etc/config.xml would have been enough.
Interestingly, there were models, resource models and collections setup. Still, manual SQL queries were used to collect and write information from the same table, while actually, the model could easily have been used. No repository.
Call to home
In the Magento Admin Panel, any time when a page was loaded, the extension kicked in by listening to the event
controller_action_predispatch and calling home to its own website for a license check. This also means that if the extension vendors site or server would be down, your Magento Admin Panel would be unreachable.
The extension itself required a base module, which did a hell of a lot of work to add notifications to pop up on each backend page, in the event of a license check failing. Not nice.
No ACL file
There was no
acl.xml file defined for the extensions backend pages or its configuration setting. Still, ACL resources were defined all of the place: In the necessary
system.xml and controller classes. But there was also a custom check for an ACL resource in one controller that simply led to code that would be only executed if you were logged in with a role with access to Any resource.
CSS instead of LESS
CSS files were added manually to both the frontend and the backend. There were no LESS files involved.
In the XML layout, there was a template definition with the
translate argument set to true, as if the template was translatable (
<argument name="template" translate="true" xsi:type="string">sample.phtml</argument>). Either this leads to really cool behaviour (using different template files per different StoreView by using translation as a driving mechanism) or it was just a mistake.
So, why is this bad?
Overall, the extension had so many flaws that it made me nauseous. The lacking requirements in
composer.json could potentially break a production site because nothing prevented the extension from being rolled out in incompatible environments. Also, the numerous bad usages of DI and the
ObjectManager led to difficulties extending (or fixing) the extension properly. It was hard to create theming overrides. It added more resources on the page (internally with the call-to-home and externally via CSS) so that installing this extension, the site performance went down.
All in all, this extension lacked in code quality. But the actual problem meant that the bill went up for the merchant using this extension. Especially, when the in-house developer of this project (merchant or system integrator) would need to fix or modify things in the extension. I hope you found this (rather long) summing-up a worthy read and I hope you agree with me that extensions like these are to be avoided. I'm happy to review some more extensions.