PSR12.Properties.ConstantVisibility should automatically detect the PHP version

[razvanphp]

Is your feature request related to a problem?

When running PSR12 against an (older) project, we see this warning message for projects that actually support PHP versions lower than 7.1.

FILE: /Volumes/Code/yii2/framework/grid/GridView.php
------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------
 52 | WARNING | Visibility must be declared on all constants if your project supports PHP 7.1 or later (PSR12.Properties.ConstantVisibility.NotFound)

In my particular case, this is defined in composer.json:

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

The php version running phpcs is not relevant, as it might run just in CI.

Describe the solution you'd like

It would be great to skip this check if the minimum version is below PHP 7.1, ideally checking this in composer.json. Also, I think this is an easy fix for phpcbf, but it's not yet implemented, it should be public as this is the default visibility if it is omitted.

Additional context (optional)

Can I please get some guidance of how this should be implemented? Saw in another issue this, but not sure it's reliable enough for this use-case:

PHP_CodeSniffer/src/Standards/Generic/Sniffs/PHP/DisallowAlternativePHPTagsSniff.php

Lines 43 to 52 in 9b6260b

	if ($this->phpVersion === null) {
	$this->phpVersion = Config::getConfigData('php_version');
	if ($this->phpVersion === null) {
	$this->phpVersion = PHP_VERSION_ID;
	}
	}
	if ($this->phpVersion < 70000) {
	$this->aspTags = (boolean) ini_get('asp_tags');
	}

• I intend to create a pull request to implement this feature.

[jrfnl]

@razvanphp Thanks for opening this issue.

It would be great to skip this check if the minimum version is below PHP 7.1, ideally checking this in composer.json.

PHPCS can not and should not rely on a composer.json file being available. PHPCS can be installed via PHIVE, git clone, stand-alone PHAR, Composer etc, so any solution which presumes the existence of a composer.json file in a project (and presumes a project-based install), will not be accepted.

Also, I think this is an easy fix for phpcbf, but it's not yet implemented, it should be public as this is the default visibility if it is omitted.

As spelled out in the CONTRIBUTING guide, PHPCS does not allow risky fixers for its native sniffs. The appropriate visibility is something which should be deliberately determined by a developer. This is not something a tool can do and making automated BC-compatible fixes just to fix the issues instead of making deliberate choices about the visibility is a risky fix, so any PR for this will not be accepted.

I'm pretty sure there are sniffs available in external standards which can make that fix for you, but it will not be made by the PHPCS native sniffs.

Can I please get some guidance of how this should be implemented ?

The code snippet shown, works based on a configured php_version and defaults to the PHP version used to run PHPCS.
The PHPCS version to run PHPCS is - in most cases - not the same as the minimum supported PHP version, so defaulting to that PHP version would not be appropriate.

[razvanphp]

ok, thanks for the detailed explanation. So no other options? I can't see a viable way to treat this case otherwise. Maybe the sniff could receive a parameter as the minimum PHP version?

[jrfnl]

So no other options?

That code snippet with php_version could be adjusted to not default to the PHP version on which PHPCS is being run. I can see that as a feasible solution, though I need to think about it a little.

Maybe the sniff could receive a parameter as the minimum PHP version?

A parameter would require a custom ruleset, in which case people can just as well use:

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

[razvanphp]

OK, for now I will just exclude it in the project phpcs.xml, but if you think about a clever solution, I'm all in to try it out. Thank you!