AbstractApiSniff creates infinite loop if class docblock is missing

[WesleyVestjens]

Preconditions

Confirmed on:

1. Magento 2.3.3
2. PHP 7.2
3. phpcs 3.4.2

Might reproduce on other versions as well, unconfirmed.

Steps to reproduce

1. Prepare phpcs so that the sniffer can be run
2. Create a class object with NO docblock on the class itself (example attached in comments)
3. Run phpcs --standard=Magento2 -vvv <file>

Expected result

1. Expected the test result to be outputted succesfully
2. (Optional) Expected a warning that a docblock is required (if this is the case?)

Actual result

1. An infinite loop occurs

Last lines of output with -vvv:

		Process token 72: T_NS_SEPARATOR => \
		Process token 73: T_STRING => CategoryInterface
			Processing Magento2\Sniffs\Functions\DiscouragedFunctionSniff... DONE in 0 seconds
			Processing Magento2\Sniffs\Strings\ExecutableRegExSniff... DONE in 0 seconds
			Processing Magento2\Sniffs\Security\InsecureFunctionSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Generic\Sniffs\Functions\CallTimePassByReferenceSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\DeprecatedFunctionsSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Generic\Sniffs\Functions\FunctionCallArgumentSpacingSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Generic\Sniffs\NamingConventions\UpperCaseConstantNameSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\PSR2\Sniffs\Methods\FunctionCallSignatureSniff... DONE in 0 seconds
		Process token 74: T_CLOSE_USE_GROUP => }
		Process token 75: T_SEMICOLON => ;
			Processing PHP_CodeSniffer\Standards\Generic\Sniffs\Formatting\DisallowMultipleStatementsSniff... DONE in 0 seconds
		Process token 76: T_WHITESPACE => \n
			Processing Magento2\Sniffs\Whitespace\MultipleEmptyLinesSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Squiz\Sniffs\WhiteSpace\SuperfluousWhitespaceSniff... DONE in 0 seconds
		Process token 77: T_WHITESPACE => \n
			Processing Magento2\Sniffs\Whitespace\MultipleEmptyLinesSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Squiz\Sniffs\WhiteSpace\SuperfluousWhitespaceSniff... DONE in 0 seconds
		Process token 78: T_ABSTRACT => abstract
			Processing PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\LowerCaseKeywordSniff... DONE in 0 seconds
		Process token 79: T_WHITESPACE => ·
			Processing Magento2\Sniffs\Whitespace\MultipleEmptyLinesSniff... DONE in 0 seconds
			Processing PHP_CodeSniffer\Standards\Squiz\Sniffs\WhiteSpace\SuperfluousWhitespaceSniff... DONE in 0 seconds
		Process token 80: T_CLASS => class
			Processing Magento2\Sniffs\Classes\AbstractApiSniff... ^C

[WesleyVestjens]

Example class (this is the class it is crashing on in my case):

namespace GetNoticed\CategorySlider\Block\Adminhtml\Edit\CategorySlide;

use Magento\Framework\{
    App\Request\DataPersistor,
    App\RequestInterface,
    Registry,
    UrlInterface,
    View\Element\UiComponent\Control\ButtonProviderInterface
};
use Magento\Catalog\{Api\Data\CategoryInterface};

abstract class AbstractButton implements ButtonProviderInterface
{
    /**
     * @var UrlInterface
     */
    protected $urlBuilder;

    /**
     * @var Registry
     */
    private $registry;

    /**
     * @var RequestInterface
     */
    private $request;

    /**
     * @var DataPersistor
     */
    private $dataPersistor;

    public function __construct(
        UrlInterface $urlBuilder,
        Registry $registry,
        RequestInterface $request,
        DataPersistor $dataPersistor
    ) {
        $this->urlBuilder = $urlBuilder;
        $this->registry = $registry;
        $this->request = $request;
        $this->dataPersistor = $dataPersistor;
    }

    protected function getCategoryId(): int
    {
        return $this->dataPersistor->get('category') instanceof CategoryInterface
            ? $this->dataPersistor->get('category')->getId()
            : $this->request->getParam('category_id');
    }

    protected function getSlideId(): ?int
    {
        return $this->request->getParam('slide_id');
    }

    protected function hasSlideId(): bool
    {
        return $this->getSlideId() !== null;
    }
}

[WesleyVestjens]

What seems to be happening under the hood, on line 50 AbstractApiSniff is searching for a previous doc comment open tag, which is missing (see example class). Instead of exiting there and providing the user with a warning/error/failure about a missing docblock, it goes into the for loop with values that are either NULL or FALSE, which seems to cause an infinite loop.

I didn't debug it much further to be honest, but I felt it was necessary to at least report it in an issue so somebody else can pick it up further. I am/would also not be entirely sure what to do in terms of making a docblock a required part of such a class, that would be up to whoever determines what should be a part of the coding standards.

[lenaorobei]

@Wesdesignz could you please provide version of the coding standard you are using.
It seems to me that this issue was fixed in #87

[WesleyVestjens]

magento/magento-coding-standard:1.0.2

[WesleyVestjens]

It seems I'm about 5 major versions behind, but composer is not letting me update the package at this time...

[WesleyVestjens]

composer req magento/magento-coding-standard squizlabs/php_codesniffer fixed the issue, the update wouldn't go through because squizlabs/php_codesniffer was behind.

[lenaorobei]

Happy to hear that. I assume we can close this ticket?

[WesleyVestjens]

🎊