New sniff to enforce early returns when there are nested conditions#2832
Closed
Crabcyborg wants to merge 1 commit into
Closed
New sniff to enforce early returns when there are nested conditions#2832Crabcyborg wants to merge 1 commit into
Crabcyborg wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughA comprehensive refactoring introduces a PHP_CodeSniffer sniff to detect and fix nested if statements convertible to early returns. The sniff has been applied systematically across dozens of controller, model, helper, and integration files, inverting conditions and restructuring control flow while preserving behavioral equivalence. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
classes/controllers/FrmSMTPController.php (1)
91-98: PHPDoc type hints conflict with the new guard, causing the Psalm error.The
@param arrayannotation tells Psalm that$upsellis always an array, making the! is_array()check appear as dead code. Update the PHPDoc to reflect the defensive handling.📝 Suggested fix
/** * Don't nag people to install WPForms * * `@since` 4.04.04 * - * `@param` array $upsell + * `@param` mixed $upsell * - * `@return` array + * `@return` mixed */ public static function remove_wpforms_nag( $upsell ) {square/helpers/FrmSquareLiteConnectHelper.php (1)
314-325: Fix undefined$passwordin the early-return and conditional-assignment paths.Psalm/PHPStan flag
$passwordas undefined at line 315 (early return) and line 324 (when$pro_licenseis falsy). At line 315,$passwordis never initialized. At line 324,$passwordremains undefined when the condition at line 320 fails.🐛 Proposed fix
private static function maybe_get_pro_license() { if ( ! FrmAppHelper::pro_is_installed() ) { - return ! empty( $password ) ? $password : false; + return false; } - $pro_license = FrmAddonsController::get_pro_license(); - - if ( $pro_license ) { - $password = $pro_license; - } - - return ! empty( $password ) ? $password : false; + $password = FrmAddonsController::get_pro_license(); + return ! empty( $password ) ? $password : false; }
🤖 Fix all issues with AI agents
In `@classes/controllers/FrmAddonsController.php`:
- Around line 1224-1242: Remove the unreachable trailing "return $result;"
leftover in FrmAddonsController.php: inside the error handling block that checks
is_wp_error($result) and handles the 'no_plugin_header' case,
wp_send_json_error() call, and the final return of the error array (which
already returns), delete the final redundant return $result; to eliminate dead
code and avoid confusion; reference the $result variable,
get_error_code()/get_error_message() usage and the wp_send_json_error() call
when locating the redundant statement.
In `@classes/controllers/FrmSMTPController.php`:
- Around line 100-103: The loop in FrmSMTPController that unsets upsell items
assumes each $plugin is an array with a 'slug' key and may throw notices; update
the foreach over $upsell to first validate the element (e.g., check
is_array($plugin) and isset($plugin['slug']) or array_key_exists('slug',
$plugin')) before calling str_contains on $plugin['slug'], and skip or unset
malformed entries accordingly to avoid errors when structure is unexpected.
In `@classes/helpers/FrmEmailSummaryHelper.php`:
- Around line 191-206: Remove the unreachable trailing "return false;" after the
computed renewal date is returned in the method that computes renewal dates (the
block using $first_form_date, YEARLY_PERIOD, BEFORE_RENEWAL_PERIOD,
self::get_date_from_today and self::save_options). Specifically, locate the
method in FrmEmailSummaryHelper that returns $renewal_date (it already returns
false earlier when !$first_form_date), delete the final dead "return false;" so
control flow is clean and there are no unreachable statements.
In `@classes/helpers/FrmTipsHelper.php`:
- Around line 94-104: The final return in FrmTipsHelper.php is unreachable due
to the earlier return of $cta_link; remove the dead statement "return
FrmAppHelper::admin_upgrade_link( $tip['link'], $tip['page'] );" so control and
returns are consistent; locate the block handling $cta_link and $tip
(references: $cta_link, $tip, FrmAppHelper::maybe_add_missing_utm and
FrmAppHelper::admin_upgrade_link) and delete the unreachable return to eliminate
dead code.
In `@classes/models/FrmCreateFile.php`:
- Around line 282-297: The code builds a sanitized $stored_credentials but
returns $credentials and leaves an unreachable "return false;"—fix by either (A)
returning the sanitized $stored_credentials instead of $credentials (so
sensitive fields are removed and port is appended), or (B) persisting
$stored_credentials (e.g., via the existing persistence mechanism used
elsewhere) and then returning an appropriate success value; remove the dead
"return false;" after the return and ensure changes occur in the FrmCreateFile
method/block where $stored_credentials is created so the sanitized credentials
are actually used or saved.
In `@classes/models/FrmEntryValidate.php`:
- Around line 523-530: The code uses a falsy check on $pattern to detect the
first segment which fails when the first segment is "0" or empty; in the loop
over $parts (from explode('\?',$pattern)) replace the falsy check with a proper
first-iteration detection—either call array_shift to handle the first part
separately or introduce an explicit boolean flag (e.g., $isFirst = true) and
flip it after processing the first part—so that segments like "0" or leading
empty segments are preserved and subsequent parts are wrapped as optional groups
exactly as intended.
In `@classes/models/FrmStyle.php`:
- Around line 338-345: The current logic uses rtrim($output, '()') which strips
all trailing parentheses and then appends one ), breaking nested groups; update
the normalization in the method using $output (in FrmStyle.php) to balance
parentheses instead: count occurrences of '(' and ')' in $output (e.g., using
substr_count) and if there are more ')' than '(' remove only the excess closing
parens from the end, if there are more '(' than ')' append the needed number of
')' to balance, and otherwise leave $output untouched; replace the rtrim(...)
plus str_contains(...) block with this parity-based trimming/append logic so
nested parentheses like calc((...)) remain valid.
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipNestedIfToEarlyReturnSniff.php`:
- Around line 250-320: The applyFix method currently declares an unused
parameter $funcOpener; remove $funcOpener from the method signature of applyFix
in FlipNestedIfToEarlyReturnSniff and update all call sites to stop passing that
argument (search for applyFix(...) usages to update them). Ensure only the
function signature and callers are changed—no other logic needs modification—and
run static analysis/tests to confirm no remaining references to $funcOpener.
In `@stripe/helpers/FrmStrpLiteConnectHelper.php`:
- Around line 409-411: Remove the unreachable dead code after the unconditional
return true in the FrmStrpLiteConnectHelper class method that currently contains
both "return true;" and "return false;": delete the trailing "return false;" so
the method only returns once, and run tests to ensure behavior is unchanged;
reference the method inside FrmStrpLiteConnectHelper where the two return
statements appear to locate and fix this.
- Around line 588-598: The code references an undefined $password when Pro is
not installed; update the early-return branch that checks
FrmAppHelper::pro_is_installed() to return false directly (rather than
referencing $password), so change the block that currently does "return ! empty(
$password ) ? $password : false;" to simply "return false;". Keep the later
logic that calls FrmAddonsController::get_pro_license() and assigns $password
when a pro license exists, then return the non-empty $password as before.
- Around line 183-185: The current method in FrmStrpLiteConnectHelper.php
returns a literal string ('Response from server was not successful') before a
second return ($body->data ?? array()), making the latter unreachable; remove
the dead first return and instead ensure the function returns the parsed body
data (e.g., return $body->data ?? []), or if an error case is intended, convert
the early literal into a conditional branch that returns the error only when the
response is unsuccessful and otherwise returns $body->data; update the function
(the block containing the two return statements) accordingly.
🧹 Nitpick comments (3)
classes/helpers/FrmFormsHelper.php (1)
2125-2131: Simplify boolean return for readability.You can return the truthiness directly to reduce noise.
♻️ Proposed tweak
- $api = FrmProFormState::get_from_request( 'a', 0 ); - - return $api ? true : false; + $api = FrmProFormState::get_from_request( 'a', 0 ); + return (bool) $api;classes/models/FrmEntryValues.php (1)
316-326: LGTM! Early return pattern correctly applied.The refactoring preserves the original logic while introducing an early exit when
exclude_fieldsis empty, avoiding unnecessary computation. The logic flow is clear: include check → early return if no exclusions → exclusion check.Optional simplification: Since we've established the early return pattern, lines 322-324 could be further simplified:
♻️ Optional: Simplify the exclusion check
$is_excluded = $this->is_field_in_array( $field, $this->exclude_fields ); -if ( $is_excluded ) { - $is_included = false; -} - -return $is_included; +return ! $is_excluded && $is_included;Or using another early return:
$is_excluded = $this->is_field_in_array( $field, $this->exclude_fields ); -if ( $is_excluded ) { - $is_included = false; +if ( $is_excluded ) { + return false; } return $is_included;square/helpers/FrmSquareLiteConnectHelper.php (1)
200-210: Remove the now‑unreachable fallback return.After the new error return on Line 208, the fallback
return $body->data ?? array();on Line 210 can never execute. Consider removing it to avoid confusion.♻️ Proposed cleanup
- return $body->data ?? array();
Comment on lines
+1224
to
1242
| if ( ! is_wp_error( $result ) ) { | ||
| return $result; | ||
| } | ||
|
|
||
| return array( | ||
| 'message' => $result->get_error_message(), | ||
| 'success' => false, | ||
| ); | ||
| } | ||
| // Ignore the invalid header message that shows with nested plugins. | ||
| if ( $result->get_error_code() === 'no_plugin_header' ) { | ||
| return $result; | ||
| } | ||
|
|
||
| if ( wp_doing_ajax() ) { | ||
| wp_send_json_error( array( 'error' => $result->get_error_message() ) ); | ||
| } | ||
|
|
||
| return array( | ||
| 'message' => $result->get_error_message(), | ||
| 'success' => false, | ||
| ); | ||
|
|
||
| return $result; |
Contributor
There was a problem hiding this comment.
Unreachable code detected.
Line 1242 return $result; is unreachable. All code paths before it either return or terminate:
- Line 1225: returns early for non-WP_Error
- Line 1230: returns for
no_plugin_headererror - Line 1234:
wp_send_json_error()terminates execution - Lines 1237-1240: returns error array
This appears to be leftover from the refactoring. The line should be removed.
🐛 Proposed fix
return array(
'message' => $result->get_error_message(),
'success' => false,
);
-
- return $result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( ! is_wp_error( $result ) ) { | |
| return $result; | |
| } | |
| return array( | |
| 'message' => $result->get_error_message(), | |
| 'success' => false, | |
| ); | |
| } | |
| // Ignore the invalid header message that shows with nested plugins. | |
| if ( $result->get_error_code() === 'no_plugin_header' ) { | |
| return $result; | |
| } | |
| if ( wp_doing_ajax() ) { | |
| wp_send_json_error( array( 'error' => $result->get_error_message() ) ); | |
| } | |
| return array( | |
| 'message' => $result->get_error_message(), | |
| 'success' => false, | |
| ); | |
| return $result; | |
| if ( ! is_wp_error( $result ) ) { | |
| return $result; | |
| } | |
| // Ignore the invalid header message that shows with nested plugins. | |
| if ( $result->get_error_code() === 'no_plugin_header' ) { | |
| return $result; | |
| } | |
| if ( wp_doing_ajax() ) { | |
| wp_send_json_error( array( 'error' => $result->get_error_message() ) ); | |
| } | |
| return array( | |
| 'message' => $result->get_error_message(), | |
| 'success' => false, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@classes/controllers/FrmAddonsController.php` around lines 1224 - 1242, Remove
the unreachable trailing "return $result;" leftover in FrmAddonsController.php:
inside the error handling block that checks is_wp_error($result) and handles the
'no_plugin_header' case, wp_send_json_error() call, and the final return of the
error array (which already returns), delete the final redundant return $result;
to eliminate dead code and avoid confusion; reference the $result variable,
get_error_code()/get_error_message() usage and the wp_send_json_error() call
when locating the redundant statement.
Comment on lines
+100
to
103
| foreach ( $upsell as $k => $plugin ) { | ||
| if ( str_contains( $plugin['slug'], 'wpforms' ) ) { | ||
| unset( $upsell[ $k ] ); | ||
| } |
Contributor
There was a problem hiding this comment.
Potential error if array element lacks expected structure.
The code accesses $plugin['slug'] without verifying $plugin is an array with a 'slug' key. If any element is malformed, this will trigger a notice or error.
🛡️ Suggested defensive check
foreach ( $upsell as $k => $plugin ) {
- if ( str_contains( $plugin['slug'], 'wpforms' ) ) {
+ if ( is_array( $plugin ) && isset( $plugin['slug'] ) && str_contains( $plugin['slug'], 'wpforms' ) ) {
unset( $upsell[ $k ] );
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| foreach ( $upsell as $k => $plugin ) { | |
| if ( str_contains( $plugin['slug'], 'wpforms' ) ) { | |
| unset( $upsell[ $k ] ); | |
| } | |
| foreach ( $upsell as $k => $plugin ) { | |
| if ( is_array( $plugin ) && isset( $plugin['slug'] ) && str_contains( $plugin['slug'], 'wpforms' ) ) { | |
| unset( $upsell[ $k ] ); | |
| } |
🤖 Prompt for AI Agents
In `@classes/controllers/FrmSMTPController.php` around lines 100 - 103, The loop
in FrmSMTPController that unsets upsell items assumes each $plugin is an array
with a 'slug' key and may throw notices; update the foreach over $upsell to
first validate the element (e.g., check is_array($plugin) and
isset($plugin['slug']) or array_key_exists('slug', $plugin')) before calling
str_contains on $plugin['slug'], and skip or unset malformed entries accordingly
to avoid errors when structure is unexpected.
Comment on lines
+191
to
206
| if ( ! $first_form_date ) { | ||
| return false; | ||
| } | ||
|
|
||
| // If the first form is more than 1 year in the past, set renewal date to the next 45 days. | ||
| if ( $renewal_date < self::get_date_from_today() ) { | ||
| $renewal_date = self::get_date_from_today( '+' . self::BEFORE_RENEWAL_PERIOD . ' days' ); | ||
| } | ||
| $renewal_date = gmdate( 'Y-m-d', strtotime( $first_form_date . '+' . self::YEARLY_PERIOD . ' days' ) ); | ||
|
|
||
| $options['renewal_date'] = $renewal_date; | ||
| self::save_options( $options ); | ||
| return $renewal_date; | ||
| // If the first form is more than 1 year in the past, set renewal date to the next 45 days. | ||
| if ( $renewal_date < self::get_date_from_today() ) { | ||
| $renewal_date = self::get_date_from_today( '+' . self::BEFORE_RENEWAL_PERIOD . ' days' ); | ||
| } | ||
|
|
||
| $options['renewal_date'] = $renewal_date; | ||
| self::save_options( $options ); | ||
| return $renewal_date; | ||
|
|
||
| return false; |
Contributor
There was a problem hiding this comment.
Unreachable code: return false; on line 206 will never execute.
After the early return refactoring, the control flow always either:
- Returns
falseat line 192 if$first_form_dateis falsy, or - Returns
$renewal_dateat line 204 after computing and saving it.
Line 206 is dead code that should be removed.
Proposed fix
$options['renewal_date'] = $renewal_date;
self::save_options( $options );
return $renewal_date;
-
- return false;
}🤖 Prompt for AI Agents
In `@classes/helpers/FrmEmailSummaryHelper.php` around lines 191 - 206, Remove the
unreachable trailing "return false;" after the computed renewal date is returned
in the method that computes renewal dates (the block using $first_form_date,
YEARLY_PERIOD, BEFORE_RENEWAL_PERIOD, self::get_date_from_today and
self::save_options). Specifically, locate the method in FrmEmailSummaryHelper
that returns $renewal_date (it already returns false earlier when
!$first_form_date), delete the final dead "return false;" so control flow is
clean and there are no unreachable statements.
Comment on lines
+94
to
104
| if ( ! $cta_link ) { | ||
| return FrmAppHelper::admin_upgrade_link( $tip['link'], $tip['page'] ); | ||
| } | ||
|
|
||
| return $cta_link; | ||
| if ( is_array( $tip['link'] ) ) { | ||
| $cta_link = FrmAppHelper::maybe_add_missing_utm( $cta_link, $tip['link'] ); | ||
| } | ||
|
|
||
| return $cta_link; | ||
|
|
||
| return FrmAppHelper::admin_upgrade_link( $tip['link'], $tip['page'] ); |
Contributor
There was a problem hiding this comment.
Dead code on line 104 — unreachable return statement.
The early-return refactoring introduced dead code. Line 104 can never execute because line 102 always returns before it. Remove the unreachable return statement.
Proposed fix
if ( is_array( $tip['link'] ) ) {
$cta_link = FrmAppHelper::maybe_add_missing_utm( $cta_link, $tip['link'] );
}
return $cta_link;
-
- return FrmAppHelper::admin_upgrade_link( $tip['link'], $tip['page'] );
}🤖 Prompt for AI Agents
In `@classes/helpers/FrmTipsHelper.php` around lines 94 - 104, The final return in
FrmTipsHelper.php is unreachable due to the earlier return of $cta_link; remove
the dead statement "return FrmAppHelper::admin_upgrade_link( $tip['link'],
$tip['page'] );" so control and returns are consistent; locate the block
handling $cta_link and $tip (references: $cta_link, $tip,
FrmAppHelper::maybe_add_missing_utm and FrmAppHelper::admin_upgrade_link) and
delete the unreachable return to eliminate dead code.
Comment on lines
+282
to
297
| if ( ! ( $has_creds || $can_ssh ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| unset( $stored_credentials['password'], $stored_credentials['port'], $stored_credentials['private_key'], $stored_credentials['public_key'] ); | ||
| $stored_credentials = $credentials; | ||
|
|
||
| return $credentials; | ||
| if ( ! empty( $stored_credentials['port'] ) ) { | ||
| // Save port as part of hostname to simplify above code. | ||
| $stored_credentials['hostname'] .= ':' . $stored_credentials['port']; | ||
| } | ||
|
|
||
| unset( $stored_credentials['password'], $stored_credentials['port'], $stored_credentials['private_key'], $stored_credentials['public_key'] ); | ||
|
|
||
| return $credentials; | ||
|
|
||
| return false; |
Contributor
There was a problem hiding this comment.
Critical: Unreachable code and unused variable indicate incomplete refactor.
Two issues in this segment:
-
Line 297 is unreachable dead code —
return false;can never execute becausereturn $credentials;on line 295 always executes first when the early guard passes. -
$stored_credentialsis computed but never used — The code builds$stored_credentialswith port appended and sensitive fields unset, but then returns$credentialsinstead. This means:- The sensitive fields (
password,private_key,public_key) are still present in the returned value - All modifications to
$stored_credentialsare wasted
- The sensitive fields (
Was the intent to return $stored_credentials (sanitized credentials), or is there a missing update_option() call to persist it?
🐛 Proposed fix (assuming intent was to return sanitized credentials)
if ( ! ( $has_creds || $can_ssh ) ) {
return false;
}
$stored_credentials = $credentials;
if ( ! empty( $stored_credentials['port'] ) ) {
// Save port as part of hostname to simplify above code.
$stored_credentials['hostname'] .= ':' . $stored_credentials['port'];
}
unset( $stored_credentials['password'], $stored_credentials['port'], $stored_credentials['private_key'], $stored_credentials['public_key'] );
- return $credentials;
-
- return false;
+ return $credentials;If $stored_credentials was meant to be saved or returned instead, please clarify the intended behavior.
🤖 Prompt for AI Agents
In `@classes/models/FrmCreateFile.php` around lines 282 - 297, The code builds a
sanitized $stored_credentials but returns $credentials and leaves an unreachable
"return false;"—fix by either (A) returning the sanitized $stored_credentials
instead of $credentials (so sensitive fields are removed and port is appended),
or (B) persisting $stored_credentials (e.g., via the existing persistence
mechanism used elsewhere) and then returning an appropriate success value;
remove the dead "return false;" after the return and ensure changes occur in the
FrmCreateFile method/block where $stored_credentials is created so the sanitized
credentials are actually used or saved.
Comment on lines
+338
to
+345
| if ( ! in_array( substr( $output, -1 ), array( '(', ')' ), true ) ) { | ||
| return $output; | ||
| } | ||
|
|
||
| if ( str_contains( $output, '(' ) ) { | ||
| $output .= ')'; | ||
| } | ||
| $output = rtrim( $output, '()' ); | ||
|
|
||
| if ( str_contains( $output, '(' ) ) { | ||
| $output .= ')'; |
Contributor
There was a problem hiding this comment.
Guard against breaking nested parentheses (Line 338-345).
rtrim( $output, '()' ) strips all trailing parentheses, then you append a single ). This collapses nested expressions like calc((100% - 10px)) into an unbalanced value (calc((100% - 10px)), which can corrupt valid CSS. If nested parentheses are valid inputs, this is a regression.
Consider skipping this normalization when multiple openings exist (or only trimming excess closings). For example:
🔧 Possible fix
- // Remove extra braces from the end.
- if ( ! in_array( substr( $output, -1 ), array( '(', ')' ), true ) ) {
- return $output;
- }
-
- $output = rtrim( $output, '()' );
-
- if ( str_contains( $output, '(' ) ) {
- $output .= ')';
- }
+ // Remove extra braces from the end.
+ if ( ! in_array( substr( $output, -1 ), array( '(', ')' ), true ) ) {
+ return $output;
+ }
+
+ // Avoid collapsing valid nested parentheses (e.g. calc((...))).
+ if ( substr_count( $output, '(' ) > 1 ) {
+ return $output;
+ }
+
+ $output = rtrim( $output, '()' );
+
+ if ( str_contains( $output, '(' ) ) {
+ $output .= ')';
+ }🤖 Prompt for AI Agents
In `@classes/models/FrmStyle.php` around lines 338 - 345, The current logic uses
rtrim($output, '()') which strips all trailing parentheses and then appends one
), breaking nested groups; update the normalization in the method using $output
(in FrmStyle.php) to balance parentheses instead: count occurrences of '(' and
')' in $output (e.g., using substr_count) and if there are more ')' than '('
remove only the excess closing parens from the end, if there are more '(' than
')' append the needed number of ')' to balance, and otherwise leave $output
untouched; replace the rtrim(...) plus str_contains(...) block with this
parity-based trimming/append logic so nested parentheses like calc((...)) remain
valid.
Comment on lines
+250
to
+320
| private function applyFix( File $phpcsFile, $ifToken, $returnToken, $returnSemicolon, $funcOpener ) { | ||
| $tokens = $phpcsFile->getTokens(); | ||
| $fixer = $phpcsFile->fixer; | ||
|
|
||
| $ifOpener = $tokens[ $ifToken ]['scope_opener']; | ||
| $ifCloser = $tokens[ $ifToken ]['scope_closer']; | ||
| $conditionOpener = $tokens[ $ifToken ]['parenthesis_opener']; | ||
| $conditionCloser = $tokens[ $ifToken ]['parenthesis_closer']; | ||
|
|
||
| // Get the original condition. | ||
| $originalCondition = ''; | ||
|
|
||
| for ( $i = $conditionOpener + 1; $i < $conditionCloser; $i++ ) { | ||
| $originalCondition .= $tokens[ $i ]['content']; | ||
| } | ||
| $originalCondition = trim( $originalCondition ); | ||
|
|
||
| // Negate the condition. | ||
| $negatedCondition = $this->negateCondition( $originalCondition ); | ||
|
|
||
| // Get the if body content. | ||
| $ifBodyContent = ''; | ||
|
|
||
| for ( $i = $ifOpener + 1; $i < $ifCloser; $i++ ) { | ||
| $ifBodyContent .= $tokens[ $i ]['content']; | ||
| } | ||
|
|
||
| // Get the return statement. | ||
| $returnStatement = ''; | ||
|
|
||
| for ( $i = $returnToken; $i <= $returnSemicolon; $i++ ) { | ||
| $returnStatement .= $tokens[ $i ]['content']; | ||
| } | ||
|
|
||
| // Dedent the if body. | ||
| $ifBodyContent = $this->dedentCode( $ifBodyContent ); | ||
|
|
||
| // Get the indentation. | ||
| $ifIndent = $this->getIndentation( $phpcsFile, $ifToken ); | ||
|
|
||
| $fixer->beginChangeset(); | ||
|
|
||
| // Replace the condition with negated version. | ||
| for ( $i = $conditionOpener + 1; $i < $conditionCloser; $i++ ) { | ||
| $fixer->replaceToken( $i, '' ); | ||
| } | ||
| $fixer->addContent( $conditionOpener, ' ' . $negatedCondition . ' ' ); | ||
|
|
||
| // Replace the if body with just the return statement. | ||
| for ( $i = $ifOpener + 1; $i < $ifCloser; $i++ ) { | ||
| $fixer->replaceToken( $i, '' ); | ||
| } | ||
| $fixer->addContent( $ifOpener, $phpcsFile->eolChar . $ifIndent . "\t" . $returnStatement . $phpcsFile->eolChar . $ifIndent ); | ||
|
|
||
| // Remove the old return statement. | ||
| for ( $i = $ifCloser + 1; $i <= $returnSemicolon; $i++ ) { | ||
| $fixer->replaceToken( $i, '' ); | ||
| } | ||
|
|
||
| // Add the dedented if body after the if, followed by the return. | ||
| $ifBodyContent = rtrim( $ifBodyContent ); | ||
| $fixer->addContent( | ||
| $ifCloser, | ||
| $phpcsFile->eolChar . $phpcsFile->eolChar . | ||
| $ifIndent . ltrim( $ifBodyContent ) . $phpcsFile->eolChar . | ||
| $phpcsFile->eolChar . | ||
| $ifIndent . $returnStatement . $phpcsFile->eolChar | ||
| ); | ||
|
|
||
| $fixer->endChangeset(); | ||
| } |
Contributor
There was a problem hiding this comment.
Remove unused $funcOpener parameter.
The parameter is never read, which trips static analysis and adds dead API surface.
🧩 Proposed fix
- if ( true === $fix ) {
- $this->applyFix( $phpcsFile, $ifToken, $returnToken, $lastToken, $funcOpener );
- }
+ if ( true === $fix ) {
+ $this->applyFix( $phpcsFile, $ifToken, $returnToken, $lastToken );
+ }- private function applyFix( File $phpcsFile, $ifToken, $returnToken, $returnSemicolon, $funcOpener ) {
+ private function applyFix( File $phpcsFile, $ifToken, $returnToken, $returnSemicolon ) {🧰 Tools
🪛 PHPMD (2.15.0)
250-250: Avoid unused parameters such as '$funcOpener'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipNestedIfToEarlyReturnSniff.php`
around lines 250 - 320, The applyFix method currently declares an unused
parameter $funcOpener; remove $funcOpener from the method signature of applyFix
in FlipNestedIfToEarlyReturnSniff and update all call sites to stop passing that
argument (search for applyFix(...) usages to update them). Ensure only the
function signature and callers are changed—no other logic needs modification—and
run static analysis/tests to confirm no remaining references to $funcOpener.
Comment on lines
+183
to
185
| return 'Response from server was not successful'; | ||
|
|
||
| return $body->data ?? array(); |
Contributor
There was a problem hiding this comment.
Unreachable dead code after return statement.
Line 185 is unreachable because line 183 always returns. This appears to be leftover code from the early-return refactoring that should be removed.
🐛 Proposed fix to remove unreachable code
return 'Response from server was not successful';
-
- return $body->data ?? array();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return 'Response from server was not successful'; | |
| return $body->data ?? array(); | |
| return 'Response from server was not successful'; | |
| } |
🤖 Prompt for AI Agents
In `@stripe/helpers/FrmStrpLiteConnectHelper.php` around lines 183 - 185, The
current method in FrmStrpLiteConnectHelper.php returns a literal string
('Response from server was not successful') before a second return ($body->data
?? array()), making the latter unreachable; remove the dead first return and
instead ensure the function returns the parsed body data (e.g., return
$body->data ?? []), or if an error case is intended, convert the early literal
into a conditional branch that returns the error only when the response is
unsuccessful and otherwise returns $body->data; update the function (the block
containing the two return statements) accordingly.
Comment on lines
+409
to
411
| return true; | ||
|
|
||
| return false; |
Contributor
There was a problem hiding this comment.
Unreachable dead code after return statement.
Line 411 is unreachable because line 409 always returns true. This leftover code from the refactoring should be removed.
🐛 Proposed fix to remove unreachable code
return true;
-
- return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return true; | |
| return false; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In `@stripe/helpers/FrmStrpLiteConnectHelper.php` around lines 409 - 411, Remove
the unreachable dead code after the unconditional return true in the
FrmStrpLiteConnectHelper class method that currently contains both "return
true;" and "return false;": delete the trailing "return false;" so the method
only returns once, and run tests to ensure behavior is unchanged; reference the
method inside FrmStrpLiteConnectHelper where the two return statements appear to
locate and fix this.
Comment on lines
+588
to
598
| if ( ! FrmAppHelper::pro_is_installed() ) { | ||
| return ! empty( $password ) ? $password : false; | ||
| } | ||
|
|
||
| if ( $pro_license ) { | ||
| $password = $pro_license; | ||
| } | ||
| $pro_license = FrmAddonsController::get_pro_license(); | ||
|
|
||
| if ( $pro_license ) { | ||
| $password = $pro_license; | ||
| } | ||
|
|
||
| return ! empty( $password ) ? $password : false; |
Contributor
There was a problem hiding this comment.
Undefined variable $password causes broken logic.
When Pro is not installed, line 589 references $password which has never been defined. The variable is only assigned on line 595, which is unreachable when Pro is not installed.
Based on the method's purpose and docstring, when Pro is not installed, this should return false so the caller falls back to the UUID.
🐛 Proposed fix
private static function maybe_get_pro_license() {
if ( ! FrmAppHelper::pro_is_installed() ) {
- return ! empty( $password ) ? $password : false;
+ return false;
}
$pro_license = FrmAddonsController::get_pro_license();
-
- if ( $pro_license ) {
- $password = $pro_license;
- }
-
- return ! empty( $password ) ? $password : false;
+ return $pro_license ? $pro_license : false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( ! FrmAppHelper::pro_is_installed() ) { | |
| return ! empty( $password ) ? $password : false; | |
| } | |
| if ( $pro_license ) { | |
| $password = $pro_license; | |
| } | |
| $pro_license = FrmAddonsController::get_pro_license(); | |
| if ( $pro_license ) { | |
| $password = $pro_license; | |
| } | |
| return ! empty( $password ) ? $password : false; | |
| private static function maybe_get_pro_license() { | |
| if ( ! FrmAppHelper::pro_is_installed() ) { | |
| return false; | |
| } | |
| $pro_license = FrmAddonsController::get_pro_license(); | |
| return $pro_license ? $pro_license : false; | |
| } |
🧰 Tools
🪛 GitHub Check: PHPStan
[failure] 589-589:
Variable $password in empty() is never defined.
[failure] 589-589:
Undefined variable: $password
🪛 GitHub Check: Psalm
[failure] 589-589: UndefinedVariable
stripe/helpers/FrmStrpLiteConnectHelper.php:589:20: UndefinedVariable: Cannot find referenced variable $password (see https://psalm.dev/024)
🤖 Prompt for AI Agents
In `@stripe/helpers/FrmStrpLiteConnectHelper.php` around lines 588 - 598, The code
references an undefined $password when Pro is not installed; update the
early-return branch that checks FrmAppHelper::pro_is_installed() to return false
directly (rather than referencing $password), so change the block that currently
does "return ! empty( $password ) ? $password : false;" to simply "return
false;". Keep the later logic that calls FrmAddonsController::get_pro_license()
and assigns $password when a pro license exists, then return the non-empty
$password as before.
Contributor
Author
|
This sniff isn't refactoring well and tying to reference undefined variables. Closing for now. |
Crabcyborg
deleted the
new_sniff_to_enforce_early_returns_over_nested_ifs
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.