phpcs error on rule classes - must be of the type integer #22947

Nazar65 · 2019-05-20T08:41:27Z

Description (*)

Unfortunately #22081 does in no way fix the real issue in the Sniff, but pushes the problem a few lines down in the code.

The issue is that the PHP_CodeSniffer\Files\File::findPrevious method returns either a pointer to the previous token of a type or false if none is found. This case is triggered when the CodeSniffer is running on a file with no closing comment tags.

Earlier the issue was because this false was passed to a function which expected an int instead. What #22081 does is to cast the boolean into an integer, which obviously is not correct (as the Sniff now treats the code like there is a comment closing token at position 0 when in fact there is none in the file).

Now the Sniff returns a new error as it's trying to get the closing comment from the token list:

An error occurred during processing; checking has been aborted.
The error message was: Undefined index: comment_closer in
dev/tests/static/framework/Magento/Sniffs/Annotation/ClassAnnotationStructureSniff.php on line 102

Fixed Issues (if relevant)

#20186: Issue title

Manual testing scenarios (*)

clean Magento installation on 2.3-develop branch
create a Foo.php file with the following contents

<?php

class Foo{}

run the phpcs on that file with the Magento ruleset ./vendor/bin/phpcs --standard=./dev/tests/static/framework/Magento/ Foo.php

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2019-05-20T08:41:29Z

Hi @Nazar65. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

orlangur · 2019-05-20T11:57:29Z

dev/tests/static/framework/Magento/Sniffs/Annotation/ClassAnnotationStructureSniff.php

+            $phpcsFile->addError('Comment block is missing', $stackPtr -1, 'MethodArguments');
+            return;
+        }
+        $this->validateAnnotationBlockExists($phpcsFile, $previousCommentClosePtr, (int)$stackPtr);


Is this cast to (int) still necessary?

@orlangur Nope, can i remove it ?

orlangur · 2019-05-20T11:58:11Z

dev/tests/static/framework/Magento/Sniffs/Annotation/MethodArgumentsSniff.php

@@ -663,6 +663,10 @@ private function noneParamsAligned(array $argumentPositions, array $commentPosit
        foreach ($argumentPositions as $index => $argumentPosition) {
            $commentPosition = $commentPositions[$index];
            $type = $paramDefinitions[$index]['type'];
+            if ($type == null) {


Why need this?

@orlangur Found one new issue in dev branch

Could you add more info - I'm more suspicious about the unset

@orlangur well, unset is not needed, when we have type null, we can just skip iteration.

Ok, please do so and also make a check strict (=== null).

orlangur · 2019-05-20T15:49:38Z

dev/tests/static/framework/Magento/Sniffs/Annotation/MethodArgumentsSniff.php

@@ -663,6 +663,10 @@ private function noneParamsAligned(array $argumentPositions, array $commentPosit
        foreach ($argumentPositions as $index => $argumentPosition) {
            $commentPosition = $commentPositions[$index];
            $type = $paramDefinitions[$index]['type'];
+            if ($type == null) {


Ok, please do so and also make a check strict (=== null).

Nazar65 · 2019-05-20T16:34:01Z

@orlangur done ✔️

magento-engcom-team · 2019-05-20T17:04:42Z

Hi @orlangur, thank you for the review.
ENGCOM-5139 has been created to process this Pull Request

soleksii · 2019-05-21T07:47:10Z

✔️ QA Passed

mattijv · 2019-05-21T08:42:06Z

@Nazar65 The exact same problem is present in the Magento\Sniffs\Annotation\MethodArgumentsSniff class lines 551 to 553.

You can use this class to test.

<?php

class Foo {

    public function bar()
    {
        return 0;
    }

}

The run the sniffs with ./vendor/bin/phpcs --standard=./dev/tests/static/framework/Magento/ Foo.php, which leads to the following error:

PHP Fatal error:  Uncaught TypeError: Argument 2 passed to
Magento\Sniffs\Annotation\MethodArgumentsSniff::validateCommentBlockExists()
must be of the type integer, boolean given

dev/tests/static/framework/Magento/Sniffs/Annotation/ClassAnnotationStructureSniff.php

orlangur · 2019-05-22T12:49:54Z

dev/tests/static/framework/Magento/Sniffs/Annotation/ClassAnnotationStructureSniff.php

@@ -97,8 +97,12 @@ public function process(File $phpcsFile, $stackPtr)
    {
        $tokens = $phpcsFile->getTokens();
        $previousCommentClosePtr = $phpcsFile->findPrevious(T_DOC_COMMENT_CLOSE_TAG, $stackPtr - 1, 0);
-        $this->validateAnnotationBlockExists($phpcsFile, (int)$previousCommentClosePtr, (int)$stackPtr);
-        $commentStartPtr = (int)$phpcsFile->findPrevious(T_DOC_COMMENT_OPEN_TAG, $stackPtr - 1, 0);
+        if ($previousCommentClosePtr === false) {


orlangur · 2019-05-22T12:50:01Z

dev/tests/static/framework/Magento/Sniffs/Annotation/MethodAnnotationStructureSniff.php

@@ -45,6 +45,10 @@ public function process(File $phpcsFile, $stackPtr)
        $tokens = $phpcsFile->getTokens();
        $commentStartPtr = $phpcsFile->findPrevious(T_DOC_COMMENT_OPEN_TAG, ($stackPtr), 0);
        $commentEndPtr = $phpcsFile->findPrevious(T_DOC_COMMENT_CLOSE_TAG, ($stackPtr), 0);
+        if ($commentStartPtr === false) {


orlangur · 2019-05-22T12:50:15Z

dev/tests/static/framework/Magento/Sniffs/Annotation/MethodArgumentsSniff.php

@@ -550,8 +550,12 @@ public function process(File $phpcsFile, $stackPtr)
        $numTokens = count($tokens);
        $previousCommentOpenPtr = $phpcsFile->findPrevious(T_DOC_COMMENT_OPEN_TAG, $stackPtr - 1, 0);
        $previousCommentClosePtr = $phpcsFile->findPrevious(T_DOC_COMMENT_CLOSE_TAG, $stackPtr - 1, 0);
-        if (!$this->validateCommentBlockExists($phpcsFile, $previousCommentClosePtr, $stackPtr)) {
-            $phpcsFile->addError('Comment block is missing', $stackPtr, 'MethodArguments');
+        if ($previousCommentClosePtr !== false && $previousCommentOpenPtr !== false) {


Nazar65 · 2019-05-22T13:02:07Z

@orlangur done ✔️

magento-engcom-team · 2019-05-22T13:06:45Z

Hi @orlangur, thank you for the review.
ENGCOM-5139 has been created to process this Pull Request

soleksii · 2019-05-23T07:02:34Z

✔️ QA Passed

…magento#22947

m2-assistant · 2019-05-24T10:56:26Z

Hi @Nazar65, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added the Release Line: 2.3 label May 20, 2019

Nazar65 requested review from orlangur and p-bystritsky and removed request for orlangur May 20, 2019 09:34

orlangur requested changes May 20, 2019

View reviewed changes

ghost assigned orlangur May 20, 2019

ghost added the Progress: needs update label May 20, 2019

orlangur requested changes May 20, 2019

View reviewed changes

orlangur approved these changes May 20, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels May 20, 2019

soleksii self-assigned this May 21, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels May 21, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels May 21, 2019

p-bystritsky self-assigned this May 21, 2019

Nazar65 added the Progress: needs update label May 21, 2019

mattijv suggested changes May 21, 2019

View reviewed changes

dev/tests/static/framework/Magento/Sniffs/Annotation/ClassAnnotationStructureSniff.php Show resolved Hide resolved

Nazar65 removed the Progress: needs update label May 21, 2019

p-bystritsky approved these changes May 22, 2019

View reviewed changes

orlangur requested changes May 22, 2019

View reviewed changes

ghost added Progress: needs update and removed Progress: extended testing labels May 22, 2019

phpcs error on rule classes - must be of the type integer

9884254

orlangur approved these changes May 22, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels May 22, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels May 22, 2019

mattijv approved these changes May 22, 2019

View reviewed changes

ghost added Progress: extended testing and removed Progress: testing in progress labels May 23, 2019

magento-engcom-team added Progress: testing in progress and removed Progress: extended testing labels May 23, 2019

ghost removed the Progress: testing in progress label May 23, 2019

magento-engcom-team added the Progress: testing in progress label May 23, 2019

ghost added the Progress: accept label May 23, 2019

magento-engcom-team removed the Progress: accept label May 23, 2019

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request May 24, 2019

ENGCOM-5139: phpcs error on rule classes - must be of the type integer …

c02f74b

…magento#22947

magento-engcom-team merged commit 9884254 into magento:2.3-develop May 24, 2019

magento-engcom-team added this to the Release: 2.3.3 milestone May 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

phpcs error on rule classes - must be of the type integer #22947

phpcs error on rule classes - must be of the type integer #22947

Nazar65 commented May 20, 2019 •

edited

Loading

m2-assistant bot commented May 20, 2019

orlangur May 20, 2019

Nazar65 May 20, 2019

orlangur May 20, 2019

Nazar65 May 20, 2019 •

edited

Loading

orlangur May 20, 2019

Nazar65 May 20, 2019

orlangur May 20, 2019

orlangur May 20, 2019

Nazar65 commented May 20, 2019

magento-engcom-team commented May 20, 2019

soleksii commented May 21, 2019

mattijv commented May 21, 2019

orlangur May 22, 2019

orlangur May 22, 2019

orlangur May 22, 2019

Nazar65 commented May 22, 2019

magento-engcom-team commented May 22, 2019

soleksii commented May 23, 2019

m2-assistant bot commented May 24, 2019

phpcs error on rule classes - must be of the type integer #22947

phpcs error on rule classes - must be of the type integer #22947

Conversation

Nazar65 commented May 20, 2019 • edited Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

m2-assistant bot commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nazar65 May 20, 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

Choose a reason for hiding this comment

Nazar65 commented May 20, 2019

magento-engcom-team commented May 20, 2019

soleksii commented May 21, 2019

mattijv commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nazar65 commented May 22, 2019

magento-engcom-team commented May 22, 2019

soleksii commented May 23, 2019

m2-assistant bot commented May 24, 2019

Nazar65 commented May 20, 2019 •

edited

Loading

Nazar65 May 20, 2019 •

edited

Loading