[Proposal] Version specific sniffs

[lenaorobei]

Problem Overview

Some of the rules like strict_types were introduced in later Magento versions and are not applicable to earlier ones. Magento Marketplace still checks the code of extensions compatible with Magento 2.0, 2.1, 2.2. How to handle version specific rules?

Solution

Provide mechanism of version specific sniffs using OOB PHP CodeSniffer functionality. By default PHP CodeSniffer will do check assuming the latest Magento version.

Implementation Details

Create new sniffs Group which will handle runtime parameter magentoVersion and run sniff only when it meets version requirement.

For example
phpcs --runtime-set magentoVersion 2.2

Each sniff from version specific group will call doRun method that checks versions compatibiliy.

use PHP_CodeSniffer\Config;

class VersionChecker
{
    public function doRun($sniffVersion)
    {
        $runtimeVersion = Config::getConfigData('magentoVersion');
        if ($runtimeVersion !== null) {
            return version_compare($runtimeVersion, $sniffVersion, '>=');
        }
        return true;
    }
}

Magento version specific sniff will contain code that determines whether the sniff needs to be executed.

class SomeNewlyIntroducedSniff implements Sniff
{
   // Magento version where the rule was introduced. 
    private $introducedIn = '2.3';

    private $versionChecker;

    public function __construct()
    {
        $this->versionChecker = new VersionChecker();
    }

    public function process(File $phpcsFile, $stackPtr)
    {
        if ($this->versionChecker->doRun($this->introducedIn) === false) {
            return;
        }
        //code goes here
    }
}

Pros

• everything in one place;
• no need to maintain the whole repo versioning.

Cons

• only default behavior will work in IDE;
• need to care about legacy sniffs if specific Magento version became unsupported.

[jissereitsma]

Simple and efficient. I was personally thinking along the same line because I would like to see this version check right in the PHP code of the sniff itself. However, there are some alternatives (on top of this option 1 that you have documented) that are maybe nice to mention here for the purpose of a debate:

