Magento Coding Standard Versioning Strategy

[lenaorobei]

Problem

The is no document that describes Magento Coding Standard versioning.

Solution

The proposal describes rules that need to be followed when making new Magento Coding Standard release.

Requested Reviewers

@buskamuza
@paliarush
@melnikovi

[buskamuza]

Move to minor.

[lenaorobei]

Moved.

[buskamuza]

Move to major.

[lenaorobei]

Moved.

[schmengler]

Why would "remove rule" be backward incompatible for Magento sniffs, but not for OOB sniffs? Both means, there will be less findings, which is technically backwards compatible

[buskamuza]

All this move to major.

[lenaorobei]

Moved.

[schmengler]

include-pattern would be a breaking change, wouldn't it?

[schmengler]

Consider to distinguish between more or less strict rules. Increasing severity or changing a sniff type from "warning" to "error" is definitely backwards incompatible. The other way around, not (similar to removing a rule)

[lenaorobei]

Thanks! That's a really good point.

[orlangur]

@schmengler

Increasing severity or changing a sniff type from "warning" to "error" is definitely backwards incompatible

Actually, not. Any rule addition or change which does not require a backward-incompatible code change to satisfy it is backward-compatible.

[paliarush]

Please address outstanding comments and ask someone from DevDocs to do "language review"

[orlangur]

Problem
The is no document that describes Magento Coding Standard versioning.

@buskamuza @paliarush @melnikovi wow, don't you think that this "unification" of sniffs went way too far? While it is just a slightly modified version of MEQP in a separate repo (which was not needed and requires additional maintenance efforts), now the next suggestion is to have SEPARATE versioning policy and SEPARATE backward compatibility policy.

Have you ever heard of https://en.wikipedia.org/wiki/Occam%27s_razor?

As I said earlier in Slack, standard evolution process must be described PRIOR to any such release. All use cases, like adding a new rule, strict following of standard for all the code should have been considered prior to any core repo change.

Not sure if I will be able to attend a meeting today, if this proposal gets merged in any form will have to prepare a proper one for one of the next meetings.

Cc: @antonkril

[lenaorobei]

Hi @orlangur!

if this proposal gets merged in any form will have to prepare a proper one for one of the next meetings.

You don't have to wait until something is merged to start discussion or working on something. Please, do not hesitate to add your constructive comments and concerns here.

FYI, the reason why unified coding standard was introduced is described here.

[orlangur]

@lenaorobei I know that, my constructive comments were ignored in #51.

TL;DR: this PR must not be merged as we do need a separate versioning and BC policy for "unified" approach. See you in 5 minutes on meeting.

[antonkril]

Why do you think we need semantic versioning? Who would be the clients for the standard that require the backward compatibility promise? How would the removal of a sniff break someone?

There are some questions that we need to answer like "Which version of magento is this standard applied for?". But I'm not sure I understand how semantic versioning of standards will help with that question.

[orlangur]

Anton, yep, exactly. We raised same questions during call yesterday and I believe found a kind of agreement that things could be much simpler with versioning approach I proposed.

The next step is: me providing it in details covering all use cases, stakeholders checking if it's a viable alternative providing additional cases/questions.

[lenaorobei]

@antonkril I think we need semantic versioning because we have responsibility for extension developers on Marketplace. If we add new rule, they need to change their code in order to pass our checks.

How would the removal of a sniff break someone?

Removal of a rule will not break anyone, that's why it is minor change.

Which version of magento is this standard applied for?

From Marketplace prospective coding standard applies to Magento 2, because the same set of rules we use for 2.0.x, 2.1.x, 2.2.x and 2.3.x compatible extensions.

[igorin]

We are not married to semver, but since it's an industry standard, we picked it to make coding standard updates self-explanatory. It's easy, generally understood, and based on specific declared rules. Other constructive suggestions are welcome, but there aren't any in this thread yet.

@orlangur our end goal is to unify the assorted static checks that pursue the same goal of enforcing coding guidelines, under a single global project and a single global policy. So we should really think of it long-term as THE sniff policy not ANOTHER policy.

P.S. "Which version of Magento is this version coding standard applicable for" is a fantastic question, something to document and communicate for sure.

[orlangur]

@lenaorobei,

If we add new rule, they need to change their code in order to pass our checks.

Yep, exactly as they are supposed to do with any Magento release - check if extension works and adjust as needed.

@igorin,

under a single global project and a single global policy. So we should really think of it long-term as THE sniff policy not ANOTHER policy.

I believe you didn't attend a meeting, unfortunately there is no recording uploaded yet. We can reuse existing release/versioning policy for other Magento components.

