Constant visibility false positive on non-class constants Ver2

[goto-masaya]

@gsherwood

https://github.com/squizlabs/PHP_CodeSniffer/issues/2615

Not included in version 3.5.1. I am using version 3.5.3. When will you capture and release it?

----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
 45 | WARNING | Visibility must be declared on all constants if your
    |         | project supports PHP 7.1 or later
    |         | (PSR12.Properties.ConstantVisibility.NotFound)
 47 | WARNING | Visibility must be declared on all constants if your
    |         | project supports PHP 7.1 or later
    |         | (PSR12.Properties.ConstantVisibility.NotFound)
 49 | WARNING | Visibility must be declared on all constants if your
    |         | project supports PHP 7.1 or later
    |         | (PSR12.Properties.ConstantVisibility.NotFound)
 51 | WARNING | Visibility must be declared on all constants if your
    |         | project supports PHP 7.1 or later
    |         | (PSR12.Properties.ConstantVisibility.NotFound)
 53 | WARNING | Visibility must be declared on all constants if your
    |         | project supports PHP 7.1 or later
    |         | (PSR12.Properties.ConstantVisibility.NotFound)
----------------------------------------------------------------------

[jrfnl]

@goto-masaya Could you share the code for which you are receiving these errors ?

[goto-masaya]

@jrfnl

Thanks for your comment.

This is not an error. The warning is a bug.

The previous ISSEU says it will be resolved with the release of 3.5.1, but it has not been resolved.

[goto-masaya]

It is controlled so that push is possible on the condition that there is no warning with git hook.

I'm in trouble when a warning comes out. But this is not an error.

[jrfnl]

@goto-masaya I think you missed my question: Could you share the code for which these issues are being reported ?

Without that nobody can evaluate this ticket for validity.

[gsherwood]

I can assure you that the fix for #2615 was in 3.5.1, and is still in 3.5.3. The code posted in that report produces no errors.

That issue was reporting the warning for non-class constants. But non-class constants cannot have visibility declared, so the warning was incorrect.

I suspect you're getting this warning on class constants. As there is no way to know if your code is supposed to run on PHP 7.1 or later, the sniff throws a warning for any constants without visibility so these can be manually checked. If your code must work on older PHP versions, you can either ignore this warning or exclude it from your standard using a ruleset.xml file.

You haven't posted any code I can use to replicate the issue you are seeing, so I'm just guessing. If you can post some code, I can confirm if the sniff is working correctly.

[goto-masaya]

@gsherwood

The non-class constant and class constant were incorrect. The problem axis was misaligned. sorry.

OK. I`m using php7.2. It is my rule.xml.

<?xml version="1.0"?>
<ruleset name="PSR12/Laravel">
    <description>PSR12 compliant rules and settings for Laravel</description>

    <arg name="extensions" value="php" />

    <!-- 適用コーディング規約の指定 -->
    <rule ref="PSR12" />

    <!-- 出力に色を適用 -->
    <arg name="colors" />

    <!-- 除外ディレクトおよびファイル -->
    <exclude-pattern>/bootstrap/</exclude-pattern>
    <exclude-pattern>/config/</exclude-pattern>
    <exclude-pattern>/database/</exclude-pattern>
    <exclude-pattern>/node_modules/</exclude-pattern>
    <exclude-pattern>/public/</exclude-pattern>
    <exclude-pattern>/resources/</exclude-pattern>
    <exclude-pattern>/routes/</exclude-pattern>
    <exclude-pattern>/storage/</exclude-pattern>
    <exclude-pattern>/vendor/</exclude-pattern>
    <exclude-pattern>/server.php</exclude-pattern>
    <exclude-pattern>/app/Console/Kernel.php</exclude-pattern>
    <exclude-pattern>/tests/CreatesApplication.php</exclude-pattern>
    <exclude-pattern>*/_ide_helper.php</exclude-pattern>
    <exclude-pattern>*/_ide_helper_models.php</exclude-pattern>
    <exclude-pattern>*/.phpstorm.meta.php</exclude-pattern>

    <!-- 除外ルールと対象ディレクトリおよびファイル -->
    <rule ref="PSR12.Properties.ConstantVisibility.NotFound">
        <!-- todo:: Remove bugs when they are resolved-->
        <!-- @see https://github.com/squizlabs/PHP_CodeSniffer/issues/2739 -->
        <exclude-pattern>/**/*</exclude-pattern>
    </rule>

</ruleset>

the sniff throws a warning for any constants without visibility so these can be manually checked.

Thrown for visible constants, why?

 "require-dev": {
        "barryvdh/laravel-debugbar": "^3.2",
        "barryvdh/laravel-ide-helper": "*",
        "beyondcode/laravel-dump-server": "^1.0",
        "squizlabs/php_codesniffer": "^3.5",
        "filp/whoops": "^2.0",
        "fzaninotto/faker": "^1.4",
        "mockery/mockery": "^1.0",
        "nunomaduro/collision": "^3.0",
        "phpunit/phpunit": "^7.5"
    },

[gsherwood]

Thrown for visible constants, why?

PSR12 says:

Visibility MUST be declared on all constants if your project PHP minimum version supports constant visibilities (PHP 7.1 or later).

PHPCS doesn't know if your project's min PHP version is 7.1. But if it is, you must declare visibility on all class constants. So when PHPCS finds a class constant without visibility, it throws a warning.

If your project's minimum PHP version is less than 7.1, you can't use class constant visibility. So add <exclude name="PSR12.Properties.ConstantVisibility"/> to your ruleset so the check is not included. In your case, you'd replace the PSR12 tag with:

<rule ref="PSR12">
    <exclude name="PSR12.Properties.ConstantVisibility"/>
</rule>

[goto-masaya]

@gsherwood

very thank you !

[czim]

Maybe I'm misunderstanding something here, but why isn't the same simple approach used as for PHP\DisallowAlternativePHPTagsSniff?

        if ($this->phpVersion === null) {
            $this->phpVersion = Config::getConfigData('php_version');
            if ($this->phpVersion === null) {
                $this->phpVersion = PHP_VERSION_ID;
            }
        }

        if ($this->phpVersion < 70100) {
            // ...
        }

[razvanphp]

Or can we better read this from composer?

    "require": {
        "php": ">=5.4.0",

and then skip this rule?