• Option 2) Create a separate branch of this repo with version-specific rules. For instance, production-2-3-0.
• Option 3) Create a separate ruleset.xml per version. For instance, ruleset-2-3-0.xml.
• Option 4) Same as option 1 but without a CLI flag but simply an inclusion of the Magento application to determine the current version. This is nice (because I'm actually still in love with including Magento runtime in static tests, even it is wrong), but the decision of this project is to have tests run without any linking to the actual application, so this is just a brainfart.

Option 2 leads to "invisibility" of the rules, in my opinion. Option 3 is nice but maybe leads to too many rulesets, while it also splits up the sniffs functionality (PHP class) and its compatibility (XML). Your option 1 combines both functionality and compatibility in one single place (PHP class). I'm personally in favour of option 1 (as suggested by you).

Off-topic suggestion A: Make $introducedIn a constant?

Off-topic suggestion B: Make it even easier for sniffs to reuse the VersionCheck. For instance, by adding a new parent class with tools like these (because I'm not picky with Composition over Inheritance when it comes to testing) or perhaps add it to a trait instead.

Off-topic suggestion C: You mentioned that declare(strict_types=1) sniffs would need to be Magento-specific. But is that correct? It is actually PHP-specific. So, if I would be able to run a Magento version in a specific PHP-environment, perhaps I want to make sure

[lenaorobei]

@jissereitsma
Cool that you've described other options. Now we definitely have the basis for debates.

Off-topic suggestion A: Make $introducedIn a constant?

Now I'm thinking about public property, because in this case we can change the property value in the ruleset.xml.

Make it even easier for sniffs to reuse the VersionCheck. For instance, by adding a new parent class with tools like these (because I'm not picky with Composition over Inheritance when it comes to testing) or perhaps add it to a trait instead.

😄 It was exactly what I wrote before, but the idea of Composition over Inheritance didn't leave my head so I changed to this. I think, you're right. The simplest approach here makes more sense.

You mentioned that declare(strict_types=1) sniffs would need to be Magento-specific. But is that correct? It is actually PHP-specific.

Yes, it's correct. It's just a bad example.

[jissereitsma]

I have no objection against making $introducedIn a public property.

As for the examle, the declare() usage is not necessarily bad, because it has become a good practice related to a Magento-version. But it involves a discussion on PHP compatibility as well (for which maybe a different PHP-specific ruleset is better). Maybe a good example is ViewModels - introduced in Magento 2.2.0, improved in 2.2.1 (no XML class attribute needed), but it doesn't work under 2.1 or before.

[jissereitsma]

Maybe to throw some SOLID at it: The responsibility for the VersionChecker class is to check upon the version. I would expect methods like isVersion($version) or isOlderThan() (just talking loosely on a Sunday morning on this). A doRun method determines whether the rule should be run, which is actually not the responsibility of VersionChecker but of the rule itself. A call like this might make more sense: if ($this->versionChecker->isVersionOrHigher($this->introducedIn) === false) {. The naming of methods then still needs some work.

[larsroettig]

Hi @jissereitsma, a VersionChecker sounds good to me

[sprankhub]

Thanks for the suggestion. I like the initial option the most as long as the "is-sniff-relevant-for-version-check" is not implemented in each and every sniff as already discussed. Regarding the cons:

only default behavior will work in IDE;

IMHO, this is not too bad since usually, one develops for the latest version of Magento, so that the default behavior should be okay.

need to care about legacy sniffs if specific Magento version became unsupported.

Not sure that you mean by that @lenaorobei. If a specific sniff just makes sense until e.g. version 2.1 and version 2.1 becomes EOL, we should simply delete this sniff. Of course this is still maintenance work, but since it is just deleting, it should not be too hard.

Another thing to think of is a $relevantUntil property. There may be cases, where a specific sniff does not only have a minimum Magento version requirement, but a maximum version requirement as well. For instance, if specific concepts like ViewModels or whatsoever is dropped at some point, sniffs for them do not make sense any more.

[lenaorobei]

@sprankhub that's a good point.

Based on our comments and suggestions, the implementation can be following.

trait VersionHandler
{
    public function isRelevant($introducedIn, $deprecatedIn = '')
    {
        $runtimeVersion = Config::getConfigData('magentoVersion');
        if (($runtimeVersion === null) && ($deprecatedIn !== '')) {
            return false;
        }
        if ($runtimeVersion !== null) {
            $isGreaterOrEquals = version_compare($runtimeVersion, $introducedIn, '>=');
            if ($deprecatedIn !== '') {
                return $isGreaterThan && version_compare($runtimeVersion, $deprecatedIn, '<');
            }
            return $isGreaterOrEquals;
        }
        return true;
    }
}

Note: $deprecatedIn is optional.
If $deprecatedIn property is set, but runtime option is missing (--runtime-set magentoVersion 2.2), sniff will be skipped assuming we test against latest Magento version.

class SomeNewlyIntroducedSniff implements Sniff
{
    use VersionHandler;

    public $introducedIn = '2.0';

    public $deprecatedIn = '2.3';

    public function process(File $phpcsFile, $stackPtr)
    {
        if ($this->isRelevant($this->introducedIn, $this->deprecatedIn) === false) {
            return;
        }
        //code goes here
    }
}

[larsroettig]

@lenaorobei this implementation looks good 👍

[buskamuza]

My 2 cents:

1. Restrictions specific to Magento version (not PHP version) should not be hard-coded in the rules themselves. Like a business logic. Today we may want to apply the rule to Magento 2.3 only, but tomorrow we might want to use it for older versions too (because we ported framework support). So I'd put such definition in the ruleset. But in this case we'll have multiple rulesets. I'd say it just reflects real picture: we really have different rules for different Magento versions.
2. Restrictions specific to PHP version should be declared in the ruleset itself because it's not a product (developer) decision, this is how the PHP world works and most likely it will not change. This would require specifying PHP version for which the tests are run.
3. Can we use <config> in the ruleset to specify Magento version and PHP version (or whatever variables are needed)? So, by default we provide ruleset.xml.dist (as with other configs) and a developer can create customized ruleset.xml.dist. Or even each Magento branch can contain a ruleset, and this repo would be the source of sniffs and example rulesets. Pros: correct ruleset will work in IDE. If a developer switches between different versions of Magento, he/she can create different rulesets (if those are not part of Magento project code).