Proposal: Sniffs consolidation #51

lenaorobei · 2018-12-03T20:04:22Z

Proposal for PHP CodeSniffer coding standards consolidation.

orlangur

To summarize, consolidation and unification is always a good idea, the only part here which looks doubtful to me is introduction of a separate repository which goes against monolithic approach of core repo. It is much better from productivity perspective and clearness on which set of rules is currently active for core (2.2, 2.3 release line etc).

MEQP may reside in separate repo, MFTF may have some deviations from core coding standard but they all must not be written from scratch but rather based on core one.

One important thing, not mentioned in proposal - whenever new rule is added to core standard, does not matter from MEQP or some other source, core codebase must be fully compliant with such rule. It does not make sense to introduce rule which is not followed and then add hundreds to thousands of phpcs suppressions.

orlangur · 2018-12-17T10:42:15Z

design-documents/testing/static/sniffs-consolidation.md

+
+## Problem Overview
+There are few Magento 2 related coding standards:
+- [Magento 2 Core sniffs](https://github.com/magento/magento2/tree/2.3-develop/dev/tests/static/framework/Magento/Sniffs)


This is the only core coding standard and it is already consolidated.

Moving sniffs to a separate repo would also allow to use them on other projects.

orlangur · 2018-12-17T10:43:58Z

design-documents/testing/static/sniffs-consolidation.md

+There are few Magento 2 related coding standards:
+- [Magento 2 Core sniffs](https://github.com/magento/magento2/tree/2.3-develop/dev/tests/static/framework/Magento/Sniffs)
+- [Magento Marketplace Extension Quality Program](https://github.com/magento/marketplace-eqp) (in short: MEQP)
+- [ExtDN PHP CodeSniffer rules](https://github.com/extdn/extdn-phpcs) (in short: Extdn)


Some MEQP sniffs were added into core before. Some additional sniffs, like mentioned ones, can be always added in the future. It would be really nice to establish process for such additon.

orlangur · 2018-12-17T10:45:20Z

design-documents/testing/static/sniffs-consolidation.md

+
+- The MEQP coding standard uses a `severity` approach (from 10 to 1). The test fails only in the case of a `severity` 10 finding (`error`). Issues with `severity` from 1 to 9 marked as a `warning` and do not cause test failure.
+
+- In the Magento 2 Core checking strategy, all findings lead to test failure. **Note:** _By default, PHP CodeSniffer assigns a severity of 5 to all errors and warnings._


Core coding standard must be as strict as possible. Would be really nice to get rid of warnings in terms of PSR-2 at some point.

This is the direction we are working in.

orlangur · 2018-12-17T10:46:30Z

design-documents/testing/static/sniffs-consolidation.md

+
+- MEQP coding standard contains some false-positive sniffs, for which the aim is just to warn developers and draw their attention to the particular code piece.
+
+- Some of Magento Core sniffs require Magento 2 instance to make a kind of `dynamic` check.


This does not really smell good, maybe report such occurrences for refactoring?

You're right. Will remove such sniffs from phpcs rules and investigate tools which work reflection and class dependencies.

orlangur · 2018-12-17T10:51:52Z

design-documents/testing/static/sniffs-consolidation.md

+
+- Some of Magento Core sniffs require Magento 2 instance to make a kind of `dynamic` check.
+
+As a result it leads to inconsistency and [code duplication](https://github.com/magento/magento2/issues/19477). Extension developers are confused about what code standard to use in their extension development and Magento 2 related projects contribution such as [MFTF](https://github.com/magento/magento2-functional-testing-framework/issues/265).


19477 has nothing to do with inconsistency. It is all about MEQP release was not tied properly to Magento 2.3.0 release.

Regarding code duplication, you mentioned earlier

Each of these coding standards contain a lot of sniffs in common

The big question here is why MEQP standard is not based on Magento 2 standard in the first place.

Same for MFTF, it's better to have exactly same standard but it is also fine if it would extend Magento 2 core standard.

The idea is to have one set of rules for Magento code modules, Magento related projects and vendor's extensions.

orlangur · 2018-12-17T10:55:36Z

design-documents/testing/static/sniffs-consolidation.md

+3. Make static check consistent.
+
+## Proposal
+1. Consolidate all related Magento 2 sniffs to the new GitHub repository.


Disagree with that. It is much easier and more effective to manage Core Coding Standard as a part or Core Repo in https://github.com/magento/magento2/tree/2.3-develop/dev/tests/static/framework/Magento.

However, it would be good to have it versioned and available separately as any other core component. Ideally would be to have it as separate read-only git subtree split.

Small addition on this: I believe that best place for such standard is somewhere near https://github.com/magento/magento2/tree/2.3-develop/lib/internal/Magento/Framework/TestFramework.

Some Magento/Framework/PhpcsCodingStandard or Magento/Framework/CodeSniffer as an independent splittable component. Should by aligned with other framework-splitting activities currently performed.

Versioning being discussed in this proposal magento/magento-coding-standard#17. Coding standard should not be part of application framework. It's more related to dev tools than to components you use in your application business logic.

orlangur · 2018-12-17T10:56:15Z

design-documents/testing/static/sniffs-consolidation.md

+
+3. Assign severity to each sniff. The higher the severity, the more strict the rule is.
+
+4. Create separate `ruleset.xml` files (set of sniffs) for Magento Core and Magento Extension Quality Program (reasons in the Problem Overview section).


I don't see this is different from current situation.

orlangur · 2018-12-17T10:59:33Z

design-documents/testing/static/sniffs-consolidation.md

+
+5. For sniffs that require Magento 2 instance use [bootstrap file](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-bootstrap-file).
+
+6. Revisit other Magento 2 static tests (`PHPMD` rules and covered by `PHPUnit` static tests) and if it's possible rewrite them to use PHP CodeSniffer.


Yeah, sounds really good, I proposed this when there was the only custom design rule related to final keyword.

What do you think about deeper php-cs-fixer integration? As to me, it would be really nice to update https://github.com/magento/magento2/blob/2.3-develop/.php_cs.dist with as much existing useful rules as possible and enforce code similar to phpcs and phpmd.

Integration of PHP CS Fixer sounds good, can be investigated more in the future proposals.

lenaorobei · 2018-12-18T20:01:14Z

@orlangur thank you for your input, however after discussion with Magento architecture team we came with the decision to create separate GitHub repository and start the consolidation process. We decided to keep one ruleset.xml for now. I will update the corresponding section of the proposal.
Some thoughts you've mentioned look reasonable for me, so I encourage you to participate in the discussion and implementation of the new unified coding standard.

orlangur · 2018-12-18T22:55:07Z

@lenaorobei yeah, I saw https://github.com/magento/magento-coding-standard created. Too bad...

Having custom process instead of unified one for such a simple thing looks like a real mess to me.

Asked @buskamuza for meeting minutes of corresponding decision-making meeting.

buskamuza · 2018-12-20T16:56:30Z

@orlangur , everything can be discussed here. This is why this proposal exists. And everything will be discussed on one of the open Architectural meetings.

I don't see any issues with a separate repo for code style rules. While, I also don't see how coding standard is a part of Magento Framework. It doesn't provide any application functionality, and it should be possible to install it on its own, w/o entire Magento Framework.

If you have specific concerns, please list them here, so we can address each of them in more details.

buskamuza · 2019-01-03T23:29:45Z

design-documents/testing/static/sniffs-consolidation.md

+
+4. Create one `ruleset.xml` file (set of sniffs) for both projects - Magento Core and Magento Extension Quality Program.
+
+5. For sniffs that require Magento 2 instance use [bootstrap file](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-bootstrap-file).


Could you please provide references to such sniffs? I agree with @orlangur that ideally we shouldn't have such ones, but let's review.

Here is the example: https://github.com/magento/magento2/blob/2.3-develop/dev/tests/static/framework/Magento/Sniffs/LiteralNamespaces/LiteralNamespacesSniff.php#L71

Before throw an error the sniff checks class_exists($className) || interface_exists($className).

Ok. Thanks.

buskamuza · 2019-01-03T23:30:15Z

design-documents/testing/static/sniffs-consolidation.md

+## Proposal
+1. Consolidate all related Magento 2 sniffs to the new GitHub repository.
+
+2. Create a new Composer package with  `phpcodesniffer-standard` type and add it to the Magento 2 `composer.json` `reqiure-dev` section.


Is this a standard package type?

Yes, it's standard package type. It allows the package to be automatically processed by composer install plugins. The user doesn't have to manually run phpcs --config-set command to configure coding standard.

buskamuza · 2019-01-03T23:30:47Z

design-documents/testing/static/sniffs-consolidation.md

+2. Store all related to Magento 2 sniffs in one place.
+3. Make static check consistent.
+
+## Proposal


The PR is a proposal. Please rephrase it so that it looks as a final document with a decision. E.g., "Solution"

orlangur · 2019-01-04T12:57:39Z

@buskamuza,

If you have specific concerns, please list them here

They are listed above ;)

Sniffs consolidation proposal

89346e9

paliarush approved these changes Dec 4, 2018

View reviewed changes

orlangur suggested changes Dec 17, 2018

View reviewed changes

Sniffs consolidation proposal

8e21835

buskamuza reviewed Jan 3, 2019

View reviewed changes

Fixes after review

3aaf976

buskamuza approved these changes Jan 4, 2019

View reviewed changes

lenaorobei mentioned this pull request Jan 7, 2019

January 9, 2019 #55

Closed

buskamuza assigned paliarush Jan 25, 2019

melnikovi merged commit c4cebc5 into magento:master Feb 15, 2019

orlangur mentioned this pull request Apr 17, 2019

Magento Coding Standard Versioning Strategy #136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Sniffs consolidation #51

Proposal: Sniffs consolidation #51

lenaorobei commented Dec 3, 2018

orlangur left a comment

orlangur Dec 17, 2018

melnikovi Feb 15, 2019

orlangur Dec 17, 2018

orlangur Dec 17, 2018

lenaorobei Feb 15, 2019

orlangur Dec 17, 2018

lenaorobei Feb 15, 2019

orlangur Dec 17, 2018

lenaorobei Feb 15, 2019

orlangur Dec 17, 2018

orlangur Dec 17, 2018

melnikovi Feb 15, 2019 •

edited

Loading

orlangur Dec 17, 2018

orlangur Dec 17, 2018

melnikovi Feb 15, 2019

lenaorobei commented Dec 18, 2018 •

edited

Loading

orlangur commented Dec 18, 2018

buskamuza commented Dec 20, 2018

buskamuza Jan 3, 2019

lenaorobei Jan 4, 2019

buskamuza Jan 4, 2019

buskamuza Jan 3, 2019

lenaorobei Jan 4, 2019

buskamuza Jan 3, 2019

orlangur commented Jan 4, 2019


		- The MEQP coding standard uses a `severity` approach (from 10 to 1). The test fails only in the case of a `severity` 10 finding (`error`). Issues with `severity` from 1 to 9 marked as a `warning` and do not cause test failure.

		- In the Magento 2 Core checking strategy, all findings lead to test failure. Note: _By default, PHP CodeSniffer assigns a severity of 5 to all errors and warnings._


		- MEQP coding standard contains some false-positive sniffs, for which the aim is just to warn developers and draw their attention to the particular code piece.

		- Some of Magento Core sniffs require Magento 2 instance to make a kind of `dynamic` check.


		- Some of Magento Core sniffs require Magento 2 instance to make a kind of `dynamic` check.

		As a result it leads to inconsistency and [code duplication](https://github.com/magento/magento2/issues/19477). Extension developers are confused about what code standard to use in their extension development and Magento 2 related projects contribution such as [MFTF](https://github.com/magento/magento2-functional-testing-framework/issues/265).


		3. Assign severity to each sniff. The higher the severity, the more strict the rule is.

		4. Create separate `ruleset.xml` files (set of sniffs) for Magento Core and Magento Extension Quality Program (reasons in the Problem Overview section).


		5. For sniffs that require Magento 2 instance use [bootstrap file](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-bootstrap-file).

		6. Revisit other Magento 2 static tests (`PHPMD` rules and covered by `PHPUnit` static tests) and if it's possible rewrite them to use PHP CodeSniffer.


		4. Create one `ruleset.xml` file (set of sniffs) for both projects - Magento Core and Magento Extension Quality Program.

		5. For sniffs that require Magento 2 instance use [bootstrap file](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-bootstrap-file).

Proposal: Sniffs consolidation #51

Proposal: Sniffs consolidation #51

Conversation

lenaorobei commented Dec 3, 2018

orlangur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melnikovi Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenaorobei commented Dec 18, 2018 • edited Loading

orlangur commented Dec 18, 2018

buskamuza commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orlangur commented Jan 4, 2019

melnikovi Feb 15, 2019 •

edited

Loading

lenaorobei commented Dec 18, 2018 •

edited

Loading