P.S. "Which version of Magento is this version coding standard applicable for" is a fantastic question, something to document and communicate for sure.

The answer is pretty straightforward in my approach. There should be the only one, no need in multiple release lines for Coding Standard, gosh :) 2.3.2 next release of Magento coding standard and both 2.2.10 and 2.3.2 next Magento releases compatible with it.

Other constructive suggestions are welcome, but there aren't any in this thread yet.

It takes some time.

[lenaorobei]

Proposal was updated with requested from @antonkril changes.

[fooman]

One aspect which I think the versioning strategy is not able to capture due to only running the latest version so far are differences in Magento branches.

So for example let's say we have a rule don't implement Magento\Framework\Setup\InstallSchemaInterface any longer as we have declarative schemas (for now this rule is just to illustrate). This works fine for Magento core code on the 2.3 branch, however once applied to Marketplace and extensions this can become problematic as this would now reject extensions that are potentially targeting Magento 2.1/2.2/2.3 with the same code. Also the rules could no longer be applied to the still active 2.2-develop and 2.1-develop branches.

A rule like the above would absolutely make sense for anything 2.3+ but to be able to move forward with just one latest version ie magento/magento-coding-standard * as per this proposal it should include support for running the ruleset on lower versions.

Also please tag versions as 3.0.0, 4.0.0, 5.0.0 etc so that packagist/composer can understand the release numbers.

[fooman]

An example for how the above could work

<?xml version="1.0"?>
<ruleset name="Magento22">
    <description>Magento Coding Standard applicable rules for Magento 2.2</description>
    <rule ref="Magento2">
        <exclude name="Magento2.Classes.UseDeclarativeSchema"/>
    </rule>
</ruleset>

[orlangur]

@fooman,

this can become problematic as this would now reject extensions that are potentially targeting Magento 2.1/2.2/2.3 with the same code

For violations which are unavoidable it is much more logical to ignore certain rules in certain files than maintain few standard branches.

Also the rules could no longer be applied to the still active 2.2-develop and 2.1-develop branches.

In general, they shouldn't. Just like we do not allow cleanup PRs for 2.2-develop at the moment. But we can also specify arbitrary version constraint in composer.json if we want.

Also please tag versions as 3.0.0, 4.0.0, 5.0.0 etc so that packagist/composer can understand the release numbers.

We don't need standard releases out of sync with Magento releases actually. So it makes much more sense to have version numbers like 2.3.2 or even more sense 100.0.1 - same as Magento Framework components (talked to Anton on this - he do not insist on major versions and is ok with such numbers).

Later, when 2.4.0 is out and no more cleanups acceptable in 2.3-develop the 2.3.x release line is frozen in terms of standard compatibility and new rules are only applied to 2.4.x.

[fooman]

@orlangur Your comments generally seem to be coming from the Magento Core development angle.

From my understanding this combined coding standard is meant to be used for more than that, like extension development, custom development, in other words anyone writing Magento and related code.

Arbitrarily keeping it in sync with the release cycle of magento/framework seems unnecessarily limiting to me.

[fooman]

The above proposal with a targeted ruleset extending from the current one would not be a separate branch - it would be part of the same release.

[orlangur]

@fooman,

this combined coding standard is meant to be used for more than that, like extension development, custom development, in other words anyone writing Magento and related code

Surely, I take into consideration all the use cases.

Arbitrarily keeping it in sync with the release cycle of magento/framework seems unnecessarily limiting to me

This is the only reasonable cycle. Just like extension or custom shop developer does some adoption for new Magento release, code can be checked with newest standard and adjusted as needed. If one wants to be an early standard adopter, dev version of standard can be used.

The above proposal with a targeted ruleset extending from the current one would not be a separate branch - it would be part of the same release.

Each release line - 2.3.x, 2.4.x - just specifies standard version it is compatible with. At some point we just stop bumping standard version for 2.3.x - when no more cleanups are supposed to be delivered.

[orlangur]

@paliarush and why this is merged without discussion? :)

[paliarush]

@orlangur this discussion was taking more than several weeks and we needed to unblock possibility to release new versions of the plugin. We had agreement on the latest version of the proposal between Magento team members.

Please feel free to propose changes to the current version via a new PR.

[orlangur]

@paliarush thanks for the quick response! I don't think merging proposal without raised issues addressing is a good idea. Releasing new standard versions without a proper strategy and earlier than 2.3.2 Magento release is even worser idea.

I'll propose a revert and versioning which does not contain described flaws soon.