Description

LiteralNamespacesSniff needs to be covered with unit test.

Acceptance Criteria

1. Test is added to the Magento/Tests/PHP directory and extend AbstractSniffUnitTest.
2. Positive as well as negative scenarios are covered by unit test. In other words, LiteralNamespacesUnitTest.inc should contain "error" code as well as example of "good" code.

Additional information

See already implemented unit tests under Magento/Tests directory.
Check your code by running

$ bin/phpunit

Preconditions

Have PHP CodeSniffer installed in the project.

Steps to reproduce

1. Require magento/magento-coding-standard
2. Run vendor/bin/phpcs -i

Expected result

1. Magento coding Standard is present in the list of installed standards.

Actual result

1. Magento coding Standard is missing in the list of installed standards.

How to Fix

Remove composer bin-dir, fix scripts to run vendor/bin/phpcs and change README.md.

Problem overview

The future direction of magento/magento-coding-standard project is to deliver unified Coding Standard to the Magento core.
For backward compatibility dev/tests/static/framework/Magento/ruleset.xml should not be removed. In this case both coding standards will have name Magento and PHP CodeSniffer will be not able to process it.

Solution

Rename Magento magento/magento-coding-standard to MagentoCS. So dev/tests/static/framework/Magento/ruleset.xml will look like:

<ruleset name="Magento">
    <description>Custom Magento coding standard.</description>
    <rule ref="MagentoCS"/>
</ruleset>

Hello!
While i was trying to clone the project from: [email protected]:magento/magento-coding-standard.git i got a permission denied.

