Skip to content
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

4.0 | Remove public $error properties #186

Open
5 tasks
jrfnl opened this issue Dec 25, 2023 · 0 comments
Open
5 tasks

4.0 | Remove public $error properties #186

jrfnl opened this issue Dec 25, 2023 · 0 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Dec 25, 2023

Follow up/repost from squizlabs/PHP_CodeSniffer#2823:

A number of sniffs contain a public $error property to toggle whether the sniff should throw an error or a warning.

This property has been superseded by the ability to specify <type>error</type> for each sniff in a custom ruleset since PHPCS 1.4.1.

I'd like to suggest removing these public $error properties from any sniffs which have them and to remove the code within the process() method of those sniffs handling the toggle.

A quick scan yields the following sniffs for which this applies:

  • Generic.ControlStructures.InlineControlStructure
  • Generic.Formatting.MultipleStatementAlignment
  • Generic.PHP.ForbiddenFunctions
  • Generic.PHP.NoSilencedErrors
  • Generic.Strings.UnnecessaryStringConcat
  • Squiz.CSS.ForbiddenStyles (sniff will be removed anyway in 4.0.0)
  • Squiz.PHP.DiscouragedFunctions
  • Squiz.PHP.ForbiddenFunctions (inherited)

The Customizable Sniff Properties wiki page would also need to be updated for this change.
I'd suggest adding a Removed in column (where relevant) to the properties table containing the version nr in which a property was removed.


@VincentLanglet left the following remark:

I do like this property when writing my own sniff extending for example Generic.PHP.ForbiddenFunctions. It seems easier to modify the $error property than overriding something in the ruleset.

I think some Sniff made to be extendable could be better with a protected $error property.


An initial change related to this issue was already made for the 4.0 branch in commit squizlabs/PHP_CodeSniffer@606d876

With @gsherwood remarking:

I only ended up removing the property from 2 of the sniffs. The rest of the sniffs used the property to change messages or set a new default, as @VincentLanglet described in his comment.

The properties were removed from Generic.Strings.UnnecessaryStringConcat and Generic.Formatting.MultipleStatementAlignment

Upon which @jrfnl replied:

The rest of the sniffs used the property to change messages or set a new default

With the possible exception of the ForbiddenFunctions sniff(s), I don't see why these properties would remain.

Just like people can use <type> in their ruleset to change something from an error to a warning, they can also adjust the <message> in the ruleset from "this is forbidden" to "this is discouraged", so there is no reason for the sniff to contain that logic.


That leaves the following actions to be executed:

  • Remove the property from the Generic.ControlStructures.InlineControlStructure sniff and review the error message.
  • Remove the property from the Generic.PHP.NoSilencedErrors sniff and review the error message.
  • Change the $error property in Generic.PHP.ForbiddenFunctions from public to protected.
  • Change the property overload in the Squiz.PHP.DiscouragedFunctions (inherited) from public to protected.
  • Change the property overload in the Squiz.PHP.ForbiddenFunctions (inherited) from public to protected.
  • No change needed in the Generic.PHP.DeprecatedFunctions, which also extends the Generic.PHP.ForbiddenFunctions sniff, though the fact that that sniff is also impacted should be mentioned in the changelog.
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

1 participant