Skip to content

Display user friendly message instead of a fatal error when unable to change a sniff property #905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
2 tasks done
rodrigoprimo opened this issue Mar 26, 2025 · 1 comment

Comments

@rodrigoprimo
Copy link
Contributor

rodrigoprimo commented Mar 26, 2025

Is your feature request related to a problem?

PHPCS displays a fatal error when the user inadvertently tries to change a sniff property that can't be changed from outside the class. Consider the ruleset.xml file below:

<?xml version="1.0"?>
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="phpcs.xsd">
    <rule ref="Generic.PHP.ForbiddenFunctions">
        <properties>
            <property name="patternMatch" value="true"/>
        </properties>
    </rule>
</ruleset>

It results in the following PHP fatal error:

PHP Fatal error:  Uncaught Error: Cannot access protected property PHP_CodeSniffer\Standards\Generic\Sniffs\PHP\ForbiddenFunctionsSniff::$patternMatch in src/Ruleset.php:1620
Stack trace:
#0 src/Ruleset.php(1449): PHP_CodeSniffer\Ruleset->setSniffProperty()
#1 src/Ruleset.php(250): PHP_CodeSniffer\Ruleset->populateTokenListeners()
#2 src/Runner.php(348): PHP_CodeSniffer\Ruleset->__construct()
#3 src/Runner.php(76): PHP_CodeSniffer\Runner->init()
#4 bin/phpcs(14): PHP_CodeSniffer\Runner->runPHPCS()
#5 {main}
  thrown in src/Ruleset.php on line 1620

Describe the solution you'd like

I want to suggest that instead of a fatal error, PHPCS display a user-friendly error message alerting that the property cannot be changed.

It needs to be determined in which cases PHPCS cannot change the sniff property. Here is a non-exhaustive list:

  • The property is not public and the __set() magic method does not exist in the sniff class.
  • The property is public but read-only (PHP >= 8.1).

Additional context

Here is the code that checks if the property exists and displays an error if it doesn't:

if (property_exists($sniffObject, $propertyName) === true
|| ($sniffObject instanceof stdClass) === true
|| method_exists($sniffObject, '__set') === true
) {
$isSettable = true;
}
if ($isSettable === false) {
if ($settings['scope'] === 'sniff') {
$notice = "Property \"$propertyName\" does not exist on sniff ";
$notice .= array_search($sniffClass, $this->sniffCodes, true).'.';
$this->msgCache->add($notice, MessageCollector::ERROR);
}
return;
}

@jrfnl
Copy link
Member

jrfnl commented Mar 26, 2025

I suppose a friendlier error message would be nice, but changing non-public properties was never supported and has never been a feature of PHPCS, so I'm not too concerned about people getting a fatal about that. I've also never seen anyone report seeing that fatal or even asking for support because of it.

The code you are referring to above was introduced to handle the PHP 8.2 dynamic properties deprecations and dynamic properties are public by nature, so that's a completely different matter.

All in all, while giving people friendly error messages is a good thing, I don't think we need to accommodate every single thing people may possibly try to do, even though it's never been supported.

I.e.: very low prio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants