New early return sniff#2828
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR applies guard-clause/early-return refactors across multiple helpers, models, controllers and field classes, and adds a new PHP_CodeSniffer sniff (FlipIfToEarlyReturnVariableSniff) with a ruleset entry to detect and auto-fix if->early-return variable patterns. Changes
Sequence Diagram(s)(omitted — changes are refactors and a static analysis addition that do not introduce a multi-component runtime sequence needing visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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: 4
🤖 Fix all issues with AI agents
In `@classes/helpers/FrmEmailSummaryHelper.php`:
- Around line 78-83: In FrmEmailSummaryHelper, remove the unreachable dead
return statement (the final "return $options;") that comes after
"self::save_options($default_options); return $default_options;"; locate the
block where self::save_options, $default_options and $options are used and
delete the trailing return $options; so the method returns only $default_options
and has no unreachable code.
In `@classes/helpers/FrmFormsHelper.php`:
- Around line 1346-1368: The current logic in FrmFormsHelper::delete_trash_links
(handling $link_details in the code block building $link) uses an elseif so when
$link_details['data'] exists the confirm onclick is skipped; change the flow to
allow both data attributes and the confirm fallback to be added: replace the
elseif with a separate if that appends the onclick confirm handler when
isset($link_details['confirm']) in addition to the existing loop that adds
data-* attributes from $link_details['data'], ensuring you keep escaping via
esc_attr() and esc_url() and still build $link and $label as before (references:
$link_details, $link, $length).
In `@classes/models/FrmEntryFormatter.php`:
- Around line 971-973: Fix the syntax error in FrmEntryFormatter where the
condition reads "$this - <= is_plain_text": replace this malformed expression
with a proper property check using the object operator and negation (i.e., check
that is_plain_text is false) so the early-return that returns $value executes
when ! $this->is_plain_text; update the condition in the method containing that
return to use $this->is_plain_text with correct negation and object operator.
In `@classes/models/FrmForm.php`:
- Around line 730-746: The current check treats $query_results falsy and returns
early, which incorrectly skips cleanup when $wpdb->query() returns 0 (no rows
deleted); change the guard in the delete flow to only return on strict false
(error) instead of falsy, so that
FrmFormActionsController::get_form_actions('email')->destroy($id, 'all'),
self::clear_form_cache(), and the do_action('frm_destroy_form', $id) /
do_action('frm_destroy_form_' . $id) calls still run when $query_results === 0
but not when $query_results === false.
🧹 Nitpick comments (8)
classes/models/fields/FrmFieldUrl.php (1)
110-113: Simplify return value per Rector suggestion.The assignment to
$valuebefore returning is unnecessary. As flagged by the pipeline, this can be simplified by returning$imagesdirectly. Also removes the extra blank lines.♻️ Suggested fix
} - $value = $images; - - - return $value; + return $images; }classes/models/fields/FrmFieldType.php (2)
1468-1481: LGTM! Clean early-return refactor.The flip from nested
if (!$readonly)to an early return for the readonly case improves readability while preserving the original behavior.Minor nit: Lines 1479-1480 have two consecutive blank lines before the return statement, which could be reduced to one for consistency.
🧹 Optional: Remove extra blank line
$select_atts['name'] = $values['field_name']; $select_atts['id'] = $values['html_id']; - return $select_atts;
1716-1728: LGTM! Early-return guard correctly applied.The refactoring correctly returns non-array values early, then handles array processing (show attribute lookup or implode with separator). Logic is preserved.
Same minor nit as above: Lines 1726-1727 have two consecutive blank lines that could be consolidated.
🧹 Optional: Remove extra blank line
$sep = $atts['sep'] ?? ', '; $value = FrmAppHelper::safe_implode( $sep, $value ); } - return $value;phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnVariableSniff.php (1)
52-54: Consider addingT_CLOSUREto the registered tokens.The sniff currently only triggers on
T_FUNCTION. If the early-return pattern should also apply to closures/anonymous functions, consider:public function register() { - return array( T_FUNCTION ); + return array( T_FUNCTION, T_CLOSURE ); }If this is intentional (e.g., closures are typically short), this comment can be ignored.
classes/models/FrmEntryFormatter.php (1)
979-986: Redundant condition after early return fix.Once line 971 is fixed to
if ( ! $this->is_plain_text ), the condition on line 979 becomes redundant since:
$this->is_plain_textis guaranteed to be true (we returned early if not)! is_array( $value )is guaranteed true (the precedingifblock handles arrays)♻️ Suggested simplification
if ( is_array( $value ) ) { foreach ( $value as $key => $single_value ) { $value[ $key ] = $this->strip_html( $single_value ); } - } elseif ( $this->is_plain_text && ! is_array( $value ) ) { + } else { if ( str_contains( $value, '<img' ) ) { $value = str_replace( array( '<img', 'src=', '/>', '"' ), '', $value ); $value = trim( $value ); } $value = strip_tags( $value ); } - return $value;classes/models/fields/FrmFieldGdpr.php (1)
182-184: Logic is correct, but the double negation reduces readability.The condition
! ( ! $errors && ! FrmFieldGdprHelper::hide_gdpr_field() )is logically equivalent to$errors || FrmFieldGdprHelper::hide_gdpr_field()via De Morgan's law. Consider simplifying for clarity:♻️ Optional: Improve readability using De Morgan's simplification
- if ( ! ( ! $errors && ! FrmFieldGdprHelper::hide_gdpr_field() ) ) { + if ( $errors || FrmFieldGdprHelper::hide_gdpr_field() ) { return $errors; }classes/helpers/FrmXMLHelper.php (1)
775-798: LGTM - Early return correctly preserves the conditional logic.The guard clause properly handles the case where
$type !== 'default_blank'or$optionsis not an array, returning$changesearly. The remaining logic for processing dropdown placeholder options is unchanged.Note: Line 798 has an orphaned
// end ifcomment that's no longer relevant after the refactor.🧹 Remove orphaned comment
$changes['options'] = $options; - // end if return $changes;classes/controllers/FrmSMTPController.php (1)
412-424: Consider simplifying the double-negated condition.The current condition
! ( ! is_object( $phpmailer ) || ! is_a( $phpmailer, 'PHPMailer' ) )applies De Morgan's law but leaves unnecessary negations. This can be simplified for readability.♻️ Simplify condition
- if ( ! ( ! is_object( $phpmailer ) || ! is_a( $phpmailer, 'PHPMailer' ) ) ) { + if ( is_object( $phpmailer ) && $phpmailer instanceof PHPMailer ) { return $phpmailer; }Note: Using
instanceofis preferred overis_a()for type checking in PHP as it's more performant and idiomatic.
|
@coderabbitai re-review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@classes/models/fields/FrmFieldCaptcha.php`:
- Around line 320-332: The current logic in FrmFieldCaptcha.php returns success
when $response['success'] is missing or true; change it to fail closed by only
treating validation as passed when the success key is present and truthy.
Replace the early-return condition with a check that returns $errors only when
isset($response['success']) && $response['success'] is true (otherwise proceed
to set the error entry in $errors['field' . $args['id']]); update the block
around $response, $errors, and the invalid message handling in the
FrmFieldCaptcha class accordingly.
In `@classes/models/fields/FrmFieldUrl.php`:
- Line 106: Replace the call to strip_tags when appending the URL with proper
HTML escaping: where FrmFieldUrl builds $images and currently does $images .=
strip_tags($url) (when $atts['html'] is true), use esc_html($url) instead so
special characters are escaped for safe HTML output; update the code path in
FrmFieldUrl that handles $atts['html'] to call esc_html on $url before
concatenation.
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnVariableSniff.php`:
- Around line 485-503: The negation fallback in FlipIfToEarlyReturnVariableSniff
returns '! ' . $condition without parentheses which breaks operator precedence
and flipComparisonOperator() may flip operators inside string literals; update
the fallback to return '! (' . $condition . ')' (matching the compound branch)
and, before calling flipComparisonOperator($condition), add a guard that rejects
conditions containing single or double quotes or escaped quotes (e.g. preg_match
on /['"]/) so flipComparisonOperator is skipped when $condition contains string
literals or patterns.
- Around line 269-281: The hasElseOrElseif method is skipping only whitespace
when searching after the if scope closer, so comments between the closing brace
and an else/elseif aren't skipped; update the phpcsFile->findNext call in
hasElseOrElseif to skip both T_WHITESPACE and comment tokens (e.g. T_COMMENT and
T_DOC_COMMENT) by passing an array of those token constants with the $exclude
flag set to true when searching from $scopeCloser + 1, then keep the existing
check that uses $next and returns in_array($tokens[$next]['code'], array(T_ELSE,
T_ELSEIF), true).
♻️ Duplicate comments (1)
classes/helpers/FrmFormsHelper.php (1)
1352-1358: Theelseifconcern remains unaddressed.As noted in a previous review, using
elseifhere means theconfirmonclick handler is skipped wheneverdataattributes exist. Looking atdelete_trash_links()(line 1419-1430), the 'delete' array defines bothdataandconfirmkeys, so the confirm fallback is currently being dropped.
🧹 Nitpick comments (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnVariableSniff.php (1)
229-233: Unused parameters flagged by PHPMD.
$phpcsFileisn’t used infindPrecedingIf,countStatementsInScope, orgetConditionString; consider removing it from signatures/call sites to quiet the linter.Also applies to: 293-295, 321-323
1 participant
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.