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

Refactor the WordPress_AbstractFunctionRestrictionsSniff #795

Merged
merged 3 commits into from
Jan 20, 2017

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jan 16, 2017

This PR contains three distinct commits. The first is a refactor of the abstract function restrictions class, the second and third are changes to child classes taking advantage of the refactor.


Refactor the WordPress_AbstractFunctionRestrictionsSniff

This refactor is intended to make the WordPress_AbstractFunctionRestrictionsSniff more extendable and reusable.
The refactor does not contain any functional changes.

  • Extend the main WordPress_Sniff to be able to use the init() function which will save us having to pass around the $phpcsFile and $tokens variables and make the utility functions contained in that class available (and allow for moving some utility functions contained in the WordPress_AbstractClassRestrictionsSniff to the main WordPress_Sniff class in a future PR).
  • Split up the logic of the sniff into three distinct functions which each individually can be overloaded in a child class.

Additionally:

  • Added since tags to properties and functions introduced after the initial version of the class.

Simplify the WordPress_AbstractClassRestrictionsSniff

The refactor of the WordPress_AbstractFunctionRestrictionsSniff allows for some code simplification in the WordPress_AbstractClassRestrictionsSniff.

The simplification does not contain any functional changes.

  • Defer to parent process() method for initial verification & setup.
  • Defer to parent process_matched_token() method for the throwing of the error.
  • Use the $this->tokens and $this->phpcsFile properties now available instead of passing the variables between functions.

Also:

  • Removed a property which couldn't be used as is anyway (as the parent uses self and PHCPS code still has to be compatible with PHP 5.2, so we can't use static).

Simplify the WordPress_Sniffs_WP_DeprecatedFunctionsSniff & make it more efficient

The refactor of the WordPress_AbstractFunctionRestrictionsSniff allows to make the WordPress_Sniffs_WP_DeprecatedFunctionsSniff more efficient.

  • No need to loop through the array and create all the error messages and subgroups for the initial getGroups() setup.
  • Instead, overload the parent's process_matched_token() method and move the logic for setting up the error message and such there, so we only execute that logic when the function has actually been matched.

This does mean that individual deprecated function checks can no longer be excluded using the exclude property.
However, the errorCode has been changed to a dynamic one which will still allow modular exclusion of one or more deprecated functions from a ruleset, though now it would need to be done using the errorCode.

jrfnl added 3 commits January 16, 2017 10:37
This refactor is intended to make the WordPress_AbstractFunctionRestrictionsSniff more extendable and reusable.
The refactor does not contain any functional changes.

* Extend the main `WordPress_Sniff` to be able to use the `init()` function which will save us having to pass around the `$phpcsFile` and `$tokens` variables and make the utility functions contained in that class available (and allow for moving some utility functions contained in the `WordPress_AbstractClassRestrictionsSniff` to the main `WordPress_Sniff ` class).
* Split up the logic of the sniff into three distinct functions which each individually can be overloaded in a child class.

Additionally:
* Added since tags to properties and functions introduced after the initial version of the class.
The refactor of the `WordPress_AbstractFunctionRestrictionsSniff` allows for some code simplification in the `WordPress_AbstractClassRestrictionsSniff`.

The simplification does not contain any functional changes.

* Defer to parent `process()` method for initial verification & setup.
* Defer to parent `process_matched_token()` method for the throwing of the error.
* Use the `$this->tokens` and `$this->phpcsFile` properties now available instead of passing the variables between functions.

Also:
* Removed a property which couldn't be used as is anyway (as the parent uses `self` and PHCPS still has to be compatible with PHP 5.2, so we can't use `static`).
…ore efficient

The refactor of the `WordPress_AbstractFunctionRestrictionsSniff` allows to make the `WordPress_Sniffs_WP_DeprecatedFunctionsSniff` more efficient.

* No need to loop through the array and create all the error messages and subgroups for the initial `getGroups()` setup.
* Instead, overload the parent's `process_matched_token()` method and move the logic for setting up the error message and such there, so we only execute that logic when the function has actually been matched.

This does mean that individual deprecated function checks can no longer be excluded using the `exclude` property.
However, the errorCode has been changed to a dynamic one which will still allow modular exclusion of one or more deprecated functions from a ruleset, though now it would need to be done using the errorCode.
@jrfnl jrfnl added this to the 0.11.0 milestone Jan 16, 2017

if ( 'warning' === $this->groups[ $group_name ]['type'] ) {
$addWhat = array( $this->phpcsFile, 'addWarning' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to directly call the method addWarning like is done further done in #diff-ede7f8be23ca928d9ba5e981dc4c08fbR1137 instead of using call_user_func()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree, that is outside the scope of this PR.
There are a number of places in the code where a error/warning switch is made and several different ways of doing this are being used. Typical one which lends itself to a utility function to remove duplicate code / create logic consistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grappler FYI: In the mean time I've prepared a branch with that utility function & fixing this across files. Waiting for some other merges to PR it to prevent conflicts.

@GaryJones GaryJones merged commit 3c3bc4a into develop Jan 20, 2017
@GaryJones GaryJones deleted the feature/refactor-abstract-function-sniff branch January 20, 2017 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants