Preconditions

Hi folks, I recently tried upgrading the coding standards library in my custom module from version 6 to version 10 and it suddenly starts complaining about missing copyright headers in files in my module?

This is something I should decide, not something you should decide right?

This coding standards library is still meant for 3rd party modules as well, right? Not only for checking core Magento files?

Steps to reproduce

1. Have a custom module
2. Have a file etc/di.xml in that module that contains no copyright header

Expected result

1. No warnings

Actual result

1. Getting a warning:

FILE: /path/to/custom/module/etc/di.xml
----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------
  | WARNING | Copyright is missing or has wrong format
----------------------------------------------------------------------------------------------------

Preconditions

• Magento.Exceptions.ThrowCatch rule implemented (magento/magento-coding-standard#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

There are a couple of sniffs duplicated between PHPCS and PHP Compatibility. I've picked the ones with better coverage after doing some tests.

| Excluded | Kept | Description | | --- | --- | --- | | Generic.PHP.DeprecatedFunctions | PHPCompatibility.FunctionUse.RemovedFunctions | Excluded one only catches deprecations and, due to it using get_defined_functions() to get deprecated methods, it doesn't include functions coming from extensions or deprecations from different PHP versions than the one which is running the sniff. | | PHPCompatibility.Syntax.ForbiddenCallTimePassByReference | Generic.Functions.CallTimePassByReference | Both are equivalent, there is no special advantage of one over the other; in fact, call-time pass-by-reference is forbidden since PHP 5.4 so we can remove both checks. |

Preconditions

1. magento/magento-coding-standard v9
2. A magento module that has an xml file that starts with <?xml version="1.0" ?>

Steps to reproduce

1. run bin/phpcs ./ --standard=Magento2 --severity=10

Expected result

1. Nothing module passes the test

Actual result

FILE: /opt/atlassian/pipelines/agent/build/etc/module.xml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | PHP syntax error: syntax error, unexpected 'version'
   |       | (T_STRING)
----------------------------------------------------------------------

FILE: /opt/atlassian/pipelines/agent/build/etc/di.xml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | PHP syntax error: syntax error, unexpected 'version'
   |       | (T_STRING)
----------------------------------------------------------------------

This bug got introduced in v9 because our pipelines were running v8 yesterday and didn't have this issue at the time.

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>

Rule

in_array creating performance issue in many places when it used a lot of times.

Reason

Performance diff: https://stackoverflow.com/questions/13483219/what-is-faster-in-array-or-isset

It causes performance issues:

• https://github.com/magento/magento2/pull/24677
• https://github.com/magento/magento2/issues/20966
• https://github.com/magento/magento2/pull/4088

Implementation

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

Rule

Currently const are declared without visibility: https://github.com/magento/magento2/blob/78bb169ff9721c8d05c35b4c29a4464fd45bccbe/app/code/Magento/Catalog/Model/Product.php#L54-L73

I suggest extending Coding Standard to require visibility - for example:

public const PRIMARY_KEY = 'entity_id';
private const DEFAULT_TIMEOUT = 60;

Reason

const should be explicit if we can use them as a reference or it's declaration is only internal.

Implementation

N/A

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.

Fixes: https://github.com/magento/magento-coding-standard/issues/314

Refactored the sniff for better readability, simplified and fixed the implementation. Added additional filtering by autogenerated class suffixes (Proxy and Interceptors are not covered as they are handled by DiscouragedDependenciesSniff)

Test results on magento2 codebase:

$ vendor/bin/phpcs --standard=Magento2 --sniffs=Magento2.PHP.AutogeneratedClassNotInConstructor magento2/app/code

FILE: magento2/app/code/Magento/Ui/Controller/Adminhtml/Index/Render.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
67 | ERROR | Class \Magento\Framework\Controller\Result\JsonFactory needs to be requested in constructor, otherwise compiler will not be able to
|       | find and generate this classes
------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: magento2/app/code/Magento/CatalogSearch/Model/ResourceModel/Advanced/Collection.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
194 | ERROR | Class \Magento\Framework\Api\Search\SearchResultFactory needs to be requested in constructor, otherwise compiler will not be able to
|       | find and generate this classes
------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: magento2/app/code/Magento/CatalogSearch/Model/ResourceModel/Fulltext/Collection.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
181 | ERROR | Class \Magento\Framework\Api\Search\SearchResultFactory needs to be requested in constructor, otherwise compiler will not be able to
|       | find and generate this classes
------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: magento2/app/code/Magento/Catalog/Controller/Adminhtml/Product/Builder.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
81 | ERROR | Class \Magento\Store\Model\StoreFactory needs to be requested in constructor, otherwise compiler will not be able to find and generate
|       | this classes
------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: magento2/app/code/Magento/Rule/Model/AbstractModel.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------
495 | ERROR | Class \Magento\Framework\Api\ExtensionAttributesFactory needs to be requested in constructor, otherwise compiler will not be able to
|       | find and generate this classes
507 | ERROR | Class \Magento\Framework\Api\AttributeValueFactory needs to be requested in constructor, otherwise compiler will not be able to find
|       | and generate this classes
------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: magento2/app/code/Magento/ConfigurableProduct/Model/LinkManagement.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
208 | ERROR | Class \Magento\ConfigurableProduct\Helper\Product\Options\Factory needs to be requested in constructor, otherwise compiler will not be
|       | able to find and generate this classes
------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: magento2/app/code/Magento/Customer/Controller/Account/Logout.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
75 | ERROR | Class CookieMetadataFactory needs to be requested in constructor, otherwise compiler will not be able to find and generate this classes
------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 1 mins, 59.01 secs; Memory: 64MB

This PR addsgetimagesize() to discouraged functions as it does not work with remote storage implementations.

Task: https://jira.corp.magento.com/browse/MC-38508

Preconditions

1. Magento 2.4.5-p1
2. Magento Coding Standard, 26

Steps to reproduce

1. Create a file, where you include \Zend_Filter_Interface in the constructor
2. Run vendor/bin/phpcs --standard=dev/tests/static/framework/Magento on that file.

Expected result

1. RestrictedCodeSniff should suggest a class, which exists in a clean Magento instance

Actual result

1. The following error comes up: ERROR | Class 'Zend_Filter_Interface' is restricted in XXX. Suggested replacement: Laminas\Filter\FilterInterface

Preconditions

Add attribute in Data Patch using $eavSetup->addAttribute() method. Place DataPatch in Vendor/Module/Setup/Patch/Data/AddTestAttribute.php file.

Example

$eavSetup = $this->eavSetupFactory->create(['setup' => $this->moduleDataSetup]);
$eavSetup->addAttribute(Product::ENTITY, $attributeCode, $attribute);

Steps to reproduce

run vendor/bin/phpcs --standard=Magento2 app/code/Vendor/Module/Setup/Patch/Data/AddTestAttribute.php

Expected result

class 'Zend_Validate_Exception' is restricted in app/code/Vendor/Module/Setup/Patch/Data/AddTestAttribute.php. 
Suggested replacement: Magento\Framework\Validator\Exception

Actual result

class 'Zend_Validate_Exception' is restricted in app/code/Vendor/Module/Setup/Patch/Data/AddTestAttribute.php. 
Suggested replacement: Magento\Framework\Validator\ValidateException

Line

https://github.com/magento/magento-coding-standard/blob/097bda3e015f35dc7c2efc0b8c7a7d8dfc158a63/Magento2/Sniffs/Legacy/_files/restricted_classes.php#L129

Solution

The Magento\Framework\Validator\ValidateException should be replaced with Magento\Framework\Validator\Exception

Preconditions

Example of less file

.legal-store-information{
  color: #aaa!important;
  padding: 13px;
  text-align: center;
  font-family: Arial, sans-serif;
  font-size: 0.8em;
  display: block;

  .a{
    color: #aaa!important;

    &:hover {
      color: #fff!important;
    }
    &:visited {
      color: #aaa!important;
    }
  }
}

Steps to reproduce

1. run vendor/bin/phpcs -d memory_limit=2G --standard=Magento2 --report-\\Micheh\\PhpCodeSniffer\\Report\\Gitlab=report-phpcs.json --basepath=/var/www/html app/design/frontend/Phoenix/

Expected result

Normal report

Actual result

Fatal Error.

PHP Fatal error:  Uncaught ValueError: strpos(): Argument #3 ($offset) must be contained in argument #1 ($haystack) in /var/www/html/vendor/magento/magento-coding-standard/Magento2/Sniffs/Less/ClassNamingSniff.php:69
Stack trace:
#0 /var/www/html/vendor/magento/magento-coding-standard/Magento2/Sniffs/Less/ClassNamingSniff.php(69): strpos()
#1 /var/www/html/vendor/squizlabs/php_codesniffer/src/Files/File.php(498): Magento2\Sniffs\Less\ClassNamingSniff->process()
#2 /var/www/html/vendor/squizlabs/php_codesniffer/src/Files/LocalFile.php(92): PHP_CodeSniffer\Files\File->process()
#3 /var/www/html/vendor/squizlabs/php_codesniffer/src/Runner.php(628): PHP_CodeSniffer\Files\LocalFile->process()
#4 /var/www/html/vendor/squizlabs/php_codesniffer/src/Runner.php(434): PHP_CodeSniffer\Runner->processFile()
#5 /var/www/html/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run()
#6 /var/www/html/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS()
#7 {main}
  thrown in /var/www/html/vendor/magento/magento-coding-standard/Magento2/Sniffs/Less/ClassNamingSniff.php on line 69

As you can see from less file, a class name is shorter than the expected offset 2. A possible solution is to check the string length before checking the underscore in the className, or check if the found position is bigger than 2

Rule

Forbid calling Magento\Framework\Module\Status::setIsEnabled

Reason

Experienced downtime as a result of a magento marketplace module (feefo/reviews) calling setIsEnabled(true) in production mode. The target was Magento_Review, which was installed but not enabled, causing blocks to attempt to load data from database tables which did not exist (declarative schema had removed them). Usage of this function outwith console command handlers should result in an automated knockback from the Magento marketplace due to potential to cause downtime like this.

Implementation

ForbiddenFunctionsSniff

Steps to reproduce

1. Use Zend_Validate in your extension
2. Validate extension with the last (26) version of Magento Coding Standard

Expected result

1. Extension is not valid and there is a correct suggestion or it is not ERROR level but WARNING
2. Suggested replacement is present in 2.4.0 Magento versions as it is supported to November 28, 2022

Actual result

1. Extension not valid
2. Suggested replacement (\Magento\Framework\Validator\ValidatorChain) is missing in 2.4.0-2.4.5

Pull request #420 bring these new changes.