squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

Make ForbiddenFunctionsSniff fixable #3783

Closed RobinvanderVliet closed 1 year ago

jrfnl commented 1 year ago

For what it's worth: I do not think this is a change which should be accepted.

While the two functions being flagged by default are one-on-one replacements, this sniff is explicitly designed to be extended and the $forbiddenFunctions property can be overwritten both by an extended sniff as well as from a custom ruleset and allows for pattern matching.

Not all function replacements will be one-on-one replacements as the arguments functions take may be different. The sniff also allows for forbidding functions without suggesting a replacement.

As things are, this would make this a very risky fixer.

RobinvanderVliet commented 1 year ago

Not all function replacements will be one-on-one replacements as the arguments functions take may be different.

I thought about that too. I use the sniff to standardize the many aliased functions in PHP to a standard form. Many of those functions are one-to-one replaceable.

I could add an optional property called "fix", which can be used to enable the fixer. That way it can be enabled, when the user is sure that all function replacements are one-to-one replacements.

The sniff also allows for forbidding functions without suggesting a replacement.

I did notice that. When that happens, my fixer does not make any change.

jrfnl commented 1 year ago

Not all function replacements will be one-on-one replacements as the arguments functions take may be different.

I thought about that too. I use the sniff to standardize the many aliased functions in PHP to a standard form. Many of those functions are one-to-one replaceable.

I could add an optional property called "fix", which can be used to enable the fixer. That way it can be enabled, when the user is sure that all function replacements are one-to-one replacements.

To be honest, I still don't think that would be the right way to go.

There already are phpcs-only/phpcbf-only options which can be used in a custom ruleset, but that means everyone who uses the sniff would need to add those options to their ruleset.

While fixers in external standards may at times be "risky" fixers, the rule of thumb for the PHP_CodeSniffer library itself, in my experience, has been that risky fixers are not acceptable.

I believe the better solution for your specific use-case would be to create a new sniff in your own standard, which then extends this sniff and overloads the addError() method. That way, you can have a fixer for the select set of functions you use it for, while not impacting the wider PHPCS community.

Do note that extending sniffs is generally only a good idea if the sniff being extended is not used in another capacity in the same ruleset as otherwise you may run into trouble with the autoloading/ruleset to sniff translation.