Add new redundant empty sniff#2756
Conversation
WalkthroughLarge-scale refactoring replacing explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
classes/models/FrmFieldFormHtml.php (1)
287-301: Align$errortruthiness usage to avoid mixed semantics (and potential sniff noise).You switched the role-injection guard to
if ( $error && ... )(Line 291), but the shortcode condition still uses! empty( $error )(Line 300). If the broader PR is standardizing on truthiness checks, consider using the same predicate for both (or explicitly casting tobool) to prevent subtle drift and keep the intent consistent.Proposed tweak (consistent truthiness + safer default)
private function replace_error_shortcode() { $this->maybe_add_error_id(); - $error = $this->pass_args['errors'][ 'field' . $this->field_id ] ?? false; + $error = $this->pass_args['errors'][ 'field' . $this->field_id ] ?? ''; - if ( $error && ! str_contains( $this->html, 'role="alert"' ) && FrmAppHelper::should_include_alert_role_on_field_errors() ) { + if ( $error && ! str_contains( $this->html, 'role="alert"' ) && FrmAppHelper::should_include_alert_role_on_field_errors() ) { $error_body = self::get_error_body( $this->html ); if ( is_string( $error_body ) && ! str_contains( $error_body, 'role=' ) ) { $new_error_body = preg_replace( '/class="frm_error/', 'role="alert" class="frm_error', $error_body, 1 ); $this->html = str_replace( '[if error]' . $error_body . '[/if error]', '[if error]' . $new_error_body . '[/if error]', $this->html ); } } - FrmShortcodeHelper::remove_inline_conditions( ! empty( $error ), 'error', $error, $this->html ); + FrmShortcodeHelper::remove_inline_conditions( (bool) $error, 'error', $error, $this->html ); }stripe/models/FrmStrpLiteAuth.php (1)
756-765: Critical: Undefined variable notice introduced.The change from
empty($success_url)to!$success_urlintroduces an undefined variable notice when$actionsis falsy. PHP'sempty()function has special behavior that suppresses notices for undefined variables, but the!operator does not.Flow:
- If
$actionsis falsy (line 759),$success_urlis never initialized (line 760)- Line 763 then references an undefined
$success_url, generating a PHP notice- This will clutter error logs and may break strict error handling environments
🐛 Proposed fix
private static function get_redirect_url( $atts ) { $actions = FrmFormsController::get_met_on_submit_actions( $atts ); + $success_url = ''; if ( $actions ) { $success_url = reset( $actions )->post_content['success_url']; } if ( ! $success_url ) { $success_url = $atts['form']->options['success_url']; }stripe/helpers/FrmStrpLiteLinkRedirectHelper.php (1)
55-62: Critical: Undefined variable error.The variable
$refereris only defined inside theifblock on line 57, but it's referenced unconditionally on line 60. Whenempty($this->entry_id)evaluates totrue,$refererremains undefined, causing an "Undefined variable" warning.The previous
empty($referer)check would handle undefined variables gracefully (returningtrue), but! $refererdoes not.🐛 Proposed fix
public function handle_error( $error_code, $charge_id = '' ) { + $referer = ''; if ( ! empty( $this->entry_id ) ) { $referer = FrmStrpLiteAuth::get_referer_url( $this->entry_id ); } if ( ! $referer ) { $referer = FrmAppHelper::get_server_value( 'HTTP_REFERER' ); }Based on static analysis hints.
classes/helpers/FrmEntriesHelper.php (1)
249-266: Critical: Undefined variable error in conditional path.The variable
$child_entriesis only defined in two branches:
- Line 252: When
str_starts_with( $atts['embedded_field_id'], 'form' )is true- Line 258: When the above is false AND
$child_valuesis truthyIf both conditions are false,
$child_entriesremains undefined when referenced on line 264, causing an "Undefined variable" warning.The previous
empty($child_entries)check would handle undefined variables gracefully, but! $child_entriesdoes not.🐛 Proposed fix
// This is an embedded form. + $child_entries = false; if ( str_starts_with( $atts['embedded_field_id'], 'form' ) ) { // This is a repeating section. $child_entries = FrmEntry::getAll( array( 'it.parent_item_id' => $entry->id ), '', '', true ); } else { // Get all values for this field. $child_values = $entry->metas[ $atts['embedded_field_id'] ] ?? false; if ( $child_values ) { $child_entries = FrmEntry::getAll( array( 'it.id' => (array) $child_values ) ); } }Based on static analysis hints.
classes/models/FrmStyle.php (1)
232-246: Fix:$insert_valuescan be undefined now (real bug; Psalm is right).
empty($insert_values)is safe on an undefined variable;if ( $insert_values )is not. Initialize it (or revert this specific check back to! empty()).Proposed fix (initialize + keep new guard)
// add more 0s and 1 (if alpha position) if needed. $missing_values = $length_of_color_codes - count( $new_color_values ); + $insert_values = array(); if ( $missing_values > 1 ) { $insert_values = array_fill( 0, $missing_values - 1, 0 ); $last_value = 4 === $length_of_color_codes ? 1 : 0; array_push( $insert_values, $last_value ); } elseif ( $missing_values === 1 ) { $insert_values = 4 === $length_of_color_codes ? array( 1 ) : array( 0 ); } if ( $insert_values ) { $new_color_values = array_merge( $new_color_values, $insert_values ); }classes/controllers/FrmXMLController.php (1)
96-109: Address the undefined variable issue flagged by Psalm.Static analysis correctly identifies that
$pagesmay be undefined at line 106. If neither the imported posts condition (line 96) nor the form condition (line 101) is true,$pagesis never initialized before being checked.🐛 Proposed fix
Initialize
$pagesat the start to prevent undefined variable:if ( ! empty( $imported['imported']['posts'] ) ) { // Return the link to the last page created. $pages = $imported['posts']; + } else { + $pages = null; } if ( $form ) { // Create selected pages with the correct shortcodes. $pages = self::create_pages_for_import( $form ); } if ( $pages ) {Based on static analysis hints.
classes/helpers/FrmStylesHelper.php (1)
696-705: Consider explicit empty-string comparison for robustness.While style slugs are auto-generated with a minimum 2-character constraint, the GET parameter path at line 693 could theoretically pass
?style_name=0. Sinceif ( '0' )is falsy in PHP, you'd skip settingstyle_classunexpectedly. Use'' !== $style_namefor defensive consistency.Suggested fix
- if ( $style_name ) { + if ( '' !== $style_name ) { $settings['style_class'] = $style_name . '.'; }classes/controllers/FrmFormsController.php (1)
2024-2078: Fix:$messagecan be undefined (Psalm failure + runtime notice risk).
$messageis only set inside theswitch; if$bulkactiondoesn’t match any case,if ( $message )reads an undefined variable.Proposed fix
switch ( $bulkaction ) { case 'delete': $message = self::bulk_destroy( $ids ); break; case 'trash': $message = self::bulk_trash( $ids ); break; case 'untrash': $message = self::bulk_untrash( $ids ); + break; + default: + $message = ''; } if ( $message ) { $errors['message'] = $message; }
🤖 Fix all issues with AI agents
In
@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php:
- Around line 176-210: In isInIfCondition, the final check "if ($code !==
T_OPEN_PARENTHESIS)" is unreachable because an earlier branch already returns
when $code === T_OPEN_PARENTHESIS; replace that redundant conditional by simply
returning false whenever you encounter any non-whitespace and non-T_BOOLEAN_NOT
token that isn't an open parenthesis: i.e., after skipping T_WHITESPACE and
T_BOOLEAN_NOT, if $code is not T_OPEN_PARENTHESIS then return false directly
(remove the redundant comparison and unreachable branch).
🧹 Nitpick comments (8)
classes/models/FrmReviews.php (1)
110-114:empty()→ truthiness is fine here, but note semantics are slightly different in general.For
$user->first_namethis is effectively equivalent (including the'0'edge case), so the change looks safe in this context.classes/models/FrmFormMigrator.php (1)
209-226: Guard change is fine here; consider guarding beforemaybe_add_end_fields()to avoid warnings if overrides return non-arrays.
For a defined$source_fieldsvariable,if ( ! $source_fields )is effectively equivalent toempty( $source_fields )whenget_form_fields()returns an array (the apparent intent). The main fragility is thatmaybe_add_end_fields()runs before the guard andforeachassumes an iterable.Proposed defensive reorder (keeps current behavior for arrays)
protected function import_form( $source_id ) { $source_form = $this->get_form( $source_id ); $source_form_name = $this->get_form_name( $source_form ); $source_fields = $this->get_form_fields( $source_form ); - $this->maybe_add_end_fields( $source_fields ); $this->current_source_form = $source_form; // If form does not contain fields, bail. if ( ! $source_fields ) { wp_send_json_success( array( 'error' => true, 'name' => esc_html( $source_form_name ), 'msg' => __( 'No form fields found.', 'formidable' ), ) ); } + + $this->maybe_add_end_fields( $source_fields );To verify the contract quickly, grep for overrides of
get_form_fields()in migrators and confirm they always return arrays (notWP_Error,null,false, etc.).classes/models/FrmFieldFormHtml.php (1)
528-563:$extra_classestruthiness check is fine; consider trimming if whitespace-only values are possible.
if ( $extra_classes )(Line 548) should behave the same as! empty(...)for typical class strings; edge cases like'0'also stay effectively excluded. Ifclassescan sometimes be whitespace-only, you may wanttrim()to avoid appending meaningless tokens (optional).stripe/models/FrmStrpLiteAuth.php (1)
343-343: Consider consistent style for both conditions.Only the first condition was updated (
empty($actions)→!$actions), while the second still usesempty($intents). For consistency with the PR objective, consider updating both.♻️ Consistent refactor suggestion
- if ( ! $actions || empty( $intents ) ) { + if ( ! $actions || ! $intents ) {phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php (1)
88-89: ExpandwasVariableAssignedto detect additional assignment patterns for completeness.The
wasVariableAssignedmethod only checks for the=operator (line 240). This causes false negatives for variables assigned via:
- Compound assignments (
+=,.=, etc.) — found in source codeforeachloops — found in 12+ controller files- Function parameters
- Reference assignments
Consider extending the method to check for
T_PLUS_EQUAL,T_CONCAT_EQUALand other compound operators, as well as foreach loop variable declarations, to improve detection accuracy.classes/helpers/FrmEntriesListHelper.php (1)
202-214: Consider$s !== ''if you want"0"to count as a real search.
if ( $s )(and previously! empty( $s )) treats"0"as “no search”, so users searching for0won’t get “No Entries Found”. If that’s unintended, switch to a strict empty-string check.Proposed tweak
- if ( $s ) { + if ( '' !== $s ) { esc_html_e( 'No Entries Found', 'formidable' ); return; }classes/helpers/FrmXMLHelper.php (1)
2390-2399: Inconsistent empty check pattern for similar variables.Lines 2393 and 2397 both reference
$reply_to, but use different checking patterns:
- Line 2393:
if ( $reply_to )- uses truthiness- Line 2397:
if ( $reply_to || ! empty( $reply_to_name ) )- mixes truthiness and! empty()Since both
$reply_toand$reply_to_nameare initialized the same way (lines 2390-2391) and represent similar string data, they should use consistent checking patterns.♻️ Suggested fix for consistency
- 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 ) . '>'; }Note: Line 2398 still uses
empty()in ternary expressions, which is appropriate for providing fallback values.square/controllers/FrmSquareLiteActionsController.php (1)
111-117: Consider simplifying the “zero amount” check for readability.
$amountis a numeric string;! $amountalready catches'0', so$amount == 000reads oddly (and looks redundant).Proposed tweak
- // phpcs:ignore Universal.Operators.StrictComparisons - if ( ! $amount || $amount == 000 ) { + // phpcs:ignore Universal.Operators.StrictComparisons + if ( ! $amount ) { $response['error'] = __( 'Please specify an amount for the payment', 'formidable' ); return $response; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
phpcs.xmlis excluded by!**/*.xml
📒 Files selected for processing (52)
classes/controllers/FrmAddonsController.phpclasses/controllers/FrmAppController.phpclasses/controllers/FrmEmailStylesController.phpclasses/controllers/FrmEntriesController.phpclasses/controllers/FrmFieldsController.phpclasses/controllers/FrmFormActionsController.phpclasses/controllers/FrmFormTemplatesController.phpclasses/controllers/FrmFormsController.phpclasses/controllers/FrmInboxController.phpclasses/controllers/FrmStylesController.phpclasses/controllers/FrmXMLController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmEntriesHelper.phpclasses/helpers/FrmEntriesListHelper.phpclasses/helpers/FrmFormsHelper.phpclasses/helpers/FrmListHelper.phpclasses/helpers/FrmOnSubmitHelper.phpclasses/helpers/FrmStylesHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmAddon.phpclasses/models/FrmCreateFile.phpclasses/models/FrmDb.phpclasses/models/FrmEntryMeta.phpclasses/models/FrmEntryValidate.phpclasses/models/FrmField.phpclasses/models/FrmFieldFormHtml.phpclasses/models/FrmForm.phpclasses/models/FrmFormAction.phpclasses/models/FrmFormApi.phpclasses/models/FrmFormMigrator.phpclasses/models/FrmMigrate.phpclasses/models/FrmPluginSearch.phpclasses/models/FrmReviews.phpclasses/models/FrmSolution.phpclasses/models/FrmSpamCheckWPDisallowedWords.phpclasses/models/FrmStyle.phpclasses/models/fields/FrmFieldDefault.phpclasses/models/fields/FrmFieldType.phpclasses/models/fields/FrmFieldUrl.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.phpsquare/controllers/FrmSquareLiteActionsController.phpsquare/controllers/FrmSquareLiteAppController.phpstripe/controllers/FrmStrpLiteActionsController.phpstripe/controllers/FrmStrpLiteEventsController.phpstripe/controllers/FrmTransLiteActionsController.phpstripe/helpers/FrmStrpLiteConnectApiAdapter.phpstripe/helpers/FrmStrpLiteLinkRedirectHelper.phpstripe/models/FrmStrpLiteAuth.phpstripe/models/FrmTransLiteDb.phptests/phpunit/applications/test_FrmApplicationsController.phptests/phpunit/base/FrmUnitTest.phptests/phpunit/fields/test_FrmFieldValidate.php
🧰 Additional context used
🧬 Code graph analysis (11)
square/controllers/FrmSquareLiteActionsController.php (1)
classes/models/FrmUsage.php (1)
actions(468-488)
classes/models/FrmAddon.php (1)
classes/models/FrmInstallPlugin.php (1)
is_active(52-54)
classes/controllers/FrmStylesController.php (1)
classes/models/FrmUsage.php (1)
settings(211-252)
classes/models/fields/FrmFieldType.php (1)
classes/models/FrmField.php (2)
is_radio(1569-1571)is_checkbox(1582-1584)
stripe/models/FrmStrpLiteAuth.php (1)
classes/models/FrmUsage.php (1)
actions(468-488)
stripe/controllers/FrmStrpLiteActionsController.php (1)
classes/models/FrmUsage.php (1)
actions(468-488)
classes/models/FrmFieldFormHtml.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5061)should_include_alert_role_on_field_errors(4020-4022)
classes/controllers/FrmAddonsController.php (1)
classes/models/FrmFormApi.php (3)
FrmFormApi(6-443)get_api_info(92-160)get_addon_for_license(255-274)
classes/models/FrmSolution.php (1)
classes/models/FrmUsage.php (1)
plugins(187-202)
classes/models/FrmFormApi.php (1)
classes/models/FrmSolution.php (1)
download_id(787-789)
classes/helpers/FrmAppHelper.php (2)
stubs.php (1)
post_type(252-253)classes/helpers/FrmListHelper.php (1)
get_param(195-204)
🪛 GitHub Actions: Psalm Code Analysis
classes/controllers/FrmFormsController.php
[error] 2064-2073: Psalm: PossiblyUndefinedVariable: Possibly undefined variable $message, first seen on line 2064 (see https://psalm.dev/018)
🪛 GitHub Check: Psalm
classes/helpers/FrmEntriesHelper.php
[failure] 264-264: PossiblyUndefinedVariable
classes/helpers/FrmEntriesHelper.php:264:10: PossiblyUndefinedVariable: Possibly undefined variable $child_entries, first seen on line 252 (see https://psalm.dev/018)
stripe/helpers/FrmStrpLiteLinkRedirectHelper.php
[failure] 60-60: PossiblyUndefinedVariable
stripe/helpers/FrmStrpLiteLinkRedirectHelper.php:60:10: PossiblyUndefinedVariable: Possibly undefined variable $referer, first seen on line 57 (see https://psalm.dev/018)
classes/models/FrmStyle.php
[failure] 243-243: PossiblyUndefinedVariable
classes/models/FrmStyle.php:243:8: PossiblyUndefinedVariable: Possibly undefined variable $insert_values, first seen on line 236 (see https://psalm.dev/018)
classes/controllers/FrmFormsController.php
[failure] 2073-2073: PossiblyUndefinedVariable
classes/controllers/FrmFormsController.php:2073:8: PossiblyUndefinedVariable: Possibly undefined variable $message, first seen on line 2064 (see https://psalm.dev/018)
classes/controllers/FrmXMLController.php
[failure] 106-106: PossiblyUndefinedVariable
classes/controllers/FrmXMLController.php:106:9: PossiblyUndefinedVariable: Possibly undefined variable $pages, first seen on line 98 (see https://psalm.dev/018)
⏰ 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). (6)
- 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: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (88)
stripe/controllers/FrmStrpLiteEventsController.php (1)
174-174: Replaceempty()with truthiness check is correct here.The change from
empty( $customer_id )to! $customer_idis functionally equivalent for this use case. Since$customer_idoriginates from a Stripe invoice ID (line 172), it will be either a non-empty string identifier or null/falsy—the string"0"is not a realistic customer ID. The truthiness check is idiomatic and aligns with modern PHP practices.classes/models/FrmPluginSearch.php (1)
209-214: Truthiness check is behaviorally equivalent here (givenabsint).Since
FrmAppHelper::get_param(..., 'absint')returns an int>= 0, switching from! empty( $addon )toif ( $addon )preserves behavior. If you want to make the intent extra explicit/robust against future sanitization changes, considerif ( 0 < $addon ).stripe/models/FrmTransLiteDb.php (1)
338-338: LGTM!The change from
empty($result)to!$resultmaintains functional equivalence. Since$wpdb->get_results()returns either an array (possibly empty) or NULL on error, both expressions evaluate identically in this context. The migration logic remains unchanged.classes/models/FrmSpamCheckWPDisallowedWords.php (1)
19-19: LGTM: Functionally equivalent refactoring.The change from
empty($mod_keys)to! $mod_keysmaintains identical behavior. Both conditions treat all falsy values the same way (empty string"",false, and notably the string"0"are all falsy in both cases), so the spam check logic is preserved.Note: The AI summary incorrectly states that
'0'is "now treated as truthy"—both the old and new code treat'0'as falsy and abort the check.classes/models/FrmMigrate.php (1)
547-547: LGTM! Truthiness checks are semantically equivalent here.The refactoring from
empty()to!operator is safe for these guard clauses. Both approaches are falsy for the same values (null, false, 0, '0', '', []) in these contexts:
$update_values: Return value from helper method$widgets: Result fromget_option()(returns false or array)$styles: Result fromget_all()(returns array or false)$exists: Result from$wpdb->get_row()(returns object or null)The migration code context makes edge cases with '0' as a valid value highly unlikely, and all variables are defined before being checked.
Also applies to: 599-599, 644-644, 700-700, 726-726, 763-763
stripe/controllers/FrmTransLiteActionsController.php (3)
162-164: LGTM! Safe truthiness check for error messages.The change from
empty($message)to! $messageis appropriate here. Error messages are extremely unlikely to be the string'0', and both checks behave identically for empty strings, null, and false values.
505-507: LGTM! Behavior is equivalent for object/false checks.The change from
empty($previous_entry)to! $previous_entrymaintains identical behavior. Both checks handle the defaultfalsevalue (when the key doesn't exist) and object values equivalently.
288-292: No instances of '0' used as a description value were found in the codebase. This edge case is theoretical and does not occur in actual usage, as payment descriptions are user-visible metadata typically containing meaningful text rather than the string '0'. The condition change from! empty($description)to$descriptionhas no practical impact on current code.classes/models/fields/FrmFieldDefault.php (1)
44-44: The change fromempty($input_html)to! $input_htmlis correct. While there is a theoretical edge case where the string "0" would be handled differently—the new behavior would output it, whereas the old behavior would callbuilder_text_field()—this scenario is not a concern. The action hooksfrm_display_added_fieldsandfrm_display_added_{type}_fieldare public hooks for external code and are not implemented within the codebase. If an action intentionally outputs content (including the string "0"), that output should be displayed, making the new behavior semantically correct.stripe/controllers/FrmStrpLiteActionsController.php (2)
33-33: LGTM: Functionally equivalent refactor.The change from
empty($actions)to! $actionsmaintains identical behavior since empty arrays are falsy in PHP. The check correctly determines whether to return the callback or show the Stripe card field.
126-126: LGTM: Functionally equivalent refactor.The change from
empty($amount)to! $amountmaintains identical behavior. Both expressions treat the string '0' and other falsy values consistently. The secondary check$amount == 000provides redundant coverage but pre-existed this change.classes/models/FrmSolution.php (2)
685-685: LGTM! Consistent with the change at line 411.The change from
empty( $pages )to! $pagesis correct. Sinceneeded_pages()returns an array, the truthiness check is functionally equivalent toempty()for determining if the array is empty.
411-411: LGTM! Truthiness check is functionally equivalent for arrays.The change from
empty( $plugins )to! $pluginsis correct. Bothrequired_plugins()andneeded_pages()return arrays (empty arrays in the base implementations), and the truthiness checks work correctly for arrays—an empty array is falsy in PHP, so both expressions evaluate identically.stripe/models/FrmStrpLiteAuth.php (2)
227-227: LGTM: Safe refactor for array check.The change from
!empty($intents)to$intentsis semantically equivalent since$intentsis always an array (returned fromget_payment_intents()which guarantees an array return type).
305-305: LGTM: Safe refactor for array check.The change from
empty($intents)to!$intentsis semantically equivalent. The variable is always defined via the null coalescing operator on line 303, defaulting to an empty array.stripe/helpers/FrmStrpLiteConnectApiAdapter.php (1)
116-116: LGTM – Refactoring maintains equivalent behavior.The change from
! empty($customer_id_error_message)to$customer_id_error_messageis semantically equivalent for all possible values. The variable is initialized as an empty string (line 77) and only reassigned to a non-empty error message string (line 106). At line 116, both conditions evaluate identically for all reachable code paths.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php (6)
1-36: LGTM! Clean setup and registration.The namespace, imports, and token registration are correctly structured.
46-78: LGTM! Proper validation of empty() structure.The validation logic correctly:
- Ensures the context is an if/elseif condition
- Verifies simple variable usage (no array access or properties)
- Uses early returns for unsupported cases
112-165: LGTM! Thorough auto-fix implementation.The fixer correctly handles both cases:
!empty($var)→$var(removes negation)empty($var)→! $var(adds negation)The whitespace cleanup is comprehensive and ensures proper formatting.
220-234: LGTM! Correct scope detection.The
findContainingFunctionmethod correctly:
- Searches for containing function or closure
- Validates scope boundaries to ensure the token is actually within the function
- Handles nested functions properly
246-269: Assignment detection is functional for common cases.The
wasVariableAssignedmethod correctly detects simple assignments with the=operator. While it doesn't cover all assignment patterns (as noted in the earlier comment), it should work for the most common case illustrated in the PR.
80-110: The transformation fromempty($var)to!$varis semantically equivalent for assigned variables. In PHP, the string'0'is a falsy value, so bothempty('0')and!'0'returntrue. This is consistent for all falsy values (0,0.0,'','0',false, empty arrays, andnull). No action required on this concern.classes/models/FrmFormApi.php (3)
53-53: LGTM: Truthiness check is equivalent.The refactoring from
empty( $edd_update )to a direct truthiness check is functionally equivalent, asget_pro_updater()returns eitherfalseor an object.
300-300: LGTM: Truthiness check is equivalent for this context.The refactoring from
empty( $cache )to! $cacheis functionally equivalent. Theget_cached_option()method returns either an array orfalsefrom WordPress option functions, and both checks handle these return types identically.
263-263: Verify edge case: string values for download_id.The change from
empty( $download_id )to! $download_idtreats string values differently:
empty('0')returnstruebut!'0'returnsfalseThe type hint allows
download_idto beint|string|null, and the codebase includes defensive checks for non-numeric strings (FrmAddon.php line 993). However,download_idis populated from JSON API responses (which decode numeric keys as integers), and no evidence shows the string'0'being assigned in practice. Verify whether non-numeric strings or string'0'are realistic concerns for this property.classes/models/FrmDb.php (1)
844-844: LGTM: Safe truthiness check for array.The change from
! empty( $cached_keys )to$cached_keysis safe here. Theget_group_cached_keys()method always returns an array (line 813), so the truthiness check behaves identically to theempty()check for this use case.tests/phpunit/base/FrmUnitTest.php (1)
335-335: LGTM: Safe truthiness check in test.The change from
empty( $users )to! $usersis safe here. Theget_users()function returns an array, and for arrays, an empty array is falsy while a non-empty array is truthy—identical behavior to theempty()check.classes/models/FrmEntryValidate.php (1)
433-443: LGTM: Truthiness check appropriate for error arrays.The change from
! empty( $new_errors )to$new_errorsis appropriate here. Field validation methods typically return arrays of errors or falsy values (null/false/empty array). For arrays, the truthiness check behaves identically toempty().Note: If any
validate()method were to return the string'0'as an error, the behavior would differ (previously treated as empty, now as truthy). However, this is an unlikely edge case for error handling.classes/models/FrmCreateFile.php (1)
177-177: LGTM!The change from
empty($creds)to! $credsis appropriate here. Given thatget_creds()returns either an array orfalse(per the docblock at line 223), both versions behave identically for the expected data types.classes/models/FrmForm.php (1)
280-280: LGTM!The change from
! empty($new_values)to$new_valuesis appropriate. Since$new_valuesis built as an array (starting at line 268), the truthiness check behaves identically toempty()for the expected data type.classes/models/FrmEntryMeta.php (1)
173-173: LGTM!The change from
empty($previous_field_ids)to! $previous_field_idsis safe. Since$previous_field_idscomes fromFrmDb::get_col()(line 130), which returns an array, the truthiness check behaves identically toempty()for the expected data type.classes/helpers/FrmOnSubmitHelper.php (1)
306-306: No issue found with the return type change.The function
FrmFormAction::form_has_action_type()is documented and implemented to return a boolean (line 716:@return bool). Its implementation isreturn ! empty( $actions );, which always produces a boolean value. Since$has_actionsalready contains a boolean, the change from! empty($has_actions)to$has_actionsis safe and introduces no behavioral difference. The concern about a numeric return value or string'0'does not apply here.Likely an incorrect or invalid review comment.
tests/phpunit/applications/test_FrmApplicationsController.php (1)
27-27: This concern does not apply—the type is already asserted.Line 25 contains
$this->assertIsArray( $data )before the condition at line 27, which guarantees$datais always an array at that point. The falsy string'0'edge case only matters for scalar values, not arrays. For arrays,empty([])and! []have identical behavior: both evaluate totrue. The change fromempty($data)to! $datais functionally equivalent for the array type being checked.Likely an incorrect or invalid review comment.
classes/controllers/FrmAppController.php (2)
65-73: Verify edge case handling for page parameter.The change from
empty($page)to!$pageis functionally equivalent for typical values (empty strings, null). However, if$pagecould ever be the string'0', both checks would treat it as falsy/empty, maintaining consistent behavior. Consider whether page names could legitimately be'0'in your routing logic.
75-77: LGTM - consistent truthiness check.The change to use
if ( $page )is consistent with the earlier check and maintains the same behavior for all expected input values including edge cases.tests/phpunit/fields/test_FrmFieldValidate.php (1)
50-57: LGTM - truthiness check appropriate for array validation.The change from
!empty($errors)toif ( $errors )is functionally equivalent for array values. Empty arrays are falsy in PHP, soif ( [] )evaluates to false, matching the behavior ofempty([]). Since$errorsis consistently an array fromFrmEntryValidate::validate(), this change is safe.Also applies to: 142-148, 160-167
classes/controllers/FrmEntriesController.php (2)
253-255: LGTM - safe for array/false return value.The change to
if ( ! $sub_form_cols )is safe.FrmField::get_all_for_form()returns either an array or false, and both empty arrays and false are falsy, making this functionally equivalent to the previousempty()check.
529-532: LGTM - consistent with array handling.The truthiness check
if ( $hidden )correctly handles the array return value fromuser_hidden_columns_for_form(). Empty arrays are falsy, so behavior is preserved.classes/models/fields/FrmFieldUrl.php (1)
83-84: LGTM - appropriate for URL validation.The change to
if ( $value && !preg_match(...) )maintains correct validation logic. For URL strings:
- Empty strings: both
empty('')and!''evaluate to true (skip validation) ✓- Valid URLs: both proceed to validation ✓
- Edge case
'0': both skip validation (though'0'is not a valid URL anyway)The truthiness check is semantically clearer for string validation.
classes/helpers/FrmFormsHelper.php (1)
572-581: No verification required—filter always returns an array.The
frm_submit_button_classfilter has a default value ofarray()(line 570). All registered implementations—FrmFormsController::update_button_classesand the filter inFrmStylesController.php—receive an array, append to it, and return it as an array. The change from!empty($classes)toif ($classes)is safe because both expressions evaluate identically for arrays: empty arrays are falsy in both cases, and non-empty arrays are truthy in both cases.classes/models/FrmFormAction.php (3)
601-607: Guard change looks safe for expected return types (arrays/false/null).
FrmFormActionsController::get_form_actions()should return an array/object or falsy; switchingempty()to truthiness here is fine assuming it never returns an unexpected scalar like0.
752-760:$actionsearly-return: OK ifFrmDb::check_cache(..., 'get_posts')returns an array.
[]vs non-empty array behaves the same as before withempty().
932-945:get_posts(... numberposts => 1)result check: OK.
get_postsreturns an array; using! $post_idis equivalent toempty($post_id)for[]vs[WP_Post].classes/helpers/FrmListHelper.php (3)
321-337:views()truthiness check is fine if$viewsis always an array.
If any filter returns a non-array falsy value (e.g.,''), the early return still makes sense.
612-706:$infinite_scrollcheck: OK, but keep types predictable.
Switching from! empty( $infinite_scroll )to$infinite_scrollis fine if_pagination_args['infinite_scroll']is stored as a boolean (or at least consistently truthy/falsy).
847-872: Primary column fallback: reasonable simplification.
The new condition reads clearer and still guards against missing/invalid column keys.classes/helpers/FrmAppHelper.php (4)
101-110: Affiliate guard change looks safe (sinceget_affiliate()returns an int).
absint(...)guarantees0+, so truthiness matches the old! empty()intent.
438-446:is_formidable_admin()guard: OK.
Treating missing/emptypageas falsy matches the previous behavior.
520-536:is_view_builder_page()fallback: OK.
post_typebeing falsy triggers the post lookup, same as the earlierempty()check.
1316-1332:remove_get_action()truthiness checks: OK given expected strings.
$action_nameis''|'action'|'action2'and$new_actionis typically a non-empty string; this should behave as before.classes/models/fields/FrmFieldType.php (5)
269-278:show_on_form_builder()include guard: OK.
Assuminginclude_form_builder_file()returns''or a real path, the truthiness check is equivalent.
1069-1082:get_container_class()align guard: OK for expected align values.
Ifaligncan ever be'0', it would be ignored (same asempty()would’ve done).
1113-1128:add_input_class()extra class guard: OK.
1161-1173: Front include guard: OK.
include_front_form_file()returning''/path makes this equivalent to the prior! empty()check.
1188-1216: Early-return on missing include file: OK.classes/models/FrmStyle.php (2)
515-546:get_all()temp style guards: OK for arrays fromget_posts.
Swappingempty()to truthiness should behave the same for[]vs non-empty arrays.
853-861: Default template fallback: OK.
If the meta is missing/empty, returning encoded defaults is consistent.classes/controllers/FrmFormActionsController.php (1)
331-331: LGTM - Truthiness checks are appropriate here.The refactoring from
empty()to direct boolean checks is safe in this file because:
- All checked variables are objects or arrays (not strings where '0' would matter)
- Variables are assigned before checks (no undefined variable warnings)
- The semantic behavior for typical cases remains unchanged
Also applies to: 489-489, 591-591, 661-661
classes/models/FrmAddon.php (1)
166-166: LGTM - Refactoring maintains consistent behavior.The truthiness checks are appropriate for these variables:
$license(string): The edge case of '0' as a license key would be treated as falsy by bothempty()and direct boolean check, maintaining consistent behavior$timeout(integer): For timestamp comparisons, a value of 0 (Unix epoch) would correctly fail the time check anyway$errors(array): Standard array truthiness checkAlso applies to: 233-233, 286-286, 488-488, 608-608
classes/controllers/FrmXMLController.php (1)
101-101: LGTM - Truthiness checks are safe for these variables.The refactoring is appropriate as all variables are arrays, objects, or error values where the semantic difference between
empty()and truthiness doesn't introduce edge case issues.Also applies to: 430-430, 659-659, 682-682, 733-733, 756-756
classes/models/FrmField.php (1)
906-906: LGTM - Refactoring is safe for these array and string checks.The truthiness checks correctly handle:
- Array variables (
$results,$sub_fields) where empty arrays are the only edge case- String field type check where the additional
!== $field_typecomparison ensures only meaningful differences are processedAlso applies to: 963-963, 1055-1055, 1340-1340
classes/helpers/FrmXMLHelper.php (1)
222-222: LGTM - Truthiness refactoring is safe for these checks.The changes appropriately handle:
- Term parent IDs (strings/integers)
- Form and array objects
- Error details arrays
- Post query results
These refactorings maintain consistent behavior for typical use cases.
Also applies to: 253-253, 1450-1450, 1555-1555, 1678-1678, 2135-2135
classes/controllers/FrmInboxController.php (1)
56-78: Guard change is safe here (variable is always defined).
$keyis always initialized (default''), so switching from! empty( $key )to$keywon’t introduce undefined-variable notices; behavior is also effectively the same for string keys (including'0').classes/controllers/FrmFormTemplatesController.php (1)
404-424: Validation tweak is fine and keepsGiven the default
''fromget_post_param,is_email().classes/controllers/FrmEmailStylesController.php (1)
301-316: LGTM: safe short-circuiting with a defined variable.Because
square/controllers/FrmSquareLiteAppController.php (1)
81-86: Verify the return type contract ofget_actions_before_submit()before finalizing this change.The change from
empty( $actions )to! $actionsis safe only if the method consistently returns an array (orfalse/[]). If the method could return other falsy values like0,'',null, orWP_Errorobjects, the two conditionals behave differently and may cause unintended behavior.Please confirm the method's return type contract (via type hints, docblock, or implementation) to ensure no non-array sentinel values are possible.
classes/controllers/FrmFieldsController.php (3)
15-25: Guard change looks safe for$fields(array expected).
$fieldsdefaults toarray()and is iterated as an array;if ( ! $fields )preserves the “empty array => die” behavior.
738-744:$placeholdertruthiness check matches priorempty()semantics (including'0').
If'0'is ever a valid placeholder value, it was already treated as empty before; this change keeps that behavior.
985-1017: Shortcode merge condition change is effectively equivalent here.
elseif ( $k && isset( $add_html[ $k ] ) )behaves like the previous! empty( $k )check for typical keys (and still treats0/'0'as “empty”).square/controllers/FrmSquareLiteActionsController.php (2)
22-27: Action presence check is fine (arrayexpected).
if ( ! $actions )is equivalent toempty( $actions )for the array returned byget_actions_before_submit.
62-66: Comment fix (Square vs Stripe) is correct.classes/controllers/FrmStylesController.php (2)
846-862: Codemirror settings guard is appropriate (array|false).
Switching from! empty( $settings )toif ( $settings )keeps the same behavior for thewp_enqueue_code_editorreturn type.
1227-1237: Style defaulting logic remains consistent (including'0'/1).
if ( ! $style || 1 == $style )preserves the old fallback-to-default behavior.classes/controllers/FrmAddonsController.php (8)
109-115: CTA fallback remains equivalent to previous empty-check behavior.
If the sale API ever returns'0', it will still fall back (same asempty()did).
435-460: Credential guard is good (defensive against non-array options).
if ( ! $creds || ! is_array( $creds ) || ! isset( $creds['license'] ) )is a solid early-return for unexpected option shapes.
492-513: Expiry guard is fine and keeps prior semantics.
! $expiresbehaves likeempty( $expires )for expected timestamp-ish values.
521-533: Installed add-ons guard change looks safe.
! $installed_addonsis equivalent toempty()for the filtered array.
542-556: Update check early-return remains safe.
The transient is returned unchanged when no add-ons are present.
634-683: License de-dupe / API merge logic remains coherent after truthiness tweaks.
No behavior change expected for normal license strings and API arrays.
774-789: License-to-addon match condition stays consistent.
The! $download_idguard still treats0/'0'as “missing” (as before).
1303-1324: Connect token init remains correct.
Regenerates the token when missing/falsy, then builds the connect URL with it.classes/controllers/FrmFormsController.php (7)
1129-1137: Shortcode guard is fine.
if ( ! $shortcode )is an equivalent early-exit for missing/empty input.
2036-2041: Bulk action prefix stripping change is OK.
Guarding with$bulkaction && str_starts_with(...)behaves like the prior! empty( $bulkaction )check for expected action strings.
2042-2049: IDs-required guard remains correct.
if ( ! $ids )preserves the old “no selection => error” behavior for both empty string and empty array.
2095-2123: JSON decode failure guard is consistent with previous behavior.
Note: this still treats a successfully-decoded empty array ([]) as failure; if that payload is impossible/invalid for this route, that’s fine.
2550-2593: Post-submit branch uses$errorstruthiness appropriately.
Arrays behave the same as the prior! empty( $errors )check.
2794-2856: Fallback action selection remains correct.
if ( ! $met_actions )keeps the same behavior for empty arrays.
3441-3450: Script selection guard should be fine (assuming suffix is ''/'.min').
$suffix && self::has_combo_js_file()matches the old! empty( $suffix )check for expected suffix values.
| private function isInIfCondition( File $phpcsFile, $stackPtr ) { | ||
| $tokens = $phpcsFile->getTokens(); | ||
|
|
||
| // Find the opening parenthesis that contains this empty() call. | ||
| for ( $i = $stackPtr - 1; $i >= 0; $i-- ) { | ||
| $code = $tokens[ $i ]['code']; | ||
|
|
||
| // Skip whitespace and the "!" operator. | ||
| if ( $code === T_WHITESPACE || $code === T_BOOLEAN_NOT ) { | ||
| continue; | ||
| } | ||
|
|
||
| // If we hit an open parenthesis, check if it belongs to if/elseif. | ||
| if ( $code === T_OPEN_PARENTHESIS ) { | ||
| $beforeParen = $phpcsFile->findPrevious( T_WHITESPACE, $i - 1, null, true ); | ||
|
|
||
| if ( false !== $beforeParen ) { | ||
| $beforeCode = $tokens[ $beforeParen ]['code']; | ||
|
|
||
| if ( $beforeCode === T_IF || $beforeCode === T_ELSEIF ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // If we hit something else, stop. | ||
| if ( $code !== T_OPEN_PARENTHESIS ) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix unreachable logic error.
Line 204 contains a logic error. At line 189, if $code === T_OPEN_PARENTHESIS is true, the method returns before reaching line 204. Therefore, when execution reaches line 204, $code is guaranteed to NOT be T_OPEN_PARENTHESIS, making the condition if ( $code !== T_OPEN_PARENTHESIS ) always true and redundant.
🐛 Proposed fix to remove redundant condition
// If we hit something else, stop.
- if ( $code !== T_OPEN_PARENTHESIS ) {
- return false;
- }
+ return false;
}
return false;The logic should simply return false if we encounter any non-whitespace, non-negation token that isn't T_OPEN_PARENTHESIS.
📝 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.
| private function isInIfCondition( File $phpcsFile, $stackPtr ) { | |
| $tokens = $phpcsFile->getTokens(); | |
| // Find the opening parenthesis that contains this empty() call. | |
| for ( $i = $stackPtr - 1; $i >= 0; $i-- ) { | |
| $code = $tokens[ $i ]['code']; | |
| // Skip whitespace and the "!" operator. | |
| if ( $code === T_WHITESPACE || $code === T_BOOLEAN_NOT ) { | |
| continue; | |
| } | |
| // If we hit an open parenthesis, check if it belongs to if/elseif. | |
| if ( $code === T_OPEN_PARENTHESIS ) { | |
| $beforeParen = $phpcsFile->findPrevious( T_WHITESPACE, $i - 1, null, true ); | |
| if ( false !== $beforeParen ) { | |
| $beforeCode = $tokens[ $beforeParen ]['code']; | |
| if ( $beforeCode === T_IF || $beforeCode === T_ELSEIF ) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| // If we hit something else, stop. | |
| if ( $code !== T_OPEN_PARENTHESIS ) { | |
| return false; | |
| } | |
| } | |
| return false; | |
| } | |
| private function isInIfCondition( File $phpcsFile, $stackPtr ) { | |
| $tokens = $phpcsFile->getTokens(); | |
| // Find the opening parenthesis that contains this empty() call. | |
| for ( $i = $stackPtr - 1; $i >= 0; $i-- ) { | |
| $code = $tokens[ $i ]['code']; | |
| // Skip whitespace and the "!" operator. | |
| if ( $code === T_WHITESPACE || $code === T_BOOLEAN_NOT ) { | |
| continue; | |
| } | |
| // If we hit an open parenthesis, check if it belongs to if/elseif. | |
| if ( $code === T_OPEN_PARENTHESIS ) { | |
| $beforeParen = $phpcsFile->findPrevious( T_WHITESPACE, $i - 1, null, true ); | |
| if ( false !== $beforeParen ) { | |
| $beforeCode = $tokens[ $beforeParen ]['code']; | |
| if ( $beforeCode === T_IF || $beforeCode === T_ELSEIF ) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| // If we hit something else, stop. | |
| return false; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In
@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php
around lines 176 - 210, In isInIfCondition, the final check "if ($code !==
T_OPEN_PARENTHESIS)" is unreachable because an earlier branch already returns
when $code === T_OPEN_PARENTHESIS; replace that redundant conditional by simply
returning false whenever you encounter any non-whitespace and non-T_BOOLEAN_NOT
token that isn't an open parenthesis: i.e., after skipping T_WHITESPACE and
T_BOOLEAN_NOT, if $code is not T_OPEN_PARENTHESIS then return false directly
(remove the redundant comparison and unreachable branch).
1 participant
No description provided.