New sniff to simplify not empty checks after a type check#2821
Conversation
📝 WalkthroughWalkthroughThis PR replaces redundant Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Crabcyborg
deleted the
new_sniff_to_simplify_not_empty_checks_after_a_type_check
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyAfterTypeCheckSniff.php`:
- Around line 73-80: The sniff incorrectly treats any T_STRING matching a check
name as a bare function call; update the logic around $funcName/$stackPtr to
skip when the previous non-whitespace token before $stackPtr is a method/static
call operator by checking $tokens[ $stackPtr - 1 ] (or using findPrevious to get
the previous non-whitespace token) and returning early if that token's code is
T_DOUBLE_COLON or T_OBJECT_OPERATOR, so that Foo::is_array(...) and
$obj->is_array(...) are not treated as built-in checks and are not auto-fixed;
keep the existing isset handling and in_array($funcName, $this->checkFunctions,
true) check intact.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
classes/controllers/FrmXMLController.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmForm.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyAfterTypeCheckSniff.phpphpcs-sniffs/Formidable/ruleset.xmltests/phpunit/base/FrmUnitTest.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a PHP doc comment, but also like 'x.x' when functions like _deprecated_function() are used.
- Treat these placeholders as acceptable in this codebase.
Files:
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyAfterTypeCheckSniff.phpclasses/helpers/FrmFieldsHelper.phpclasses/models/FrmForm.phptests/phpunit/base/FrmUnitTest.phpclasses/helpers/FrmXMLHelper.phpclasses/controllers/FrmXMLController.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (6)
tests/phpunit/base/FrmUnitTest.php (1)
505-507: Truthiness gate aligns with intent.This keeps the numeric filter behind an array guard and matches the new sniff direction.
classes/helpers/FrmFieldsHelper.php (1)
1600-1602: LGTM for multi-select truthiness check.This is a safe simplification after the array guard.
classes/controllers/FrmXMLController.php (1)
479-481: Looks good—cleaner gating.Truthiness after
is_arrayis consistent and preserves behavior.classes/models/FrmForm.php (1)
843-845: OK to simplify the scalar check.Truthiness here matches the previous non-empty behavior for scalar IDs.
classes/helpers/FrmXMLHelper.php (1)
2225-2228: Simplification is safe here.The numeric guard still applies and the truthiness check removes redundant
empty().phpcs-sniffs/Formidable/ruleset.xml (1)
42-42: Rule registration looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Check if this is a recognized check function or isset. | ||
| $funcName = $tokens[ $stackPtr ]['content']; | ||
|
|
||
| if ( $tokens[ $stackPtr ]['code'] === T_ISSET ) { | ||
| $funcName = 'isset'; | ||
| } elseif ( ! in_array( $funcName, $this->checkFunctions, true ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "RedundantEmptyAfterTypeCheckSniff.php" -type fRepository: Strategy11/formidable-forms
Length of output: 154
🏁 Script executed:
cat -n phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyAfterTypeCheckSniff.phpRepository: Strategy11/formidable-forms
Length of output: 7930
🏁 Script executed:
# Verify the token constants used in proposed fix exist in PHP_CodeSniffer
rg -t php "(T_OBJECT_OPERATOR|T_DOUBLE_COLON|T_FUNCTION)" phpcs-sniffs/ | head -20Repository: Strategy11/formidable-forms
Length of output: 2863
🏁 Script executed:
# Check if there are any existing usages of findPrevious pattern in the codebase
rg -t php "findPrevious.*T_WHITESPACE.*true" phpcs-sniffs/ | head -10Repository: Strategy11/formidable-forms
Length of output: 1715
Guard against method/static calls to avoid unsafe fixes.
The sniff currently treats any T_STRING matching a type check name (like is_array) as a built-in function without checking the preceding token. This causes it to incorrectly match method and static method calls such as Foo::is_array($x) or $obj->is_array($x), then auto-fix them to Foo::is_array($x) && $x or $obj->is_array($x) && $x, potentially breaking semantics if the method doesn't guarantee non-emptiness. Add a guard to skip when the previous non-whitespace token is T_DOUBLE_COLON or T_OBJECT_OPERATOR.
🛠️ Proposed fix
$funcName = $tokens[ $stackPtr ]['content'];
+ $prevNonWhitespace = $phpcsFile->findPrevious( T_WHITESPACE, $stackPtr - 1, null, true );
+ if ( false !== $prevNonWhitespace && in_array( $tokens[ $prevNonWhitespace ]['code'], array( T_OBJECT_OPERATOR, T_DOUBLE_COLON ), true ) ) {
+ return;
+ }
+
if ( $tokens[ $stackPtr ]['code'] === T_ISSET ) {
$funcName = 'isset';
} elseif ( ! in_array( $funcName, $this->checkFunctions, true ) ) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if this is a recognized check function or isset. | |
| $funcName = $tokens[ $stackPtr ]['content']; | |
| if ( $tokens[ $stackPtr ]['code'] === T_ISSET ) { | |
| $funcName = 'isset'; | |
| } elseif ( ! in_array( $funcName, $this->checkFunctions, true ) ) { | |
| return; | |
| } | |
| // Check if this is a recognized check function or isset. | |
| $funcName = $tokens[ $stackPtr ]['content']; | |
| $prevNonWhitespace = $phpcsFile->findPrevious( T_WHITESPACE, $stackPtr - 1, null, true ); | |
| if ( false !== $prevNonWhitespace && in_array( $tokens[ $prevNonWhitespace ]['code'], array( T_OBJECT_OPERATOR, T_DOUBLE_COLON ), true ) ) { | |
| return; | |
| } | |
| if ( $tokens[ $stackPtr ]['code'] === T_ISSET ) { | |
| $funcName = 'isset'; | |
| } elseif ( ! in_array( $funcName, $this->checkFunctions, true ) ) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyAfterTypeCheckSniff.php`
around lines 73 - 80, The sniff incorrectly treats any T_STRING matching a check
name as a bare function call; update the logic around $funcName/$stackPtr to
skip when the previous non-whitespace token before $stackPtr is a method/static
call operator by checking $tokens[ $stackPtr - 1 ] (or using findPrevious to get
the previous non-whitespace token) and returning early if that token's code is
T_DOUBLE_COLON or T_OBJECT_OPERATOR, so that Foo::is_array(...) and
$obj->is_array(...) are not treated as built-in checks and are not auto-fixed;
keep the existing isset handling and in_array($funcName, $this->checkFunctions,
true) check intact.
1 participant
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.