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

Introduce new string_to_errorcode() utility method and implement where applicable. #814

Merged
merged 2 commits into from
Jan 29, 2017

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jan 21, 2017

Introduce new string_to_errorcode() utility method and implement where applicable.

This PR addresses two distinct but closely related issues.

1. Errorcodes should be limited to certain characters for them to be safely usable both in XML ruleset files as well as on the command line.

This PR introduces a new string_to_errorcode() method to be used for dynamic errorcodes using arbitrary strings which will convert any non-valid characters to underscores.

This PR also implements the usage of this function for all addError()/addWarning calls which warrant it.

2. Unique/dynamic sniffcodes

The sniffs which are based on the abstract classes all provide the public exclude property to selectively exclude a certain group from the checks the sniff executes.
The sniffcode, however, was also based on the group.
So while there were two different ways to disable the messages about a certain group (<property name="exclude" ...>, <exclude name=... sniffcode>), there was no possible way to leave a group enabled and only disable the message about one individual item (function/variable/etc) within that group.

This PR fixes that.

As groups could already be disabled using the exclude property, the errorcode will now refer to the individual item to allow for disabling the messages about one item in a group.

This PR breaks backward-compatibility, though the break will probably only affect a relatively small number of users which use heavily customized custom rulesets.

Also, with the merge of #633, BC was broken anyway for most sniffs this PR affects, so merging this in the same release as in which #633 is contained will ensure that users will only have to update their rulesets once for these changes.

The BC break of #633 is that groups of functions have been moved around to new sniffs, so <exclude name=...> and <property name="exclude" ...> directives in rulesets would no longer work as expected as they would now need to reference another sniff.

The BC break in this PR affects only the <exclude name=...> directives which use errorcodes - in contrast to sniff names - as the errorcodes will change.

This change affects the errorcodes for the following sniffs:

  • WordPress.DB.RestrictedClasses
  • WordPress.DB.RestrictedFunctions
  • WordPress.Functions.DontExtract
  • WordPress.PHP.DevelopmentFunctions
  • WordPress.PHP.DiscouragedPHPFunctions
  • WordPress.PHP.POSIXFunctions
  • WordPress.PHP.RestrictedFunctions
  • WordPress.VIP.FileSystemWritesDisallow
  • WordPress.VIP.OrderByRand
  • WordPress.VIP.PostsPerPage
  • WordPress.VIP.RestrictedFunctions
  • WordPress.VIP.RestrictedVariables
  • WordPress.VIP.SessionFunctionsUsage
  • WordPress.VIP.SlowDBQuery
  • WordPress.VIP.TimezoneChange
  • WordPress.WP.AlternativeFunctions
  • WordPress.WP.DiscouragedFunctions

…licable.

This commit breaks backward-compatibility, though the break will probably only affect a relatively small number of users.
@JDGrimes
Copy link
Contributor

@jrfnl Would you mind also listing here which sniffs, if any, previously were likely to actually have problematic characters in their error codes? (And why, specifically—that is, what were the error codes based on?) Just for easier reference.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 21, 2017

Would you mind also listing here which sniffs, if any, previously were likely to actually have problematic characters in their error codes?

Up to now, there weren't any as the codes were based on groupnames and groupnames were provided in ascii in the arrays.

With the change in this PR, the errorcodes are now however based on a combination of the groupname and the function/variable being matched which could include :: or -> syntaxes or spaces which could become problematic.

@JDGrimes
Copy link
Contributor

Ah, OK. Thanks for the clarification, I wasn't sure if there were any other ones that were already dynamic and might have contained problematic characters. I guess I could have looked at the diff more closely.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 21, 2017

@JDGrimes No worries.
The errorcode which were dynamic before have all been changed in this PR, some have become an even more dynamic code than before, all have been wrapped within the new utility.

The utility function is really a "Better safe than sorry" function. A little overhead in some cases, but it will prevent any breakage in outlier cases.

@jrfnl
Copy link
Member Author

jrfnl commented Jan 27, 2017

Anyone got any more feedback on this ? Or can it be merged ? (some other branches are waiting for this one)

@JDGrimes JDGrimes merged commit 0ceb4cb into develop Jan 29, 2017
@JDGrimes JDGrimes deleted the feature/errorcodes-for-groups branch January 29, 2017 22:20
@jrfnl
Copy link
Member Author

jrfnl commented Jan 29, 2017

@JDGrimes Thanks!

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

Successfully merging this pull request may close these issues.

3 participants