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

NamingConventions/ValidHookName: various improvements #335

Merged
merged 7 commits into from
Nov 4, 2023

Conversation

jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Nov 4, 2023

NamingConventions/ValidHookName: remove some redundant code

The same (low performance impact) conditions are used directly after too, so no need to double the effort.

NamingConventions/ValidHookName: add some extra defensive coding

As things are, we already know that $first_non_empty will always return an integer stack pointer, as the same check is done in the process() method and this method isn't called if the return would have been false.

Having said that, that means these methods are (too) closely coupled and a change in the logic in the process() method could inadvertently cause PHP to start throwing notices in the verify_yoast_hook_name() method.

So, I'm adding some extra defensive coding just in case, though also ignoring that code for code coverage as, as things are, the condition will always be false.

NamingConventions/ValidHookName: improve message clarity of the NonString warning

The message called to manually examine the hook name for compliance with the max word count, but did not specify the applicable max word count, making the message in-actionable.

Fixed now.

Related to #247

NamingConventions/ValidHookName: rename local variable

The $param variable name is quite non-descript.

This commit updates the parameter name (in two places) to $hook_name_param to make it more obvious what the variable signifies.

NamingConventions/ValidHookName::verify_yoast_hook_name(): change function signature

Originally, the verify_yoast_hook_name() method would get passed the complete stack of parameters.

This is not necessary as the method only examines the first - $hook_name - parameter and that parameter has already been retrieved from the stack before.

This changes the function signature of the verify_yoast_hook_name() method to only expect the parameter information for the $hook_name parameter, instead of the complete stack of parameters.

It also changes the method from public to private. This method never needed to be public.

NamingConventions/ValidHookName: add support for PHP 8.0+ named parameters

While WP does not officially support function calls using named parameters, that doesn't mean that nobody will use them....

This adjusts the sniff to allow for calls to hook functions using named parameters.

The way the correct parameter is retrieved now uses the new WPHookHelper::get_hook_name_param() method, which uses the PHPCSUtils PassedParameters::getParameterFromStack() method under the hood. The WPHookHelper class keeps track of which functions are considered hook functions, what parameter names are used in WP and the parameter position.

Note: as parameters can be skipped and/or passed in an unconventional order, it is now possible for the parameter to not exist. While that would result in a fatal error in PHP (missing required parameter), the sniff should still handle this situation gracefully, which is why the check for the parameter has been moved up and will cause the sniff to bow out if the parameter can not be found.

Includes additional unit tests.

Note: function calls with named arguments are not supported for the do_action() or apply_filters() functions as those have a parameter with a spread operator (but that's irrelevant for the sniff).

NamingConventions/ValidHookName: minor documentation improvements

jrfnl added 7 commits November 4, 2023 11:59
The same (low performance impact) conditions are use directly after too, so no need to double the effort.
As things are, we already know that `$first_non_empty` will always return an integer stack pointer, as the same check is done in the `process()` method and this method isn't called if the return would have been `false`.

Having said that, that means these methods are (too) closely coupled and a change in the logic in the `process()` method could inadvertently cause PHP to start throwing notices in the `verify_yoast_hook_name()` method.

So, I'm adding some extra defensive coding just in case, though also ignoring that code for code coverage as, as things are, the condition will always be `false`.
…tring` warning

The message called to manually examine the hook name for compliance with the max word count, but did not specify the applicable max word count, making the message in-actionable.

Fixed now.

Related to 247
The `$param` variable name is quite non-descript.

This commit updates the parameter name (in two places) to `$hook_name_param` to make it more obvious what the variable signifies.
…ction signature

Originally, the `verify_yoast_hook_name()` method would get passed the complete stack of parameters.

This is not necessary as the method only examines the first - `$hook_name` - parameter and that parameter has already been retrieved from the stack before.

This changes the function signature of the `verify_yoast_hook_name()` method to only expect the parameter information for the `$hook_name` parameter, instead of the complete stack of parameters.

It also changes the method from `public` to `private`. This method never needed to be `public`.
…eters

While WP does not officially support function calls using named parameters, that doesn't mean that nobody will use them....

This adjusts the sniff to allow for calls to hook functions using named parameters.

The way the correct parameter is retrieved now uses the new `WPHookHelper::get_hook_name_param()` method, which uses the PHPCSUtils `PassedParameters::getParameterFromStack()` method under the hood. The `WPHookHelper` class keeps track of which functions are considered hook functions, what parameter names are used in WP and the parameter position.

Note: as parameters can be skipped and/or passed in an unconventional order, it is now possible for the parameter to not exist. While that would result in a fatal error in PHP (missing required parameter), the sniff should still handle this situation gracefully, which is why the check for the parameter has been moved up and will cause the sniff to bow out if the parameter can not be found.

Includes additional unit tests.

Note: function calls with named arguments are not supported for the `do_action()` or `apply_filters()` functions as those have a parameter with a spread operator (but that's irrelevant for the sniff).
@coveralls
Copy link

Coverage Status

coverage: 98.406% (+0.002%) from 98.404%
when pulling 4385597 on JRF/yoastcs-validhookname-various-improvements
into bc72022 on develop.

@jrfnl jrfnl merged commit 5fbd039 into develop Nov 4, 2023
25 checks passed
@jrfnl jrfnl deleted the JRF/yoastcs-validhookname-various-improvements branch November 4, 2023 11:06
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.

2 participants