Generic InlineControlStructureSniff can create parse error when case/if/elseif/else have mixed brace and braceless definitions

[madflow]

Hi,

bin/phpcbf --version
PHP_CodeSniffer version 2.5.1 (stable) by Squiz (http://www.squiz.net)

I tried to clean the following function:

bin/phpcbf --standard=PSR2 test.php

<?php
// test.php

function sth($data, $type)
{
    switch ($type) {
    case Format::NONE:
        return true;
    case Format::DATE:
    case Format::TIME:
    case Format::DATETIME:
    case Format::DATETIMEU:
        if (empty($data)) { return true; 
        }
        elseif (!is_object($data)) {
            $datetime = date_create($data);
            return ($datetime!==false);
        }
        elseif ($data instanceof \DateTime) 
        return true;
    else {
        throw new FormatException('Unknown datetime object: ' . get_class($data));
    }
            break;
    default:
        throw new FormatException('Unknown format: ' . print_r($type, true));
    }
}

This results in:

Processing test.php [PHP => 199 tokens in 26 lines]... DONE in 8ms (12 fixable violations)
=> Fixing file: 0/12 violations remaining [made 6 passes]... DONE in 51ms
Patched 1 file
Time: 134ms; Memory: 4.75Mb

➜ /tmp php -l test.php
PHP Parse error: syntax error, unexpected 'else' (T_ELSE) in test.php on line 20
Errors parsing test.php

I did the same with

PHP_CodeSniffer version 2.3.4 (stable) by Squiz (http://www.squiz.net)

=> this does not create a parse error.

[aik099]

Yes, there seem to be a syntax error. Maybe code you're testing (that file specifically) is loaded via autoloader somehow?

If that is, then it's 2nd occasion, when including Composer auto-loader inside CLI.php of PHP_CodeSniffer has backfired.

[gsherwood]

Maybe code you're testing (that file specifically) is loaded via autoloader somehow?

I think the report is actually saying that PHPCBF fixed the file incorrectly. This doesn't have anything to do with autoloading.

[gsherwood]

In your code, the elseif ($data instanceof \DateTime) doesn't contain any braces while the rest of the IF statement does. I think this is where PHPCS is being tripped up, but only when inside a case statement.

[gsherwood]

I figured out the problem, although it is the very definition of an edge case, and I have no idea how to solve it.

With this code snippet:

<?php
switch ($type) {
    case 1:  // line 3
        if ($foo) {
            return true;
        } elseif ($baz) // line 6
            return true; // line 7
        else {
            echo 'else';
        }
    break;
}

The return on line 7 is being assigned as the closer for the case on line 3 because the elseif on line 6 doesn't have any braces and so PHPCS walks back to the ( after the elseif and continues processing from there.

I can't find a more simple test case than this as it seems the combination of the mixed braces/non-braces and the use of a case statement is required.

[gsherwood]

I've fixed this case without breaking any other tests by adding an exception for it in the tokenizer. Your code is now fixed like this:

<?php
// test.php

function sth($data, $type)
{
    switch ($type) {
        case Format::NONE:
            return true;
        case Format::DATE:
        case Format::TIME:
        case Format::DATETIME:
        case Format::DATETIMEU:
            if (empty($data)) {
                return true;
            } elseif (!is_object($data)) {
                $datetime = date_create($data);
                return ($datetime!==false);
            } elseif ($data instanceof \DateTime) {
                return true;
            } else {
                throw new FormatException('Unknown datetime object: ' . get_class($data));
            }
            break;
        default:
            throw new FormatException('Unknown format: ' . print_r($type, true));
    }
}

Thanks for reporting.

[madflow]

👍