Review of a bad Magento 2 extension

Baseline

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 (Block or ViewModel).

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.

Undocumented requirements

The composer.json and 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_Catalog and Magento_Checkout) were missing from the require section in composer. And the modules were not listed in the sequence section of the etc/module.xml file.

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

When the 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.

The 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()

Using around() in Plugin classes is always open for debate. In the code, I found multiple instances of plugins that used around() where actually before() or 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 menu.xml, 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.

Translatable templates?

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.