New sniff to remove redundant intermediate variables#2825
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new PHP_CodeSniffer sniff to detect and auto-fix redundant intermediate variables and updates one file to inline a comment count into a static property instead of using a local temporary variable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✏️ Tip: You can disable this entire section by setting 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: 1
🤖 Fix all issues with AI agents
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.php`:
- Around line 1-16: CI reports a phpdoc_types_order violation in this file; run
php-cs-fixer fix (or apply the phpdoc types order rule) to reorder types in the
docblocks according to project/php-cs-fixer configuration for
Formidable_Sniffs_CodeAnalysis_RedundantIntermediateVariableSniff (namespace
Formidable\Sniffs\CodeAnalysis) so the phpdoc type annotations comply with the
configured ordering.
🧹 Nitpick comments (4)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.php (4)
164-165: Remove unused variable$targetEnd.The variable
$targetEndis initialized on line 165 and reassigned on lines 187 and 209, but it is never used. The method returns$afterValueas the'end'value, making$targetEnddead code.🧹 Suggested fix
private function getTargetAssignment( File $phpcsFile, array $tokens, $nextStatement, $variableName ) { // Check for self::$property, static::$property, or $this->property. $target = ''; - $targetEnd = $nextStatement; $equalToken = false;Also remove the assignments
$targetEnd = $propertyToken;on lines 187 and 209.
255-265: Consider removing unused$phpcsFileparameter.The
$phpcsFileparameter is not used in this method. However, if you prefer to keep method signatures consistent across helper methods, you can suppress the warning with@SuppressWarnings(PHPMD.UnusedFormalParameter)or simply leave it for future use.
322-322: Remove unused parameter$targetAssignmentEnd.The
$targetAssignmentEndparameter is passed toapplyFix()but never used. The method uses$targetAssignment['end']instead. Consider removing this parameter and updating the call site on line 122.🧹 Suggested fix
Update the method signature:
- private function applyFix( File $phpcsFile, array $tokens, $varStart, $assignmentEnd, $nextStatement, array $targetAssignment, $targetAssignmentEnd ) { + private function applyFix( File $phpcsFile, array $tokens, $varStart, $assignmentEnd, $nextStatement, array $targetAssignment ) {Update the call site on line 122:
- $this->applyFix( $phpcsFile, $tokens, $stackPtr, $assignmentEnd, $nextStatement, $targetAssignment, $targetAssignmentEnd ); + $this->applyFix( $phpcsFile, $tokens, $stackPtr, $assignmentEnd, $nextStatement, $targetAssignment );
366-374: Consider extracting the magic number+ 10into a constant or using$targetAssignment['end']directly.The
+ 10buffer on lines 366 and 374 is a heuristic to ensure finding nearby tokens. While this works for the expected patterns, using$targetAssignment['end'] + 1(just past the semicolon) should be sufficient since the variable token must appear before the semicolon.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/helpers/FrmCSVExportHelper.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.phpphpcs-sniffs/Formidable/ruleset.xml
🧰 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:
classes/helpers/FrmCSVExportHelper.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.php
🧬 Code graph analysis (1)
classes/helpers/FrmCSVExportHelper.php (1)
classes/models/FrmDb.php (2)
FrmDb(6-863)get_count(231-234)
🪛 GitHub Actions: Inspections
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.php
[error] 1-1: PHP CS Fixer reported 1 fixable issue (phpdoc_types_order in RedundantIntermediateVariableSniff.php); process exited with code 8. Run 'php-cs-fixer fix' to apply fixes.
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.php
165-165: Avoid unused local variables such as '$targetEnd'. (undefined)
(UnusedLocalVariable)
255-255: Avoid unused parameters such as '$phpcsFile'. (undefined)
(UnusedFormalParameter)
278-278: Avoid unused parameters such as '$phpcsFile'. (undefined)
(UnusedFormalParameter)
299-299: Avoid unused parameters such as '$phpcsFile'. (undefined)
(UnusedFormalParameter)
322-322: Avoid unused parameters such as '$targetAssignmentEnd'. (undefined)
(UnusedFormalParameter)
⏰ 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). (5)
- GitHub Check: Run ESLint
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Run PHPCS inspection
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (6)
classes/helpers/FrmCSVExportHelper.php (1)
158-170: LGTM!The direct assignment to
self::$comment_counteliminates the redundant intermediate variable while preserving identical behavior. Type safety is maintained sinceFrmDb::get_count()returnsint, matching the property type.phpcs-sniffs/Formidable/ruleset.xml (1)
43-43: LGTM!The new rule reference follows the established pattern and is correctly placed in the Code Analysis section.
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantIntermediateVariableSniff.php (4)
46-124: LGTM!The
process()method has well-structured logic with appropriate early returns and safety checks. The guard forrequire/includestatements (line 97-99) is a good safeguard against edge cases where included files might reference the variable.
134-150: LGTM!The backward traversal correctly identifies the enclosing function scope and properly validates boundaries.
278-307: LGTM on the logic.Both methods correctly scan for variable usage within the specified ranges. The same note about unused
$phpcsFileparameters applies here as mentioned forhasRequireOrIncludeAfter().
341-363: LGTM!The line removal logic correctly identifies the start of the declaration line and properly handles the trailing newline to avoid leaving blank lines.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.