From 2be5d71a622a194eccbff6977c1f9f47f15bd3d2 Mon Sep 17 00:00:00 2001 From: Mike Letellier Date: Fri, 9 Jan 2026 12:16:13 -0400 Subject: [PATCH 1/2] Update redundant empty sniff to also catch conditions with && and || --- classes/controllers/FrmAddonsController.php | 6 +- .../controllers/FrmDashboardController.php | 2 +- classes/controllers/FrmEntriesController.php | 2 +- classes/helpers/FrmAppHelper.php | 4 +- classes/helpers/FrmXMLHelper.php | 4 +- classes/models/FrmPluginSearch.php | 2 +- classes/models/FrmSettings.php | 2 +- classes/models/FrmUsage.php | 2 +- classes/models/fields/FrmFieldUrl.php | 4 +- .../RedundantEmptyOnAssignedVariableSniff.php | 146 +++++++++++++++--- .../phpunit/misc/test_FrmCSVExportHelper.php | 2 +- 11 files changed, 136 insertions(+), 40 deletions(-) diff --git a/classes/controllers/FrmAddonsController.php b/classes/controllers/FrmAddonsController.php index d2c0640e71..8370a3547b 100644 --- a/classes/controllers/FrmAddonsController.php +++ b/classes/controllers/FrmAddonsController.php @@ -830,7 +830,7 @@ protected static function prepare_addons( &$addons ) { $addon['activate_url'] = ''; - if ( $addon['installed'] && ! empty( $activate_url ) && ! self::is_plugin_active( $file_name, $slug ) ) { + if ( $addon['installed'] && $activate_url && ! self::is_plugin_active( $file_name, $slug ) ) { $addon['activate_url'] = add_query_arg( array( '_wpnonce' => wp_create_nonce( 'activate-plugin_' . $file_name ), @@ -1337,7 +1337,7 @@ public static function can_install_addon_api() { // The download link is not required if already installed. $is_installed = FrmAppHelper::pro_is_included(); - $file_missing = ! $is_installed && empty( $post_url ); + $file_missing = ! $is_installed && ! $post_url; if ( ! $post_auth || $file_missing ) { return false; @@ -1345,7 +1345,7 @@ public static function can_install_addon_api() { // Verify auth. $auth = get_option( 'frm_connect_token' ); - return ! empty( $auth ) && hash_equals( $auth, $post_auth ); + return $auth && hash_equals( $auth, $post_auth ); } /** diff --git a/classes/controllers/FrmDashboardController.php b/classes/controllers/FrmDashboardController.php index eb816d4086..37083fd6ec 100644 --- a/classes/controllers/FrmDashboardController.php +++ b/classes/controllers/FrmDashboardController.php @@ -362,7 +362,7 @@ public static function entries_columns( $columns = array() ) { public static function welcome_banner_has_closed() { $user_id = get_current_user_id(); $banner_closed_by_users = self::get_closed_welcome_banner_user_ids(); - return ! empty( $banner_closed_by_users ) && in_array( $user_id, $banner_closed_by_users, true ); + return $banner_closed_by_users && in_array( $user_id, $banner_closed_by_users, true ); } /** diff --git a/classes/controllers/FrmEntriesController.php b/classes/controllers/FrmEntriesController.php index b1807f9a9f..19386b67ae 100644 --- a/classes/controllers/FrmEntriesController.php +++ b/classes/controllers/FrmEntriesController.php @@ -750,7 +750,7 @@ public static function destroy() { public static function process_entry( $errors = '', $ajax = false ) { $form_id = FrmAppHelper::get_post_param( 'form_id', '', 'absint' ); - if ( FrmAppHelper::is_admin() || empty( $_POST ) || empty( $form_id ) || ! isset( $_POST['item_key'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing + if ( FrmAppHelper::is_admin() || empty( $_POST ) || ! $form_id || ! isset( $_POST['item_key'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Missing return; } diff --git a/classes/helpers/FrmAppHelper.php b/classes/helpers/FrmAppHelper.php index 50f9df298b..f84bf77f70 100644 --- a/classes/helpers/FrmAppHelper.php +++ b/classes/helpers/FrmAppHelper.php @@ -3212,7 +3212,7 @@ private static function add_time_to_date( $time_format, $date ) { $trimmed_format = trim( $time_format ); - if ( $time_format && ! empty( $trimmed_format ) ) { + if ( $time_format && $trimmed_format ) { return ' ' . __( 'at', 'formidable' ) . ' ' . self::get_localized_date( $time_format, $date ); } @@ -3578,7 +3578,7 @@ public static function select_current_page( $page, $current_page, $action = arra $frm_action = 'reports'; } - if ( ! $action || ( ! empty( $frm_action ) && in_array( $frm_action, $action, true ) ) ) { + if ( ! $action || ( $frm_action && in_array( $frm_action, $action, true ) ) ) { echo ' class="current_page"'; } } diff --git a/classes/helpers/FrmXMLHelper.php b/classes/helpers/FrmXMLHelper.php index f41a65f5cf..0842570445 100644 --- a/classes/helpers/FrmXMLHelper.php +++ b/classes/helpers/FrmXMLHelper.php @@ -2371,7 +2371,7 @@ private static function migrate_autoresponder_to_action( $form_options, $form_id } } - if ( is_numeric( $email_field ) && ! empty( $email_field ) ) { + if ( is_numeric( $email_field ) && $email_field ) { $email_field = '[' . $email_field . ']'; } @@ -2394,7 +2394,7 @@ private static function migrate_autoresponder_to_action( $form_options, $form_id $new_notification2['post_content']['reply_to'] = $reply_to; } - if ( $reply_to || ! empty( $reply_to_name ) ) { + if ( $reply_to || $reply_to_name ) { $new_notification2['post_content']['from'] = ( empty( $reply_to_name ) ? '[sitename]' : $reply_to_name ) . ' <' . ( empty( $reply_to ) ? '[admin_email]' : $reply_to ) . '>'; } diff --git a/classes/models/FrmPluginSearch.php b/classes/models/FrmPluginSearch.php index f99dcc2422..3ceb1bbef5 100644 --- a/classes/models/FrmPluginSearch.php +++ b/classes/models/FrmPluginSearch.php @@ -223,7 +223,7 @@ private function maybe_dismiss() { */ protected function get_dismissed_hints() { $dismissed_hints = get_option( self::$dismissed_opt ); - return ! empty( $dismissed_hints ) && is_array( $dismissed_hints ) ? $dismissed_hints : array(); + return $dismissed_hints && is_array( $dismissed_hints ) ? $dismissed_hints : array(); } /** diff --git a/classes/models/FrmSettings.php b/classes/models/FrmSettings.php index f8bdf9a3a3..0e8ac0a720 100644 --- a/classes/models/FrmSettings.php +++ b/classes/models/FrmSettings.php @@ -350,7 +350,7 @@ private function set_default_options() { if ( is_multisite() && is_admin() ) { $mu_menu = get_site_option( 'frm_admin_menu_name' ); - if ( $mu_menu && ! empty( $mu_menu ) ) { + if ( $mu_menu && $mu_menu ) { $this->menu = $mu_menu; $this->mu_menu = 1; } diff --git a/classes/models/FrmUsage.php b/classes/models/FrmUsage.php index f462be0224..a4854278d7 100644 --- a/classes/models/FrmUsage.php +++ b/classes/models/FrmUsage.php @@ -54,7 +54,7 @@ public function uuid( $regenerate = false ) { $uuid_key = 'frm-usage-uuid'; $uuid = get_option( $uuid_key ); - if ( $regenerate || empty( $uuid ) ) { + if ( $regenerate || ! $uuid ) { // Definitely not cryptographically secure but // close enough to provide an unique id $uuid = md5( uniqid() . site_url() ); diff --git a/classes/models/fields/FrmFieldUrl.php b/classes/models/fields/FrmFieldUrl.php index 374f79edfa..082e08fa0d 100644 --- a/classes/models/fields/FrmFieldUrl.php +++ b/classes/models/fields/FrmFieldUrl.php @@ -68,7 +68,7 @@ protected function fill_default_atts( &$atts ) { public function validate( $args ) { $value = $args['value']; - if ( trim( $value ) === 'http://' || empty( $value ) ) { + if ( trim( $value ) === 'http://' || ! $value ) { $value = ''; } else { $value = esc_url_raw( $value ); @@ -82,7 +82,7 @@ public function validate( $args ) { // validate the url format if ( $value && ! preg_match( '/^http(s)?:\/\/(?:localhost|(?:[\da-z\.-]+\.[\da-z\.-]+))/i', $value ) ) { $errors[ 'field' . $args['id'] ] = FrmFieldsHelper::get_error_msg( $this->field, 'invalid' ); - } elseif ( $this->field->required == '1' && empty( $value ) ) { // phpcs:ignore Universal.Operators.StrictComparisons + } elseif ( $this->field->required == '1' && ! $value ) { // phpcs:ignore Universal.Operators.StrictComparisons $errors[ 'field' . $args['id'] ] = FrmFieldsHelper::get_error_msg( $this->field, 'blank' ); } diff --git a/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php b/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php index 82581f2b2d..e9b3594be3 100644 --- a/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php +++ b/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php @@ -53,8 +53,10 @@ public function register() { public function process( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); - // Only process if empty() is directly inside an if/elseif condition. - if ( ! $this->isInIfCondition( $phpcsFile, $stackPtr ) ) { + // Find the statement context (if condition, return statement, etc.). + $context = $this->getStatementContext( $phpcsFile, $stackPtr ); + + if ( false === $context ) { return; } @@ -92,7 +94,7 @@ public function process( File $phpcsFile, $stackPtr ) { } // Check if the variable was unconditionally assigned earlier in this function. - if ( ! $this->wasVariableUnconditionallyAssigned( $phpcsFile, $functionToken, $stackPtr, $variableName ) ) { + if ( ! $this->wasVariableUnconditionallyAssigned( $phpcsFile, $functionToken, $context, $variableName ) ) { return; } @@ -172,13 +174,38 @@ public function process( File $phpcsFile, $stackPtr ) { } } + /** + * Get the statement context for the empty() call. + * + * Returns the token position of the statement start (if, elseif, return, etc.) + * if the empty() is in a valid boolean context, or false otherwise. + * + * @param File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the empty token. + * + * @return int|false The statement token position, or false if not in a valid context. + */ + private function getStatementContext( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + // Check if in an if/elseif condition. + $ifContext = $this->isInIfCondition( $phpcsFile, $stackPtr ); + + if ( false !== $ifContext ) { + return $ifContext; + } + + // Check if in a boolean expression (with && or ||). + return $this->isInBooleanExpression( $phpcsFile, $stackPtr ); + } + /** * Check if the empty() call is directly inside an if/elseif condition. * * @param File $phpcsFile The file being scanned. * @param int $stackPtr The position of the empty token. * - * @return bool True if inside an if/elseif condition, false otherwise. + * @return int|false The if/elseif token position, or false if not in an if condition. */ private function isInIfCondition( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); @@ -200,7 +227,7 @@ private function isInIfCondition( File $phpcsFile, $stackPtr ) { $beforeCode = $tokens[ $beforeParen ]['code']; if ( $beforeCode === T_IF || $beforeCode === T_ELSEIF ) { - return true; + return $beforeParen; } } @@ -216,6 +243,82 @@ private function isInIfCondition( File $phpcsFile, $stackPtr ) { return false; } + /** + * Check if the empty() call is in a boolean expression (with && or ||). + * + * @param File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the empty token. + * + * @return int|false The statement token position, or false if not in a boolean expression. + */ + private function isInBooleanExpression( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + // Find the end of the empty() call. + $openParen = $phpcsFile->findNext( T_WHITESPACE, $stackPtr + 1, null, true ); + + if ( false === $openParen || $tokens[ $openParen ]['code'] !== T_OPEN_PARENTHESIS ) { + return false; + } + + $closeParen = $tokens[ $openParen ]['parenthesis_closer']; + + // Look for && or || after the empty() call. + $nextToken = $phpcsFile->findNext( T_WHITESPACE, $closeParen + 1, null, true ); + + if ( false !== $nextToken && in_array( $tokens[ $nextToken ]['code'], array( T_BOOLEAN_AND, T_BOOLEAN_OR ), true ) ) { + // Find the statement start (return, etc.). + return $this->findStatementStart( $phpcsFile, $stackPtr ); + } + + // Look for && or || before the empty() call (or before the ! if negated). + $searchFrom = $stackPtr; + $prevToken = $phpcsFile->findPrevious( T_WHITESPACE, $searchFrom - 1, null, true ); + + if ( false !== $prevToken && $tokens[ $prevToken ]['code'] === T_BOOLEAN_NOT ) { + $prevToken = $phpcsFile->findPrevious( T_WHITESPACE, $prevToken - 1, null, true ); + } + + if ( false !== $prevToken && in_array( $tokens[ $prevToken ]['code'], array( T_BOOLEAN_AND, T_BOOLEAN_OR ), true ) ) { + return $this->findStatementStart( $phpcsFile, $stackPtr ); + } + + return false; + } + + /** + * Find the start of the statement containing the given token. + * + * @param File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token. + * + * @return int|false The statement start token position, or false if not found. + */ + private function findStatementStart( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + for ( $i = $stackPtr - 1; $i >= 0; $i-- ) { + $code = $tokens[ $i ]['code']; + + // Found a statement start. + if ( in_array( $code, array( T_RETURN, T_ECHO, T_IF, T_ELSEIF ), true ) ) { + return $i; + } + + // Found a semicolon or opening brace - statement starts after this. + if ( $code === T_SEMICOLON || $code === T_OPEN_CURLY_BRACKET ) { + return $phpcsFile->findNext( T_WHITESPACE, $i + 1, null, true ); + } + + // Stop if we hit a function or class boundary. + if ( $code === T_FUNCTION || $code === T_CLOSURE || $code === T_CLASS ) { + return false; + } + } + + return false; + } + /** * Find the function or method that contains the given token. * @@ -244,31 +347,24 @@ private function findContainingFunction( File $phpcsFile, $stackPtr ) { * Check if the variable was unconditionally assigned earlier in the function. * * Only returns true if the variable is guaranteed to be set - i.e., it was - * assigned at the same scope level as the if statement (not inside a nested block). + * assigned at the same scope level as the statement (not inside a nested block). * - * @param File $phpcsFile The file being scanned. - * @param int $functionToken The position of the function token. - * @param int $emptyPtr The position of the empty() call. - * @param string $variableName The variable name to check. + * @param File $phpcsFile The file being scanned. + * @param int $functionToken The position of the function token. + * @param int $statementToken The position of the statement containing empty(). + * @param string $variableName The variable name to check. * * @return bool True if the variable was unconditionally assigned, false otherwise. */ - private function wasVariableUnconditionallyAssigned( File $phpcsFile, $functionToken, $emptyPtr, $variableName ) { + private function wasVariableUnconditionallyAssigned( File $phpcsFile, $functionToken, $statementToken, $variableName ) { $tokens = $phpcsFile->getTokens(); $scopeOpener = $tokens[ $functionToken ]['scope_opener']; - // Find the if/elseif statement that contains the empty() call. - $ifToken = $this->findContainingIf( $phpcsFile, $emptyPtr ); - - if ( false === $ifToken ) { - return false; - } - - // The if statement's level is what we compare against. - $ifLevel = $tokens[ $ifToken ]['level']; + // The statement's level is what we compare against. + $statementLevel = $tokens[ $statementToken ]['level']; - // Search from the function start to the if statement (not the empty call). - for ( $i = $scopeOpener + 1; $i < $ifToken; $i++ ) { + // Search from the function start to the statement. + for ( $i = $scopeOpener + 1; $i < $statementToken; $i++ ) { if ( $tokens[ $i ]['code'] !== T_VARIABLE ) { continue; } @@ -284,11 +380,11 @@ private function wasVariableUnconditionallyAssigned( File $phpcsFile, $functionT continue; } - // Check if the assignment is at the same scope level as the if statement. - // This ensures the variable was assigned unconditionally before the if. + // Check if the assignment is at the same scope level as the statement. + // This ensures the variable was assigned unconditionally before the statement. $assignmentLevel = $tokens[ $i ]['level']; - if ( $assignmentLevel === $ifLevel ) { + if ( $assignmentLevel === $statementLevel ) { return true; } } diff --git a/tests/phpunit/misc/test_FrmCSVExportHelper.php b/tests/phpunit/misc/test_FrmCSVExportHelper.php index e4c5f12b5f..47a94ce6ed 100644 --- a/tests/phpunit/misc/test_FrmCSVExportHelper.php +++ b/tests/phpunit/misc/test_FrmCSVExportHelper.php @@ -165,7 +165,7 @@ public function test_generate_csv() { 'form_cols' => FrmField::get_all_for_form( $form->id, '', 'include' ), ) ); - $this->assertTrue( is_string( $csv_path ) && ! empty( $csv_path ) && file_exists( $csv_path ) ); + $this->assertTrue( is_string( $csv_path ) && $csv_path && file_exists( $csv_path ) ); $csv_content = file_get_contents( $csv_path ); unlink( $csv_path ); From 62cdc0347557f3845e8b67b280f840ade35ae0dc Mon Sep 17 00:00:00 2001 From: Mike Letellier Date: Fri, 9 Jan 2026 12:18:56 -0400 Subject: [PATCH 2/2] Run sniff through php cs fixer --- .../RedundantEmptyOnAssignedVariableSniff.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php b/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php index e9b3594be3..032b5bb614 100644 --- a/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php +++ b/phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php @@ -183,7 +183,7 @@ public function process( File $phpcsFile, $stackPtr ) { * @param File $phpcsFile The file being scanned. * @param int $stackPtr The position of the empty token. * - * @return int|false The statement token position, or false if not in a valid context. + * @return false|int The statement token position, or false if not in a valid context. */ private function getStatementContext( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); @@ -205,7 +205,7 @@ private function getStatementContext( File $phpcsFile, $stackPtr ) { * @param File $phpcsFile The file being scanned. * @param int $stackPtr The position of the empty token. * - * @return int|false The if/elseif token position, or false if not in an if condition. + * @return false|int The if/elseif token position, or false if not in an if condition. */ private function isInIfCondition( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); @@ -249,7 +249,7 @@ private function isInIfCondition( File $phpcsFile, $stackPtr ) { * @param File $phpcsFile The file being scanned. * @param int $stackPtr The position of the empty token. * - * @return int|false The statement token position, or false if not in a boolean expression. + * @return false|int The statement token position, or false if not in a boolean expression. */ private function isInBooleanExpression( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); @@ -292,7 +292,7 @@ private function isInBooleanExpression( File $phpcsFile, $stackPtr ) { * @param File $phpcsFile The file being scanned. * @param int $stackPtr The position of the current token. * - * @return int|false The statement start token position, or false if not found. + * @return false|int The statement start token position, or false if not found. */ private function findStatementStart( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens();