Remove more useless parentheses#2767
Conversation
WalkthroughThe PR removes redundant parentheses around boolean expressions and ternary assignments across many helpers, models, fields, and view files, and updates a PHP_CodeSniffer sniff to treat logical/comparison/coalesce/ternary expressions as simple (no parentheses required). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantParenthesesSniff.php (1)
141-194: Don’t auto-fix parentheses when the expression containsand/or/xor(PHP operator precedence changes semantics).With the new unconditional
return true, this sniff can “fix” cases like$x = ( $a or $b );into$x = $a or $b;, which is not equivalent in PHP becauseor/and/xorhave lower precedence than assignment operators. This is a correctness bug for both=and compound assignments (+=,.=etc.).
Also, the comment at Line 192 mentions only;/,but the code also allows).Proposed fix
private function isSimpleExpression( File $phpcsFile, $openParen, $closeParen ) { $tokens = $phpcsFile->getTokens(); // Count operators that would make this a complex expression. $hasCoalesce = false; $hasTernary = false; - $hasLogicalOp = false; + $hasLogicalOp = false; + $hasLowPrecLogic = false; // and/or/xor $hasComparisonOp = false; $hasArithmeticOp = false; $nestedParenDepth = 0; for ( $i = $openParen + 1; $i < $closeParen; $i++ ) { $code = $tokens[ $i ]['code']; @@ // Check for various operators. if ( $code === T_COALESCE ) { $hasCoalesce = true; } elseif ( $code === T_INLINE_THEN || $code === T_INLINE_ELSE ) { $hasTernary = true; - } elseif ( in_array( $code, array( T_BOOLEAN_AND, T_BOOLEAN_OR, T_LOGICAL_AND, T_LOGICAL_OR, T_LOGICAL_XOR ), true ) ) { + } elseif ( in_array( $code, array( T_LOGICAL_AND, T_LOGICAL_OR, T_LOGICAL_XOR ), true ) ) { + $hasLowPrecLogic = true; + $hasLogicalOp = true; + } elseif ( in_array( $code, array( T_BOOLEAN_AND, T_BOOLEAN_OR ), true ) ) { $hasLogicalOp = true; } elseif ( in_array( $code, array( T_IS_EQUAL, T_IS_NOT_EQUAL, T_IS_IDENTICAL, T_IS_NOT_IDENTICAL, T_LESS_THAN, T_GREATER_THAN, T_IS_SMALLER_OR_EQUAL, T_IS_GREATER_OR_EQUAL, T_SPACESHIP ), true ) ) { $hasComparisonOp = true; } elseif ( in_array( $code, array( T_PLUS, T_MINUS, T_MULTIPLY, T_DIVIDE, T_MODULUS ), true ) ) { $hasArithmeticOp = true; } } - // If there are arithmetic operators, it might need parentheses for precedence. - if ( $hasArithmeticOp ) { + // If there are arithmetic operators, it might need parentheses for precedence. + // If there are and/or/xor operators, parentheses are required in assignments due to precedence. + if ( $hasArithmeticOp || $hasLowPrecLogic ) { return false; } - // Logical and comparison operators don't need parentheses in a simple assignment. - // The parentheses are only needed when there's an operator precedence issue outside. - // Since we already checked that this is followed by ; or , the parentheses are redundant. + // `&&`/`||` and comparisons don't need parentheses in this assignment context. + // Note: caller allows `;`, `,`, or `)` after the closing parenthesis. return true; }
🧹 Nitpick comments (1)
classes/helpers/FrmXMLHelper.php (1)
955-967: Parentheses removal is safe here; consider a small readability tweak (optional).Line 963 preserves behavior (comparisons bind tighter than
&&). Optional: assign$custom_style = $form['options']['custom_style'];before the comparison to reduce repetition.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
classes/helpers/FrmAppHelper.phpclasses/helpers/FrmEntriesListHelper.phpclasses/helpers/FrmFieldsHelper.phpclasses/helpers/FrmStylesHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmCreateFile.phpclasses/models/FrmDb.phpclasses/models/FrmField.phpclasses/models/FrmForm.phpclasses/models/fields/FrmFieldCaptcha.phpclasses/models/fields/FrmFieldType.phpclasses/models/fields/FrmFieldUserID.phpclasses/views/frm-fields/back-end/settings.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantParenthesesSniff.php
🧰 Additional context used
🧬 Code graph analysis (4)
classes/views/frm-fields/back-end/settings.php (1)
classes/models/FrmField.php (2)
FrmField(6-1619)is_option_true(1399-1404)
classes/helpers/FrmFieldsHelper.php (1)
classes/helpers/FrmAppHelper.php (1)
checked(2417-2421)
classes/models/fields/FrmFieldType.php (2)
classes/models/FrmField.php (2)
FrmField(6-1619)is_read_only(1369-1372)classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5024)is_admin(582-591)
classes/models/FrmForm.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5024)is_admin(582-591)
⏰ 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). (9)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (16)
classes/models/FrmDb.php (1)
98-98: Parentheses removal is safe and aligns with best practices.The redundant parentheses around the logical expression are safely removed. The
!operator has higher precedence than&&, and multiple&&operators are left-associative, so the evaluation order is unambiguous without extra grouping.classes/models/fields/FrmFieldCaptcha.php (1)
328-328: Ternary parentheses removal is safe and improves code clarity.The redundant parentheses around a simple ternary expression are safely removed. With no nested ternaries and a clear single-level condition, the parentheses were purely stylistic and their removal improves readability.
classes/helpers/FrmAppHelper.php (1)
3166-3166: Comparison parentheses removal is safe and maintains clarity.The redundant parentheses around a simple comparison expression are safely removed. The
!==operator has clear precedence, and the intent (checking whether a time string differs from midnight) remains immediately apparent.classes/helpers/FrmStylesHelper.php (1)
1091-1091: LGTM! Clean removal of redundant parentheses.The parentheses around the comparison were unnecessary. The simplified form is more readable and maintains the same behavior.
classes/helpers/FrmFieldsHelper.php (2)
697-697: LGTM! Parentheses correctly removed from boolean assignment.The outer parentheses were redundant. The expression correctly evaluates whether a field option should be checked based on the default value.
736-736: LGTM! Clean ternary expression.Removing parentheses around the ternary condition follows standard PHP conventions and improves readability.
classes/helpers/FrmEntriesListHelper.php (1)
250-250: LGTM! Simplified boolean assignment.The parentheses were unnecessary for this simple comparison. The change improves readability.
classes/models/FrmCreateFile.php (1)
281-282: LGTM! Clean boolean assignments.The outer parentheses were redundant for these boolean assignments. The && operator precedence ensures the expressions evaluate correctly without them.
classes/models/fields/FrmFieldUserID.php (1)
68-68: LGTM! Redundant parentheses removed.The outer parentheses were unnecessary for this boolean assignment. The && operators evaluate left-to-right as expected, and the phpcs ignore comment is appropriately placed for the nonce verification check.
classes/models/FrmField.php (2)
1025-1037: LGTM:$should_get_subformssimplification keeps the same logic.Line 1033 is clearer and precedence-safe (
&&chain).
1588-1604: LGTM:is_field_typeboolean flattening is equivalent and readable.Lines 1592-1595 keep the same truth table; parentheses still make the
data/lookup/productcases explicit.classes/views/frm-fields/back-end/settings.php (1)
478-484: LGTM:$has_validationremains correct and readable.Line 482 is precedence-safe, and the grouped
( $display['invalid'] && ! $hidden_invalid )keeps intent clear.classes/models/FrmForm.php (2)
368-383: LGTM:$is_settings_pageparentheses removal is behavior-preserving.Line 375 is still clear and safe (no precedence ambiguity).
1290-1294: LGTM:show_submitsimplification is safe and clearer.Line 1292’s
&&chain is unambiguous and matches prior intent.classes/models/fields/FrmFieldType.php (2)
822-829: LGTM:$use_stylesimplification is short-circuit safe.Line 823 won’t read
custom_stylewhen it’s unset due to||short-circuiting.
1464-1476: LGTM:$readonlysimplification is behavior-preserving.Line 1465 keeps the same condition and reads a bit cleaner.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantParenthesesSniff.php (1)
186-194: Logic looks correct, minor comment suggestion.The simplified logic is sound—arithmetic operators still require parentheses for precedence, while logical, comparison, coalesce, and ternary operators can safely omit outer parentheses in assignments and returns.
💡 Optional: Update comment to match expanded scope
Consider updating the comment on lines 191-193 to explicitly mention returns:
-// Logical and comparison operators don't need parentheses in a simple assignment. +// Logical and comparison operators don't need parentheses in a simple assignment or return.This would align with the addition of
T_RETURNon line 69.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/models/FrmEntryValidate.phpclasses/models/FrmField.phpclasses/models/FrmTableHTMLGenerator.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantParenthesesSniff.php
✅ Files skipped from review due to trivial changes (1)
- classes/models/FrmTableHTMLGenerator.php
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmField.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: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (2)
classes/models/FrmEntryValidate.php (1)
683-683: LGTM! Clean formatting improvement.The removal of outer parentheses around the boolean expression is correct and improves readability without changing behavior.
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantParenthesesSniff.php (1)
61-72: LGTM! Good addition to handle return statements.Adding
T_RETURNto the list of valid preceding tokens is a logical extension that will catch redundant parentheses in return statements likereturn ( $value );. The variable rename to$validPrecedingTokensis also more accurate.
Crabcyborg
changed the title
Remove more useless parantheses
1 participant
This update tweaks the redundant parentheses sniff to catch a few more.