Skip to content

New sniff to enforce early returns when there are nested conditions #2832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Crabcyborg wants to merge 1 commit into from
Closed

New sniff to enforce early returns when there are nested conditions #2832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 34 additions & 26 deletions classes/controllers/FrmAddonsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,20 +1221,24 @@ protected static function handle_addon_action( $installed, $action ) {
break;
}

if ( is_wp_error( $result ) ) {
// Ignore the invalid header message that shows with nested plugins.
if ( $result->get_error_code() !== 'no_plugin_header' ) {
if ( wp_doing_ajax() ) {
wp_send_json_error( array( 'error' => $result->get_error_message() ) );
}
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;
Comment on lines +1224 to 1242

@coderabbitai coderabbitai Bot Jan 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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_header error
  • 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.

}

Expand Down Expand Up @@ -1362,23 +1366,27 @@ public static function install_addon_api() {
}

// If empty license, save it now.
if ( empty( self::get_pro_license() ) && function_exists( 'load_formidable_pro' ) ) {
load_formidable_pro();
$license = stripslashes( FrmAppHelper::get_param( 'key', '', 'request', 'sanitize_text_field' ) );

if ( ! $license ) {
return array(
'success' => false,
'error' => 'That site does not have a valid license key.',
);
}
if ( ! ( empty( self::get_pro_license() ) && function_exists( 'load_formidable_pro' ) ) ) {
return array(
'success' => true,
);
}

$response = FrmAddon::activate_license_for_plugin( $license, 'formidable_pro' );
load_formidable_pro();
$license = stripslashes( FrmAppHelper::get_param( 'key', '', 'request', 'sanitize_text_field' ) );

if ( ! $response['success'] ) {
// Could not activate license.
return $response;
}
if ( ! $license ) {
return array(
'success' => false,
'error' => 'That site does not have a valid license key.',
);
}

$response = FrmAddon::activate_license_for_plugin( $license, 'formidable_pro' );

if ( ! $response['success'] ) {
// Could not activate license.
return $response;
}

return array(
Expand Down
16 changes: 9 additions & 7 deletions classes/controllers/FrmAppController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,14 +1196,16 @@ private static function maybe_add_wp_site_health() {
public static function api_install() {
delete_transient( 'frm_updating_api' );

if ( self::needs_update() ) {
$running = get_option( 'frm_install_running' );
if ( ! self::needs_update() ) {
return true;
}

if ( false === $running || $running < strtotime( '-5 minutes' ) ) {
update_option( 'frm_install_running', time(), 'no' );
self::install();
delete_option( 'frm_install_running' );
}
$running = get_option( 'frm_install_running' );

if ( false === $running || $running < strtotime( '-5 minutes' ) ) {
update_option( 'frm_install_running', time(), 'no' );
self::install();
delete_option( 'frm_install_running' );
}

return true;
Expand Down
30 changes: 17 additions & 13 deletions classes/controllers/FrmFieldsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -888,14 +888,16 @@ private static function maybe_add_error_html_for_js_validation( $field, array &$
private static function get_form_for_js_validation( $field ) {
global $frm_vars;

if ( ! empty( $frm_vars['js_validate_forms'] ) ) {
if ( isset( $frm_vars['js_validate_forms'][ $field['form_id'] ] ) ) {
return $frm_vars['js_validate_forms'][ $field['form_id'] ];
}
if ( empty( $frm_vars['js_validate_forms'] ) ) {
return false;
}

if ( ! empty( $field['parent_form_id'] ) && isset( $frm_vars['js_validate_forms'][ $field['parent_form_id'] ] ) ) {
return $frm_vars['js_validate_forms'][ $field['parent_form_id'] ];
}
if ( isset( $frm_vars['js_validate_forms'][ $field['form_id'] ] ) ) {
return $frm_vars['js_validate_forms'][ $field['form_id'] ];
}

if ( ! empty( $field['parent_form_id'] ) && isset( $frm_vars['js_validate_forms'][ $field['parent_form_id'] ] ) ) {
return $frm_vars['js_validate_forms'][ $field['parent_form_id'] ];
}

return false;
Expand Down Expand Up @@ -1056,12 +1058,14 @@ private static function add_pattern_attribute( $field, array &$add_html ) {
* @return mixed
*/
public static function check_value( $opt, $opt_key, $field ) {
if ( is_array( $opt ) ) {
if ( FrmField::is_option_true( $field, 'separate_value' ) ) {
$opt = $opt['value'] ?? $opt['label'] ?? reset( $opt );
} else {
$opt = $opt['label'] ?? reset( $opt );
}
if ( ! is_array( $opt ) ) {
return $opt;
}

if ( FrmField::is_option_true( $field, 'separate_value' ) ) {
$opt = $opt['value'] ?? $opt['label'] ?? reset( $opt );
} else {
$opt = $opt['label'] ?? reset( $opt );
}

return $opt;
Expand Down
10 changes: 6 additions & 4 deletions classes/controllers/FrmFormTemplatesController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,10 +667,12 @@ public static function append_new_template_to_nav( $nav_items, $nav_args ) {
$is_new_template = FrmAppHelper::simple_get( 'new_template' );

// Append 'new_template=true' to each nav item's link if 'new_template' exists in the URL.
if ( $is_new_template ) {
foreach ( $nav_items as &$item ) {
$item['link'] .= '&new_template=true';
}
if ( ! $is_new_template ) {
return $nav_items;
}

foreach ( $nav_items as &$item ) {
$item['link'] .= '&new_template=true';
}

return $nav_items;
Expand Down
32 changes: 18 additions & 14 deletions classes/controllers/FrmFormsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2475,13 +2475,15 @@ public static function show_form( $id = '', $key = '', $title = false, $descript
private static function maybe_get_form_to_show( $id ) {
$form = false;

if ( $id ) {
// Form id or key is set.
$form = FrmForm::getOne( $id );
if ( ! $id ) {
return $form;
}

if ( ! $form || $form->parent_form_id || $form->status === 'trash' ) {
$form = false;
}
// Form id or key is set.
$form = FrmForm::getOne( $id );

if ( ! $form || $form->parent_form_id || $form->status === 'trash' ) {
$form = false;
}

return $form;
Expand Down Expand Up @@ -2867,16 +2869,18 @@ private static function is_valid_on_submit_action( $action, $args, $event = 'cre
return ! empty( $action->post_content['success_url'] );
}

if ( 'page' === $action_type ) {
if ( empty( $action->post_content['success_page_id'] ) ) {
return false;
}
if ( 'page' !== $action_type ) {
return true;
}

$page = get_post( $action->post_content['success_page_id'] );
if ( empty( $action->post_content['success_page_id'] ) ) {
return false;
}

if ( ! $page || 'trash' === $page->post_status ) {
return false;
}
$page = get_post( $action->post_content['success_page_id'] );

if ( ! $page || 'trash' === $page->post_status ) {
return false;
}

return true;
Expand Down
12 changes: 7 additions & 5 deletions classes/controllers/FrmSMTPController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,13 @@
* @return array
*/
public static function remove_wpforms_nag( $upsell ) {
if ( is_array( $upsell ) ) {
foreach ( $upsell as $k => $plugin ) {
if ( str_contains( $plugin['slug'], 'wpforms' ) ) {
unset( $upsell[ $k ] );
}
if ( ! is_array( $upsell ) ) {
return $upsell;

Check failure on line 97 in classes/controllers/FrmSMTPController.php

View workflow job for this annotation

GitHub Actions / Psalm

NoValue 

classes/controllers/FrmSMTPController.php:97:4: NoValue: All possible types for this return were invalidated - This may be dead code (see https://psalm.dev/179)
}

foreach ( $upsell as $k => $plugin ) {
if ( str_contains( $plugin['slug'], 'wpforms' ) ) {
unset( $upsell[ $k ] );
}
Comment on lines +100 to 103

@coderabbitai coderabbitai Bot Jan 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

}

Expand Down
52 changes: 26 additions & 26 deletions classes/helpers/FrmAppHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,16 +497,14 @@ public static function is_admin_page( $page = 'formidable' ) {
global $pagenow;
$get_page = self::simple_get( 'page', 'sanitize_title' );

if ( $pagenow ) {
// allow this to be true during ajax load i.e. ajax form builder loading
$is_page = ( $pagenow === 'admin.php' || $pagenow === 'admin-ajax.php' ) && $get_page === $page;

if ( $is_page ) {
return true;
}
if ( ! $pagenow ) {
return is_admin() && $get_page === $page;
}

return is_admin() && $get_page === $page;
// allow this to be true during ajax load i.e. ajax form builder loading
$is_page = ( $pagenow === 'admin.php' || $pagenow === 'admin-ajax.php' ) && $get_page === $page;

return $is_page ? true : is_admin() && $get_page === $page;
}

/**
Expand Down Expand Up @@ -2749,12 +2747,14 @@ private static function maybe_truncate_key_before_appending( $column, $key ) {

$max_key_length_before_truncating = 60;

if ( strlen( $key ) > $max_key_length_before_truncating ) {
$key = substr( $key, 0, $max_key_length_before_truncating );
if ( strlen( $key ) <= $max_key_length_before_truncating ) {
return $key;
}

$key = substr( $key, 0, $max_key_length_before_truncating );

if ( is_numeric( $key ) ) {
$key .= 'a';
}
if ( is_numeric( $key ) ) {
$key .= 'a';
}

return $key;
Expand Down Expand Up @@ -3716,16 +3716,14 @@ public static function maybe_utf8_encode( $value ) {
return $value;
}

if ( function_exists( 'iconv' ) ) {
$converted = iconv( $from_format, $to_format, $value );

// Value is false if $value is not ISO-8859-1.
if ( false !== $converted ) {
return $converted;
}
if ( ! function_exists( 'iconv' ) ) {
return $value;
}

return $value;
$converted = iconv( $from_format, $to_format, $value );

// Value is false if $value is not ISO-8859-1.
return false !== $converted ? $converted : $value;
}

/**
Expand Down Expand Up @@ -4212,12 +4210,14 @@ public static function locales( $type = 'date' ) {
* @return string
*/
public static function get_menu_icon_class() {
if ( is_callable( 'FrmProAppHelper::get_settings' ) ) {
$settings = FrmProAppHelper::get_settings();
if ( ! is_callable( 'FrmProAppHelper::get_settings' ) ) {
return 'frmfont frm_logo_icon';
}

if ( is_object( $settings ) && ! empty( $settings->menu_icon ) ) {
return $settings->menu_icon;
}
$settings = FrmProAppHelper::get_settings();

if ( is_object( $settings ) && ! empty( $settings->menu_icon ) ) {
return $settings->menu_icon;
}

return 'frmfont frm_logo_icon';
Expand Down
20 changes: 11 additions & 9 deletions classes/helpers/FrmEmailSummaryHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,19 +188,21 @@ private static function get_renewal_date() {
// If renewal date doesn't exist, get from the first form creation date.
$first_form_date = self::get_earliest_form_created_date();

if ( $first_form_date ) {
$renewal_date = gmdate( 'Y-m-d', strtotime( $first_form_date . '+' . self::YEARLY_PERIOD . ' days' ) );
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;
Comment on lines +191 to 206

@coderabbitai coderabbitai Bot Jan 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Unreachable code: return false; on line 206 will never execute.

After the early return refactoring, the control flow always either:

  1. Returns false at line 192 if $first_form_date is falsy, or
  2. Returns $renewal_date at 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.

}

Expand Down
12 changes: 5 additions & 7 deletions classes/helpers/FrmEntriesHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,14 +970,12 @@ public static function get_visible_unread_inbox_count() {
return 0;
}

if ( is_callable( 'FrmProSettingsController::inbox_badge' ) ) {
$inbox_count = FrmProSettingsController::inbox_badge( $inbox_count );

if ( ! $inbox_count ) {
return 0;
}
if ( ! is_callable( 'FrmProSettingsController::inbox_badge' ) ) {
return $inbox_count;
}

return $inbox_count;
$inbox_count = FrmProSettingsController::inbox_badge( $inbox_count );

return $inbox_count ? $inbox_count : 0;
}
}
12 changes: 7 additions & 5 deletions classes/helpers/FrmEntriesListHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,14 @@ protected function get_order_by() {

FrmAppController::apply_saved_sort_preference( $orderby, $order );

if ( str_contains( $orderby, 'meta' ) ) {
$order_field_type = FrmField::get_type( str_replace( 'meta_', '', $orderby ) );
if ( ! str_contains( $orderby, 'meta' ) ) {
return FrmDb::esc_order( $orderby . ' ' . $order );
}

if ( in_array( $order_field_type, array( 'number', 'scale', 'star' ), true ) ) {
$orderby .= '+0';
}
$order_field_type = FrmField::get_type( str_replace( 'meta_', '', $orderby ) );

if ( in_array( $order_field_type, array( 'number', 'scale', 'star' ), true ) ) {
$orderby .= '+0';
}

return FrmDb::esc_order( $orderby . ' ' . $order );
Expand Down
Loading
Loading