Rafael:code_sniffer shero1$ git clone [email protected]:magento/magento-coding-standard.git
Cloning into 'magento-coding-standard'...
Warning: Permanently added the RSA host key for IP address '192.30.253.112' to the list of known hosts.
[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Description

This unified coding standard will contain all Magento related sniffs, so there is no need to keep core sniffs under Magento 2 dev/tests/static folded.

Acceptance Criteria

1. Sniffs are removed from Magento 2 dev/tests/static/framework/Magento folder.
2. magento/magento-coding-standard depencency is added to the require-dev section of Magento 2 composer.json file.
3. No duplicate findings detected by other static tools (eg: Magento\TestFramework\Utility\XssOutputValidatorTest covers the same as XssTemplateSniff, FinalImplementation Mess Detector rule and FinalImplementation sniff).
4. Static build is green.

Additional information

Needs to be done after the first Magento Coding Standard release.

Description

LineLengthUnitTest needs to be covered with unit test.

Acceptance Criteria

1. Test is added to the Magento/Tests/Files directory and extend AbstractSniffUnitTest.
2. Positive as well as negative scenarios are covered by unit test. In other words, LineLengthUnitTest.inc should contain "error" code as well as example of "good" code.

Additional information

See already implemented unit tests under Magento/Tests directory.
Check your code by running

$ bin/phpunit

Rule

The use of literal logical operators (and, or) is discouraged. && and || should be used instead.

Reason

Operator precedence is different for and vs. && and or vs. ||.
It may cause unexpected behaviour:

$x = false || true;  // is ($x = (false || true)) => true
$x = false or true;  // is (($x = false) or true) => false
    
$x = true && false;  // is ($x = (true && false)) => false
$x = true and false; // is (($x = true) and false) => true

Please use the official documentation for more details.

Implementation

Add Squiz.Operators.ValidLogicalOperators to the ruleset.xml.

Describe why the use of Object Manager is considered as a bad practice.

Acceptance criteria

1. ObjectManagerSniff.md file is created.
2. Explanation about why the use of Object Manager is discouraged.
3. Good and bad code snippets provided.

ExtDN's TemplateObjectManagerSniff.md can be used as a good example.

Rule

PHPDoc formatting for class properties.

Acceptance Criteria:

• Sniff covers described on DevDocs requirements:

Class attributes must have type declaration using @var tag.

• Rule covered by unit test.

<severity>5</severity>
<type>warning</type>

Rule

Function/method call in for loop declaration MUST be avoided.

Reason

PHP will call the method/function on each iteration of the for loop.

Bad Example:

for ($i = 0; $i < count($array); $i++){
    //code goes here
}

Good example:

$count = count($array);
for ($i = 0; $i < $count; $i++){
    //code goes here
}

Implementation

• Subscribe to the T_FOR token.
• Check if condition contains function call (T_STRING with followed by open parentheses).

Background

When creating a Block class, a Block class could set its PHTML template in multiple ways: Through XML layout, through a call to $this->setTemplate() and through a variable $_template. The new design of Block classes is to configure them at constructor time, meaning that configuration options (like the template) are added using constructor arguments. This allows for the XML layout to change the template. The template in the Block class is then only defined as a default value, if the XML layout is not overriding the template: This default value is best defined via a protected variable $_template.

Reason

If $this->setTemplate() is used instead, this could lead to potential issues: First of all, setters are deprecated in Block classes (because constructor arguments should be preferred instead). Second, if $this->setTemplate() is added to the constructor after calling upon the parent constructor, it would undo the configuration via XML layout. Simply put: It is outdated and leads to issues quickly.

Implementation

ExtDn SetTemplateInBlockSniff.

Rule

Abstract classes MUST NOT be marked as public @api.

Reason

Source: Magento Technical Guidelines.

Interfaces should be used instead because Magento follows Composition over Inheritance principle.

Implementation

• Subscribe to T_CLASS token.
• Analyse its PHPDoc block and try find @api annotation.

Description

PHP CodeSniffer goes out-of-box with beautifier tool (phpcbf). To use this functionality sniff should throw fixable warning/error and provide fix (tokens replacement, content change, etc).
Now rules under Magento/Annotation raise regular warnings so annotation issues cannot be fixes automatically by phpcbf.

Expected behavior

The sniff throws fixable warning instead of warning and provides fix for an issue it detects.

Benefits

Annotation issues will be fixable automatically by phpcbf tool.

Additional information

Useful links:

1. Coding Standard Tutorial
2. Beautifier Tool

Rule

PHPDoc formatting for functions and methods.

Acceptance Criteria:

• Sniff covers described on DevDocs requirements:

Functions and methods should have:

A short description in case it adds meaningful information beyond the method name.

If the purpose of the method is not obvious, a long description that explains the motivation behind the implementation. The comment must describe why method is implemented and not how. For example:

• If a workaround or hack is implemented, explain why it is necessary and include any other details necessary to understand the algorithm.
• For non-obvious implementations where the implementation logic is complicated or does not correspond to the Technical Vision or other known best practices, include an explanation in the doc block's description. An implementation is non-obvious if another developer has questions about it.
  The declaration of all arguments (if any) using @param tag, unless the argument type is indicated in the method signature. All @param annotations must include the appropriate argument type. If any argument requires a @param annotation, all arguments must be listed (all or none).

The @param annotations must be in the same order as the method arguments.

The declaration of the return type using the @return tag must only be added if the method return type signature does not supply all necessary information (see below for more information on return types).

Declaration of possibly thrown exception using @throws tag, if the actual body of function triggers throwing an exception. All occurrences of @throws in a DocBlock must be after any @param and @return annotations.

Exceptions to these rules:

• Testing methods in Unit tests may have doc blocks to describe the purpose of the test, for example referencing github issues.
• Test method annotations may include data providers and other testing annotations.
• Non-testing methods may have a doc block with description according to the rules above.

The @inheritdoc tag SHOULD NOT be used. If a child class method requires a long description to explain its purpose, it may use @inheritdoc to indicate the new description is intended as an addition to the parent method description. In general such method overrides are a code smell and should be used as an incentive to make the code more self-documenting if possible.

• Rule covered by unit test.

<severity>5</severity>
<type>warning</type>

Marketplace Technical Review

PHP CodeSniffer is one of the tools Magento Marketplace uses for extensions verification. Find more details about current approach here.
Severities is the way to determine how strict the rule is. The higher is the severity the stricter the rule is.

⚠️ Note: only severity 10 issues lead to extension rejection.

Proposed Severities Definition

Proposed Rules Severities

The Magento standard contains 96 sniffs.

Fixable column identifies whether issue can be fixed by phpcbf tool

• ✔️ yes
• ✖️ no
• ✔️ ✖️ partially

Preconditions

1. magento/magento-coding-standard 1.0.2
2. PHP_CodeSniffer version 3.3.2 (stable) by Squiz (http://www.squiz.net)
3. PHP 7.2.15-0ubuntu0.18.04.2 (cli) (built: Mar 22 2019 17:05:14) ( NTS )

Steps to reproduce

1. magento-coding-standard/vendor/bin/phpcs --standard=Magento2 --ignore=magento-coding-standard ./
2. File content of AbstractArray.php

<?php

namespace LimeSoda\Framework\Option;

abstract class AbstractArray implements OptionArrayInterface
{
}

Expected result

1. Warnings about missing comments, ...
2. No error

Actual result

FILE: /test/magento2-framework/Option/AbstractArray.php
----------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------
 1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: Undefined index: comment_closer in
   |       | /test/magento2-framework/magento-coding-standard/Magento2/Sniffs/Classes/AbstractApiSniff.php on line 51
----------------------------------------------------------------------------
Time: 173ms; Memory: 6Mb

Preconditions

Together with this coding standard I'm trying to use this coding standard https://github.com/slevomat/coding-standard for checking php7 features.
Unfortunatelly this library requires codesniffer ^3.4.2, but magento coding standard requires codesnifer ~3.3.0 which means less the 3.4.0.

What about supporting Codesniffer 3.4 ?

Rule

If a method uses system resources (such as files, sockets, streams, etc), the code MUST be wrapped with a try block and the corresponding finally block. In the finally sections, all resources SHOULD be properly released.

Reason

Source: Magento Technical Guidelines.

Implementation

• Subscribe to T_STRING token.
• Prepare the list of build in PHP functions that use resources or alternatively functions prefixes (stream_, socket_, etc)
• Check if token content is in_array of system functions.
• Raise a warning if no outer try statement detected.

Preconditions

magento-coding-standard v1.0.1

Steps to reproduce

1. Create some code somewhere in dev/tests/* that violates a rule like "Magento2.Exceptions.DirectThrow"

Results

The dev/tests/* directories are not excluded, so all rules are applied, even if they are not necessary for test code.
Exactly which rules should be excluded may be up for discussion, but I would assume the same exclusions for */Tests/* would apply.

Steps to reproduce

Run phpcs against following code:

<?php

class ClassWithNoAnnotation
{
    protected function methodWithNoAnnotation(){
        //code here
    }
}

Expected result

Warning about missing annotation is displayed.

Actual result

ERROR | An error occurred during processing; checking has been aborted. The error message was: Undefined index: comment_closer in magento-coding-standard/Magento/Sniffs/Annotation/MethodAnnotationStructureSniff.php on line 48 (Internal.Exception)

Rule from ruleset.xml

    <rule ref="Magento.Files.LineLength">
        <properties>
            <property name="lineLimit" value="120"/>
            <property name="absoluteLineLimit" value="120"/>
        </properties>
        <type>warning</type>
        <severity>6</severity>
    </rule>

Properties from parent Generic.Files.LineLength sniff are processed instead of changed in Magento.Files.LineLength values.

Steps to reproduce

1. Create test fixture with line length = 101.

Expected result

1. Unit test does not detect warning.

Actual result

[LINE 2] Expected 0 error(s) and 1 warning(s) in LineLengthUnitTest.inc but found 1 error(s) and 0 warning(s). The error(s) found were:
 -> Line exceeds maximum limit of 100 characters; contains 101 characters (Magento.Files.LineLength.MaxExceeded)

Discovered while implementing Unit test for LineLengthSniff.

Preconditions

1. Use fix from #74 (original implementation of the sniff is broken)

Steps to reproduce

1. in Magento2/Tests/Files/LineLengthUnitTest.inc, add this line:

   $test = "If a string exceeds 120 characters and a part of the string is Phrase('xy'), this would not be caught by the regular expression because it matches any line containing Phrase('xy') or __('xy').";
   

2. Run vendor/bin/phpunit

Expected result

1. An error as the line contains no translation but only the keywords "Phrase" and "__" inside a string, e.g.

   PHPUnit 5.7.27 by Sebastian Bergmann and contributors.
   
   ..........F....................                                   31 / 31 (100%)
   
   Time: 441 ms, Memory: 14.00MB
   
   There was 1 failure:
   
   1) Magento2\Tests\Files\LineLengthUnitTest::testSniff
   [LINE 32] Expected 0 error(s) in LineLengthUnitTest.inc but found 1 error(s). The error(s) found were:
    -> Line exceeds maximum limit of 120 characters; contains 202 characters (Magento2.Files.LineLength.MaxExceeded)
   
   /home/matthias/PhpstormProjects/magento-coding-standard/vendor/squizlabs/php_codesniffer/tests/Standards/AbstractSniffUnitTest.php:205
   
   FAILURES!
   Tests: 31, Assertions: 0, Failures: 1.
   

Actual result

1. The long string is missed:

   PHPUnit 5.7.27 by Sebastian Bergmann and contributors.
   
   ...............................                                   31 / 31 (100%)
   
   Time: 519 ms, Memory: 14.00MB
   
   OK (31 tests, 0 assertions)
   

Description

ConstantUsageSniff (Magento2.Translation.ConstantUsage) should be deleted because it encourage code duplication (what is bad practice) by message: Constants are not allowed as the first argument of translation function, use string literal instead.

Expected behavior

Don't inspect if constants used as first argument of translation function.

Benefits

Less code duplicates - less bugs.

Additional information

Seems that this Sniff and code recomendation exists because magento command bin/magento i18n:collect-phrases in current implementation couldn't extract strings from constants.

Some ideas how to fix it:

• implement constant resolving in i18n:collect-phrases (maybe by nikic/PHP-Parser) - the most obvious but difficult for implementation way;
• extract constants used in translation function that would be inspected manually by developers (with pointing to file and line) by command line option - the most easy for implementation way;
• extract all strings from files or just strings that not used in translation function by command line option;
• extract all constant strings by command line option;

Description

A WARNING is thrown for sniff "Empty FUNCTION statement detected"

This warning should not be through if the empty function is an around plugin. A common use-case for around plugins is to prevent execution of the original calling method. For example:

    public function aroundPrepareFavicon(Renderer $subject, callable $proceed)
    {
        // Do not call $proceed() to remove favicon code
    }

Expected behavior

I would expect this sniff to still run and throw a WARNING in all circumstances other than an around plugin. The sniff should not throw a WARNING for an around plugin.

A consideration may be to mandate the use of a comment for an around plugin with an empty function statement. This would ensure that the empty around plugin not calling the callable $proceed is desired. See above for an example comment which explains why this around plugin is empty.

Benefits

Empty around plugins are not only valid but this is a common use-case. The sniff should not detect this as an error. A comment within an empty around plugin would also most likely be desired and document to the user why this around function is empty.

Rule

Method chaining in class design MUST be avoided.

Reason

Source: Magento Technical Guidelines.

According to technical guidelines:

Application SHOULD be structured in compliance with the CQRS principle.

Good explanation of why method chaining contradicts this principal was described by Martin Fowler in the "Domain-Specific-Languages" book:

Command-query separation is an extremely valuable principle in programming, and I strongly encourage teams to use it. One of the consequences of using Method Chaining in internal DSLs is that it usually breaks this principle - each method alters state but returns an object to continue the chain. I have used many decibels disparaging people who don't follow command-query separation, and will do so again.

Suggested Implementation Direction

Detect return $this; in methods.

Rule

Getters SHOULD NOT change the state of an object.

Reason

Source: Magento Technical Guidelines.

Implementation

Check if method name starts with get and find class member assignment operation inside this method.

<severity>8</severity>
<type>warning</type>

Description

Magento2.PHP.DiscouragedFunction needs to be changed in order to cover different use-cases.

Expected behavior

The first part of the rule should cover following:

• The use of insecure functions triggers 9 severity warning (applicable to all codebase).
  Examples of insecure functions: exec, md5, unserialize, assert.

The first part of the rule should cover following:

• The use of discouraged functions triggers 8 severity warning (not applicable to */Test/* and */lib/*).
  Examples of discouraged functions: sizeof, print_r, ^socket_.*$, ^stream_.*$.

Rule

LocalizedException SHOULD only be thrown in the Presentation layer (Controllers, Blocks).

Source: Technical Guidelines.

Possible Implementation

Subscribe to the T_THROW token. If LocalizedException is thrown, check if file path contains Block or Controller. If not, raise a warning.

Description

LiteralNamespacesSniff should not check class_exists($className) || interface_exists($className).

Expected behavior

Sniff should be rewritten in order to make static check and not rely on loaded Magento classes and interfaces.

Benefits

1. Sniff will work without Magento instance.
2. Sniff will work in IDE.

Problem

Magento2.PHP.DateTime.Overcomplicated relies on class name only and does not any additional checks (like instanceof). This causes false-positive findings.

Solution

Remove Magento2.PHP.DateTime.Overcomplicated sniff from Magento Coding Standard.
Implement this check using static tool that can analyze extended class info (PHPStan).

Keep the ticket open to track the rule implementation.

@paliarush

Rule

Controllers SHOULD implement one of the HTTP method marker interfaces like HttpPostActionInterface (see https://github.com/magento/magento2/tree/2.3/lib/internal/Magento/Framework/App/Action).

Reason

Source: Magento DevDocs About Routing

Implementation

Check for subclasses of \Magento\Framework\App\Action\AbstractAction or classes, which implement \Magento\Framework\App\ActionInterface and check if these classes also implement one of the marker interfaces.

Important Details

This sniff should be applicable to Magento >=2.3, because older versions of Magento do not have HTTP method marker interfaces.

Description

Magento2.Classes.ObjectInstantiation Detects direct object instantiation via new keyword.

The test run of Magento2.Classes.ObjectInstantiation rule againstMagento2 codebase found >2000 issues. Some of them look like false-positive. Examples:

Direct Phrase object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \DOMXPath object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \DOMDocument object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \DateTime object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \Magento\Framework\File\Uploader object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \Zend_Validate_Alnum object instantiation is discouraged in Magento. Use dependency injection or factories instead. 
Direct \Zend_Validate_Alpha object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \Zend_Db_Expr object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct \NumberFormatter object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct DataObject object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct InputOption object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct InputArgument object instantiation is discouraged in Magento. Use dependency injection or factories instead.
Direct Table object instantiation is discouraged in Magento. Use dependency injection or factories instead.

Problem

Is there any cases when object instantiation via new keyword is allowed in Magento?

Rule

The use of final keyword is prohibited.

Reason

Magento is a highly extensible and customizable platform.
Final classes and method are not compatible with plugins and proxies.

Implementation

Detect T_FINAL token in the file.
Sniff should replace corresponding PHPMD rule.

Preconditions

Create fixture with following code:

<?= /* @noEscape */ json_encode($config) ?>

Steps to reproduce

Run Magento2 Coding Standard against this code.

Expected result

No issues detected.
/* @noEscape */ is allowed.
https://devdocs.magento.com/guides/v2.3/frontend-dev-guide/templates/template-security.html

Actual result

WARNING | Unescaped output detected.

Rule

Now I can see that this standard works for .php and .phtml files only. Any plans regarding .js, .css, .less files support ?

Preconditions

Magento2 CodingStandard is installed.

Steps to reproduce

Run against Magento2 codebase.
vendor/bin/phpcs --standard=Magento2 app/code lib/internal/ --sniffs=Magento2.Exceptions.Namespace -s

Expected result

No false-positive findings.

Actual result

>60 false-positive findings detected. Example:

FILE: app/code/Magento/OfflineShipping/Model/ResourceModel/Carrier/Tablerate/CSV/ColumnResolver.php
--------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------------------------------------------
 60 | WARNING | Namespace for ColumnNotFoundException class is not specified. (Magento2.Exceptions.Namespace.NotFoundNamespace)
 65 | WARNING | Namespace for ColumnNotFoundException class is not specified. (Magento2.Exceptions.Namespace.NotFoundNamespace)
--------------------------------------------------------------------------------------------------------------------------------------------------

Solution

Remove this rule from Coding Standard in favor of implementation that can capture class existence (PHPStan).

Rule

Proxies and interceptors MUST never be explicitly requested in constructors.

Source - Magento Technical Guidelines

Description

Proxies are used for lazy-loading. Details on DevDocs.

Proxies should be configured via di.xml file.

<type name="FastLoading">
    <arguments>
        <argument name="slowLoading" xsi:type="object">SlowLoading\Proxy</argument>
    </arguments>
</type>

Plugins (interceptors) modify the behavior of public class functions by intercepting a function call and running code before, after, or around that function call. Details on DevDocs.

Plugins should be declared via di.xml file.

<config>
    <type name="{ObservedType}">
      <plugin name="{pluginName}" type="{PluginClassName}" sortOrder="1" disabled="false" />
    </type>
</config>

Implementation

Subscribe to the T_CLASS token and analyse __constructor arguments.

<severity>8</severity>
<type>warning</type>

Description

Consider the following code:
<input type="hidden" name="<?php /* @escapeNotVerified */ echo $block->getInputElementName();?>" value="" id="<?php /* @escapeNotVerified */ echo $_id;?>"

A template with this snippet will raise the following warning:

x | WARNING | Short echo tag syntax must be used; expected "<?=" but found "<?php echo"

Expected behavior

Template XSS Security tags are here to improve the quality of code and so are EQP tests. It seems of higher value to defend XSS Security tags than to promote the use of short echo tags over it. We should not see a Warning when a XSS Security Annotation is used.

Benefits

EQP Standards would reflect better Magento's recommandations & tools.
Reports would be less bloated with Warnings with little to no value.

Additional information

I have cloned down the repo, and added the ruleset to PHPCS.

In PHPStorm, I have selected Magento2 ruleset.

When I run the inspection on a file, it shows duplicate results for the same sniff, please see attached screenshot.

Rule

Multiple isset statements SHOULD be combined into one.

Reason

Just the code style question aimed to make the codebase consistent.

Bad example:

if (isset($a) && isset($b) && isset($c)) {
    // code goes here
}

Good example:

if (isset($a, $b, $c)) {
    // code goes here
}

Implementation

Check for multiple isset statements combined with && inside one condition.

Reason

Rules under Magento/Annotation folder rely on token index instead of token content and do not verify findings (findPrevious and findNext can return false). This leads to false-positive findings and and errors during execution (#38).

Solution

1. Remove unstable Magento/Annotation sniffs.
2. Add out-of-box PHP CodeSniffer rules that cover the bigger part of functionality + add extra useful and efficient checks.

    <!--Ensures doc blocks follow basic formatting.-->
    <rule ref="Generic.Commenting.DocComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>
    
    <!--Parses and verifies the class doc comment.
        Verifies that:
      - A class DocBlock comment exists.
      - The comment uses the correct DocBlock style.
      - There are no blank lines after the class comment.
      - No tags are used in the DocBlock.-->
    <rule ref="Squiz.Commenting.ClassComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>
    
    <!--Allow tags in the aforesaid sniff.-->
    <rule ref="Squiz.Commenting.ClassComment.TagNotAllowed">
        <severity>0</severity>
        <type>warning</type>
    </rule>

    <!--Parses and verifies the doc comments for functions.-->
    <rule ref="Squiz.Commenting.FunctionComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>

    <!--Verifies that a @throws tag exists for each exception type a function throws.-->
    <rule ref="Squiz.Commenting.FunctionCommentThrowTag">
        <severity>5</severity>
        <type>warning</type>
    </rule>

    <!--Parses and verifies the variable doc comment.-->
    <rule ref="Squiz.Commenting.VariableComment">
        <severity>5</severity>
        <type>warning</type>
    </rule>

3. Implement sniffs for missing rule: @inheritdoc formatting - issue #54

Bonus

Some of the OOB sniffs detect fixable issues that can be automatically fixed by phpcbf tool.

Preconditions

• Magento.Exceptions.ThrowCatch rule implemented (#43; tested on Magento Coding Standard 2.0.0 and 1.0.1)

Steps to reproduce

• Run phpcs with --standard=Magento2 against file containing multiple catch blocks (it fails as it should):

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

namespace Magento;

/**
 * Doer
 */
class Doer
{
    /**
     * Do Something
     */
    public function doSomething()
    {
        try {
            $result = 2;
        } catch (\DummyException $e) {
            throw $e;
        } catch (\NewException $e) {
            throw $e;
        }

        return $result;
    }
}

Run phpcs with --standard=Magento2 against file containing one catch block (it passes when it should not):

<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

namespace Magento;

/**
 * Doer
 */
class Doer
{
    /**
     * Do Something
     */
    public function doSomething()
    {
        try {
            $result = 2;
        } catch (\DummyException $e) {
            throw $e;
        }

        return $result;
    }
}

Expected result

• Both files should fail Magento.Exceptions.ThrowCatch rule

Actual result

• Only file with two catch blocks fails Magento.Exceptions.ThrowCatch rule

Rule

All new PHP files MUST have strict type mode enabled by starting with declare(strict_types=1);. All updated PHP files SHOULD have strict type mode enabled. PHP interfaces SHOULD NOT have this declaration.

Reason

Source: Magento Technical Guidelines.

With PHP 7, it is possible to add type hinting to your code. However, this doesn't mean that types are actually enforced, unless strict typing is enabled by adding declare(strict_types=1) to the top of each PHP file.

PHP code becomes more robust when type hinting (argument types, return types) are added. With the declare(strict_types=1) added, there is less chance for bugs that related to type casting.

Implementation

Please refer to ExtDN an generic PHP CodeSniffer (not merged yet) implementations.

Important details

This sniff should be applicable to Magento >=2.2, because older versions of Magento supports PHP 5.5 and 5.6.
Follow the discussion about version specific sniffs for more details.

Description

EmptyLineMissedSniff needs to be covered with unit test.

Acceptance Criteria

1. Test is added to the Magento/Tests/Whitespace directory and extend AbstractSniffUnitTest.
2. Positive as well as negative scenarios are covered by unit test. In other words, EmptyLineMissedUnitTest.inc should contain "error" code as well as example of "good" code.

Additional information

See already implemented unit tests under Magento/Tests directory.
Check your code by running

$ bin/phpunit

We should add as grumphp for checking the quality of commits.

Sample Config:

parameters:
    bin_dir: "./vendor/bin"
    git_dir: "."
    hooks_dir: ~
    hooks_preset: local
    stop_on_failure: false
    ignore_unstaged_changes: false
    ascii:
        succeeded: ~
        failed: ~
    tasks:
        phpversion:
            project: '7.1'
        xmllint:
            ignore_patterns: ["src/dev"]
        yamllint:
            ignore_patterns: ["src/dev"]
        securitychecker:
            lockfile: ./src/composer.lock
            format: ~
            end_point: ~
            timeout: ~
            run_always: false
        git_blacklist:
            keywords:
            - "die("
            - "var_dump("
            - "print_r("
            - "var_export("
            - "exit;"
            whitelist_patterns:
            - /^src\/app\/(.*)/
            triggered_by: ['php']
        phpmnd:
            directory: ./src/app/code
            whitelist_patterns: []
            exclude: []
            exclude_name: []
            exclude_path: []
            extensions: []
            hint: false
            ignore_numbers: []
            ignore_strings: []
            strings: false
            triggered_by: ['php']
        phpcs:
            standard: "./src/dev/tests/static/framework/Magento/ruleset.xml"
            severity: ~
            error_severity: ~
            warning_severity: 6
            tab_width: ~
            report: summary
            report_width: ~
            whitelist_patterns:
            - /^src\/app\/code\/(.*)/
            encoding: ~
            ignore_patterns: []
            sniffs: []
            triggered_by: [php]
        phpunit:
            config_file: ./dev/tests/phpunit-no-coverage.xml
            testsuite: ~
            group: []
            always_execute: false

We using this tool for more than one year in our projects. We getting better commits and lower rejection rate of Pull Requests.

Info:
https://www.integer-net.com/magento-2-automatic-code-quality-check-with-grumphp/

To discuss:

• what should checked before commit
• if can use this tool for only this repo

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.

Description

Move RawQuerySniff from MEQP Coding Standard:

Acceptance criteria:

• sniff is moved from MEQP repo to magento-coding-standard;
• sniff is covered by unit test;
• unite test covers negative and positive scenarios.

Additional information

Type: warning.
Severity: 9.

Rule

Exceptions MUST NOT be handled in the same function where they are thrown.

Reason

Source: Magento Technical Guidelines.

Implementation

Find try-catch block and throw keyword in the same method.
Exception type must be the same.

Addtional Information

Type: warning
Severity: 8

Rule

The use of static methods is discouraged.

Reason

Source: Magento Technical Guidelines.

Plugins cannot be used with static methods, because static methods cannot be intercepted.
As a result, It hurts extensibility.

Implementation

• Subscribe to the T_STATIC token.
• If T_FUNCTION is found right to it raise a warning.

Rule

The use of slow array functions (array_merge) in loop is discouraged.

Reason

Merging arrays in a loop is slow and causes high CPU usage. Some benchmarking.

Bad example:

    $options = [];
    foreach ($configurationSources as $source) {
        // code here
        $options = array_merge($options, $source->getOptions());
    }

Good example:

    $options = [];
    foreach ($configurationSources as $source) {
        // code here
        $options[] = $source->getOptions();
    }

    // PHP 5.6+
    $options = array_merge([], ...$options);

Implementation

• Subscribe to T_STRING token and check if it's content is array_merge.
• Try to find outer loop.
• If loop is found rase a warning (need to discuss additional logic).

<severity>7</severity>
<type>warning</type>