djoos / Symfony-coding-standard

Development repository for the Symfony coding standard
MIT License
402 stars 102 forks source link

Infinity loop in ValidClassNameSniff #171

Closed yurahaid closed 5 years ago

yurahaid commented 5 years ago

Hi If run sniffer for this code

<?php

interface BaseException {}

interface FooExceptionInterface extends BaseException {};

I get infinity loop.

In Symfony\Sniffs\NamingConventions\ValidClassNameSniff::116

$class = $phpcsFile->findPrevious(T_CLASS, $stackPtr); //findPrevious search T_CLASS but in this example we have interface and findPrevious returned false
$name = $phpcsFile->findNext(T_STRING, $class); // findNext expected $class  as int but this case - $class is bool 
//and in findNext method exist loop:
 for ($i = $start; $i < $end; $i++) 
//$i is false and if execute $i++ we get alway false and this loop is infinity
wickedOne commented 5 years ago

Define one class per file - this does not apply to private helper classes that are not intended to be instantiated from the outside and thus are not concerned by the PSR-0 and PSR-4 autoload standards; https://symfony.com/doc/current/contributing/code/standards.html#structure

does this also occur when you split them up in two seperate files?

yurahaid commented 5 years ago

Yes, if have file with only

<?php

interface FooExceptionInterface extends BaseException {};

this also occur.

wickedOne commented 5 years ago

i appear to be unable to reproduce your issue... could you perhaps specify which version phpcs and which version of this repo you're using?

yurahaid commented 5 years ago

PHP_CodeSniffer version 3.4.2 repo - 3.9.0

yurahaid commented 5 years ago

File for testing, but before run sniffer - change extension from txt to php InfinityLoopTest.txt

yurahaid commented 5 years ago

My command - vendor/bin/phpcs --standard=Symfony/ruleset.xml InfinityLoopTest.php my php version - v7.1.27

wickedOne commented 5 years ago

ok, bit of a weird edge case, but fixed @ https://github.com/djoos/Symfony-coding-standard/pull/172