Skip to content

Proposal: Sniffs consolidation #51

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 15, 2019
Merged

Proposal: Sniffs consolidation #51

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89346e9
Sniffs consolidation proposal
lenaorobei Dec 3, 2018
8e21835
Sniffs consolidation proposal
lenaorobei Dec 18, 2018
3aaf976
Fixes after review
lenaorobei Jan 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions design-documents/testing/static/sniffs-consolidation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Overview
PHP CodeSniffer is a static code analysis tool that tokenizes PHP, JavaScript and CSS files to detect violations of a defined coding standard.

A coding standard in PHP CodeSniffer is a collection of sniff files combined in the `ruleset.xml` file. Each sniff checks one part of the coding standard.

Each sniff can have its own `severity` (priority) and `type` (two options available: `error` or `warning`).

## 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- [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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Each of these coding standards contain a lot of sniffs in common. However, there are also some differences in the code checking approach. For example:

- 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._
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the direction we are working in.


- 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


## Goals
1. Make it easier to run static checks for extensions developers.
2. Store all related to Magento 2 sniffs in one place.
3. Make static check consistent.

## Solution
1. Consolidate all related Magento 2 sniffs to the new GitHub repository.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@melnikovi melnikovi Feb 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


2. Create a new Composer package with `phpcodesniffer-standard` type and add it to the Magento 2 `composer.json` `reqiure-dev` section.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a standard package type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Thanks.


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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


7. Work with community on improvements to sniffs and make regular releases.