Repository navigation
Conversation
Limited to private methods so that sniffs extending these classes are unaffected. A return type on a public or protected method would be a fatal error for any subclass that overrides it without one. Four private methods are left untyped. Three return int|false, which needs union types from PHP 8.0. test_patterns() returns the result of preg_replace(), which can be null.
GaryJones
left a comment
GaryJones
left a comment
There was a problem hiding this comment.
Thanks Tom, this is a really nice tidy-up, and keeping it to private methods is exactly the right call given none of the sniff classes are final. I checked each new : bool and : void against its method body and callers, and they all hold up. Tests, ruleset tests and PHPCS all pass locally on the head commit too.
One thought on the description: test_patterns() in AbstractVariableRestrictionsSniff could take a nullable ?string return type, as that's been available since PHP 7.1. Entirely your call whether it's worth it for a value that's only null if PCRE errors, but if it stays untyped, it might be worth changing its @return string to @return string|null so the docblock matches the reasoning.
Everything inline is non-blocking.
| * @return bool | ||
| */ | ||
| private function isEarlyMainQueryCheck( $stackPtr ) { | ||
| private function isEarlyMainQueryCheck( $stackPtr ): bool { |
There was a problem hiding this comment.
Not something this PR introduces, but I noticed it while checking the : bool here. The findNext( [ T_RETURN ], ..., 'return', true ) call further down uses $local = true, so it stops at the first semicolon in the if body. That means the early return is only recognised if it's the first statement, which gives a false positive for something like:
add_action( 'pre_get_posts', function( $query ) {
if ( ! $query->is_main_query() ) {
do_log( 'x' );
return;
}
$query->set( 'cat', '-5' );
} );Remove the do_log() line and the warning goes away. Happy to open an issue for it rather than widen this PR.
Tiny nit while we're here, now that it's typed: the if ( $next ) { return true; } return false; at the end could just be return $next !== false;.
There was a problem hiding this comment.
I can reproduce that with the snippet, but I think it needs an issue and another PR
| * @return bool | ||
| */ | ||
| private function isInsideIfConditonal( $stackPtr ) { | ||
| private function isInsideIfConditonal( $stackPtr ): bool { |
There was a problem hiding this comment.
Again, not from this PR, just something I noticed in passing: the two end() calls on lines 272 and 277 read conditions before the array_key_exists() / is_array() / empty() guard on line 284, so the guard doesn't actually protect them, and the reset() on line 282 doesn't affect the result. Fine to leave for a follow-up, but it could probably collapse to one guard up front plus an in_array( T_IF, $conditions, true ) check.
There was a problem hiding this comment.
Agreed, I'll leave it for a separate PR so this one stays about return types
| * @return bool | ||
| */ | ||
| private function isFunctionCall( $stackPtr ) { | ||
| private function isFunctionCall( $stackPtr ): bool { |
There was a problem hiding this comment.
Optional nit: with : bool declared, the return ! ( $this->tokens[ $previous ]['code'] === T_FUNCTION ); at the end of this method might read a little more directly as return $this->tokens[ $previous ]['code'] !== T_FUNCTION;. Entirely up to you.
I left this one untyped because preg_replace() can return null. Nullable types have been available since PHP 7.1, so it can be typed, and the docblock now says string|null as well.
With `: bool` declared, isEarlyMainQueryCheck() and isFunctionCall() no longer need the if/else and the negated equality to produce a boolean.
|
@GaryJones ready for another review, I filed new issues and PRs for some of the items |
2 participants
The sniffs document return types in docblocks only. The minimum PHP version is 7.4, so the private methods can declare them.
This is limited to private methods on purpose. None of the sniff classes are final, so a return type on a public or protected method would be a fatal error for any third-party sniff that overrides it without one.
Three private methods stay untyped. They return
int|false, which needs union types from PHP 8.0.