Add new custom sniffs for catching redundant truthy and empty checks#2785
Conversation
WalkthroughThis pull request introduces two new PHP_CodeSniffer sniffs for detecting and auto-fixing redundant code patterns, then applies the simplifications to two existing codebase files. The sniffs detect redundant truthiness checks before string comparisons and redundant isset/empty patterns, offering automatic fixes. Two controller/helper methods are simplified to remove explicit truthiness checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantTruthyBeforeStringComparisonSniff.php (2)
48-49: Remove unused variable.The
$tokensvariable is declared but never used in theprocess()method. Each helper method fetches its own$tokensfrom$phpcsFile->getTokens().Suggested fix
public function process( File $phpcsFile, $stackPtr ) { - $tokens = $phpcsFile->getTokens(); - // Find the expression before &&. $beforeAnd = $this->getExpressionBefore( $phpcsFile, $stackPtr );
247-247: Consider caching the token count.
count($tokens)is evaluated on every loop iteration. While performance impact is minimal for PHPCS sniffs, caching the count is a cleaner pattern.Suggested fix
- for ( $i = $start; $i < count( $tokens ); $i++ ) { + $tokenCount = count( $tokens ); + for ( $i = $start; $i < $tokenCount; $i++ ) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
phpcs.xmlis excluded by!**/*.xml
📒 Files selected for processing (4)
classes/controllers/FrmUsageController.phpclasses/helpers/FrmAppHelper.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantTruthyBeforeStringComparisonSniff.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIssetEmptyCheckSniff.php
🧰 Additional context used
🧬 Code graph analysis (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIssetEmptyCheckSniff.php (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantTruthyBeforeStringComparisonSniff.php (2)
register(36-38)process(48-115)
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantTruthyBeforeStringComparisonSniff.php
49-49: Avoid unused local variables such as '$tokens'. (undefined)
(UnusedLocalVariable)
⏰ 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). (2)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (7)
classes/controllers/FrmUsageController.php (1)
121-122: LGTM!The simplification is correct. When
$form_typeis falsy (empty string, null, etc.), it cannot equal'published', so the explicit truthy check was redundant. This aligns with the newRedundantTruthyBeforeStringComparisonSniffbeing introduced in this PR.classes/helpers/FrmAppHelper.php (1)
542-547: LGTM!The simplification correctly removes the redundant truthy check. If
$pagenowis falsy, the strict comparison to'admin-ajax.php'will returnfalseanyway.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIssetEmptyCheckSniff.php (3)
1-36: LGTM - Well-structured sniff implementation.The sniff correctly detects the
isset($x) && empty($x)pattern. The semantic equivalence is valid because afterisset()confirms the variable exists,empty()only checks for falsiness, which is equivalent to! $x.
46-129: LGTM!The process method is well-structured with proper guard clauses and early returns. The fix logic correctly replaces
empty($var)with! $varby substituting theemptytoken and removing the parentheses content.
140-168: LGTM!The
getParenthesesContenthelper correctly normalizes content by stripping whitespace, enabling accurate comparison of variables with different spacing patterns.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantTruthyBeforeStringComparisonSniff.php (2)
430-440: Good edge case handling.The sniff correctly skips empty strings and numeric strings. For empty string comparisons (
$var === ''), the truthy check is semantically meaningful. For numeric strings like'0', PHP's type juggling makes truthy checks potentially relevant.
396-409: LGTM - Property/array access detection.The sniff correctly ensures only simple variables are matched by checking for object operators (
->,?->), array brackets ([), and static access (::) after the variable. This prevents false positives on patterns like$var['key'] && $var['key'] === 'string'where the truthy check may not be redundant.
Add new custom sniffs for catching redundant truthy and empty checks
The empty sniff isn't caught in Lite, but it helped catch some code in Pro (https://github.com/Strategy11/formidable-pro/pull/6193).