This week, the “extension quality” working group of ExtDN hosted their first community hangout, open for anyone interested to join. This blog post is going to be a short recap and also an introduction to the initiative.

Working Group “Extension Quality”

I have the pleasure to lead the other new initiative, “extension quality”, together with Jisse Reitsma (Yireo) and Kristof Ringleff (Fooman). Our goal is to define and formalize a set of rules for high quality extensions. This is something, the official Magento Extension Quality Program (MEQP), the gatekeeper for Magento Marketplace, could have been. But it is lacking a few key aspects, that are important to us:

• Documentation of rules: The documentation (PDF) is vague; the Coding Standard contains a huge amount of rules, but mostly without explanation (why you should not do something and what is the alternative)
• Agility and Release Cycle: While there has been some development within the last year (also by the community), the current release 1.0.5 is from April 11th, 2017 and creating a new one seems to be a huge effort (see Github issue).

We will reuse the MEQP coding standards as far as they make sense to us, but not without documenting them. In the end, it would be great to contribute our tools back to Magento and the Marketplace, but for now we believe that the community can move faster on its own. We are still in close contact with the Marketplace team, giving and taking feedback, which is a good thing!

Our current project is the ExtDN PHPCS ruleset for the PHP Code Sniffer (phpcs). We collect and discuss possible rules in the Github issues, including those that would not be possible/feasible to implement as code sniffer rules. We plan to integrate and implement other tools later, but concentrate on phpcs for now. For further discussions, everybody is invited to join the #extdn-ext-quality channel on Magento Community Engineering Slack (email requests to join to engcom@magento.com).

Community Hangouts

On May 29th, we had our first open hangout. This is going to become a regular thing, probably once a month, and is meant to cover the following:

• Status updates
• Organizational stuff
• Discussion of rules and implementations, which are undecided yet (Github issues tagged with “on agenda of hangout”). There is no voting and the final decision will be made by the working group, but input is important and we will always try to find a consensus first.
• Gathering feedback

First Hangout: Recap

After some technical and organizational difficulties (lessons learned: time zones are hard, do not run a zoom meeting without anybody that has banning permissions), we were able to welcome a handful of participants, including Magento Marketplace Lead Architect Ravi Menon. I’ll skip the part with trolls and introduction and jump right to the discussed topics:

Rule: Virtual Types should have namespaced names (PSR-2)

First, we need to look into the possibilities of phpcs sniffs that read XML files. If feasible, the rule can be created, but with a lower severity to make it a recommendation, not a requirement. The same goes for a rule that virtual types should have “Virtual” as part of their name, either as prefix or as suffix.
There still are some disagreements on the topic (See Github issue and Twitter for example). Some arguments, briefly summarized:

• PSR-2 namespaced names may mislead to the presumption that they are classes
• This can be addressed with a “Virtual” prefix or suffix. PSR-2 has the advantage of consistency.
• It should not matter, if a type is an interface, a class, or a virtual type. It should be possible to switch at any time.
• Virtual types do not exist in code, they are just configuration. If a virtual type is replaced with a real type, declaration and usage usually can be changed at once in the same XML file.

Rule: Curly braces should not be used in PHTML files

There was a good discussion on readability of templates, and the final consent was that the best way to formulate a rule is

Opening and closing braces must be within the same

<?php ... ?>

1	<?php ... ?>

block.

So if there is HTML content inside a “foreach” loop or an “if” block, the “endforeach” and “endif” syntax must be used to prevent lonely closing braces. Otherwise it is difficult to see where they belong to.

Rule: Keyword “new” should not be used

We agreed that the spirit of the rule is good, namely to encourage dependency injection, but it may be too strict. We want to try it out for a while, with a lower warning level and of course document when and why this makes sense.

Rule: Do not provide static methods and properties (was: Paamayim Nekudotayim (double colon, static) reference should not be used)

The least thing to check is whether there are any public static definitions, maybe also protected static definitions, with a medium warning level.
The rule should not apply for test code because static methods are necessary e.g. for data providers. We can either exclude *Test.php or use a warning level that is not going to be used for tests.

Rule: Do not use class names as string literals (was: Warning for String-Representation in $this->createMock)

The hard part here is how to detect if some string is supposed to be a class name. There is a MEQP sniff that assumes a fully installed Magento, including generated code. It uses “class_exists()” and “interface_exists()” on strings to find out if there is a class for it. But we want to be able to run the ExtDN code sniffer in any extension repository, without installing Magento, so this is not enough.

A discussed idea was to look for arguments of certain methods like “ObjectManager::create()” but there are many possibilities where a class name can be used as string. We will try to implement the rule with a regular expression instead, using a low warning level, and try out if any false positives occur. The MEQP rule can be included as an addition – if there is no Magento installation, it will just find nothing.

Errors, Warnings, and Severity Levels

Phpcs distinguishes between warnings and errors, and additionaly has severity levels from 1-10 for each.

Magento uses only 6, 8 and 10. The marketplace automatically rejects extensions if there is a violation of a rule with severity 10. The lower numbers are assigned without any particular rule.

To implement sensible rules for severity levels, we need to figure out use cases first, that would require a certain minimum severity threshold.

We found the following interesting thresholds (no numbers yet):

• Experimental: Rules that we want to try out first and should be ignored by anyone else
• Strict: All the best practices for extensions
• Default: Good extensions should have that
• Project code: Where some of the rules for extensions do not apply, e.g. extensibility does not matter
• Test code: Some more rules do not matter

That means, if for example default is “6”, a threshold of 6 checks all the rules with severity 6 or greater. This will exclude strict rules and experimental rules.

A concrete proposal is now on the Github issue.

Conclusion

Thanks again to everybody who joined the hangout and participated in discussions on Github and Slack. This blew up in short time and I hope to preserve that momentum!

If you are an extension provider and care about quality, feel free to join the group via this form or the ExtDN website.

But if you are an integrator or extension user, your voice is just as important and you are invited to participate as well! Chime in on Github issues and join the #extdn-ext-quality channel on Magento Community Engineering Slack (email requests to join to engcom@magento.com).

And if you are a developer and willing to help out with new code sniffer rules, look out for the good first issue label – these are rules that are not too complex to implement and should be a good start into development of phpcs sniffs. You can also pick any issue with an accepted label that are not assigned to anybody yet. Just mention that you start developing on it. And if you have any questions, feel free to ask on Slack!