Add sniffs to catch redundant isset checks#2792
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Code Analysis Sniffs phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIssetBeforeNotEmptySniff.php, phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantNotIssetBeforeEmptySniff.php |
Two new sniff classes implementing redundant isset/empty detection. Each registers for specific tokens, processes code patterns, extracts parentheses content via helper method getParenthesesContent(), compares variable expressions, and provides fixable error reporting that removes redundant checks. |
Configuration phpcs.xml |
Rule set restructured with three simplified isset rules replaced and three new redundancy detection rules introduced (RedundantTruthyBeforeStringComparison, RedundantIssetBeforeNotEmpty, RedundantNotIssetBeforeEmpty), then PreferObGetClean re-added. |
Possibly related PRs
- Add new custom sniffs for catching redundant truthy and empty checks #2785: Adds similar PHP_CodeSniffer sniffs in the same namespace with comparable pattern detection logic and identical
getParenthesesContent()helper method structure.
Poem
🐰 Two sniffs bound by parentheses and logic's grace,
Redundant isset checks are put in their place,
With pattern matching swift and fixes so clean,
Your code grows leaner than it's ever been! ✨
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main purpose of the PR: adding PHP_CodeSniffer sniffs to detect redundant isset checks, which aligns with the two new sniff classes and configuration changes. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧹 Recent nitpick comments
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIssetBeforeNotEmptySniff.php (1)
142-154: Consider extracting shared helper to reduce duplication.The
getParenthesesContentmethod is identical in bothRedundantIssetBeforeNotEmptySniffandRedundantNotIssetBeforeEmptySniff. You could extract this to a trait (e.g.,ParenthesesContentTrait) or an abstract base class to adhere to DRY.Example trait extraction
// In a new file: phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/Traits/ParenthesesContentTrait.php namespace Formidable\Sniffs\CodeAnalysis\Traits; use PHP_CodeSniffer\Files\File; trait ParenthesesContentTrait { private function getParenthesesContent( File $phpcsFile, $openParen, $closeParen ) { $tokens = $phpcsFile->getTokens(); $content = ''; for ( $i = $openParen + 1; $i < $closeParen; $i++ ) { if ( $tokens[ $i ]['code'] !== T_WHITESPACE ) { $content .= $tokens[ $i ]['content']; } } return $content; } }Then use it in both sniffs:
class RedundantIssetBeforeNotEmptySniff implements Sniff { use Traits\ParenthesesContentTrait; // ... }
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIssetBeforeNotEmptySniff.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantNotIssetBeforeEmptySniff.phpphpcs.xml
🧰 Additional context used
🧬 Code graph analysis (2)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantNotIssetBeforeEmptySniff.php (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIssetBeforeNotEmptySniff.php (3)
register(35-37)process(47-131)getParenthesesContent(142-154)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIssetBeforeNotEmptySniff.php (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantNotIssetBeforeEmptySniff.php (3)
register(35-37)process(47-131)getParenthesesContent(142-154)
⏰ 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). (6)
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (4)
phpcs.xml (1)
249-251: LGTM!The new redundant isset/empty sniff rules are properly added to the Code Analysis section, following the existing naming conventions.
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIssetBeforeNotEmptySniff.php (1)
47-131: Well-structured sniff implementation.The detection logic correctly validates each step of the pattern before flagging, and the variable comparison ensures both
isset()andempty()operate on the same expression. The fix appropriately removes only the redundantisset(...) &&portion.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantNotIssetBeforeEmptySniff.php (2)
35-37: Broad token registration is acceptable here.Registering for
T_BOOLEAN_NOTmeans this sniff triggers on every!in the codebase. The early returns (lines 53-55) efficiently filter out irrelevant cases. This is a standard pattern for PHP_CodeSniffer sniffs when the target construct starts with a common token.
47-131: LGTM!The implementation correctly detects the
!isset($var) || empty($var)pattern and safely removes the redundant portion. The logic properly handles edge cases like multi-argumentisset()or mismatched variables by comparing extracted content.
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 @coderabbitai help to get the list of available commands and usage tips.
…t_isset_checks Add sniffs to catch redundant isset checks
This is an old anti-pattern that has mostly been fixed through refactoring already.
It was able to catch an issue in Views.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.