Update redundant empty sniff to also catch conditions with && and ||#2758
Conversation
WalkthroughThis PR systematically refactors conditional logic across the codebase by replacing PHP's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
classes/helpers/FrmXMLHelper.php (1)
2363-2400: Harden$email_fieldparsing: avoidstr_contains()/array edge cases.
$email_fieldcan become anint(default0) or anarray(afterexplode()), which risks a PHP 8+TypeError(str_contains) or storing an unexpected type intoemail_to. Since this block is being refactored, it’s a good spot to normalize$email_fieldto a string.Proposed fix
- $email_field = $form_options['ar_email_to'] ?? 0; + $email_field = (string) ( $form_options['ar_email_to'] ?? '' ); - if ( str_contains( $email_field, '|' ) ) { + if ( $email_field && str_contains( $email_field, '|' ) ) { // data from entries field $email_field = explode( '|', $email_field ); if ( isset( $email_field[1] ) ) { $email_field = $email_field[1]; + } else { + $email_field = ''; } } - if ( is_numeric( $email_field ) && $email_field ) { + if ( is_numeric( $email_field ) && $email_field ) { $email_field = '[' . $email_field . ']'; }phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php (1)
403-420: Remove unusedfindContainingIfmethod.This method is not called anywhere in the codebase after the refactoring introduced
getStatementContextandisInIfCondition. It can be safely removed.
🧹 Nitpick comments (5)
classes/models/FrmSettings.php (1)
353-353: Simplify the redundant condition.The condition
$mu_menu && $mu_menuchecks the same value twice. This should be simplified to just$mu_menu.♻️ Proposed simplification
- if ( $mu_menu && $mu_menu ) { + if ( $mu_menu ) {classes/controllers/FrmDashboardController.php (1)
362-366: Consider guardingin_arraywithis_array(...)for robustness.If the stored option is ever corrupted/non-array,
in_array(..., $banner_closed_by_users, true)can throw in PHP 8+. Not introduced by this change, but easy to harden while touching this line.Proposed hardening
- return $banner_closed_by_users && in_array( $user_id, $banner_closed_by_users, true ); + return is_array( $banner_closed_by_users ) && $banner_closed_by_users && in_array( $user_id, $banner_closed_by_users, true );classes/controllers/FrmAddonsController.php (1)
1335-1349: Type-hardenhash_equalsinputs to avoid PHP 8+ TypeError on unexpected option values.If
frm_connect_tokenis ever stored as a non-string (corrupted option),hash_equalscan fatally error. Easy to guard while touching this line.Proposed hardening
// Verify auth. $auth = get_option( 'frm_connect_token' ); - return $auth && hash_equals( $auth, $post_auth ); + return is_string( $auth ) && '' !== $auth && hash_equals( $auth, $post_auth );phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php (2)
189-189: Remove unused variable$tokens.This variable is declared but never used within the method, as the work is delegated to helper methods that fetch tokens themselves.
🧹 Suggested fix
private function getStatementContext( File $phpcsFile, $stackPtr ) { - $tokens = $phpcsFile->getTokens(); - // Check if in an if/elseif condition. $ifContext = $this->isInIfCondition( $phpcsFile, $stackPtr );
237-240: Redundant condition check.At this point in the loop,
$codecannot beT_OPEN_PARENTHESISsince that case returns on line 230 or 234. The condition is always true when reached.🧹 Suggested fix
- // If we hit something else, stop. - if ( $code !== T_OPEN_PARENTHESIS ) { - return false; - } + // If we hit something else (not whitespace, !, or open paren), stop. + return false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
classes/controllers/FrmAddonsController.phpclasses/controllers/FrmDashboardController.phpclasses/controllers/FrmEntriesController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmPluginSearch.phpclasses/models/FrmSettings.phpclasses/models/FrmUsage.phpclasses/models/fields/FrmFieldUrl.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.phptests/phpunit/misc/test_FrmCSVExportHelper.php
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmEntriesController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5061)is_admin(587-596)
classes/controllers/FrmAddonsController.php (2)
classes/models/FrmInstallPlugin.php (2)
activate_url(66-68)is_installed(45-47)classes/models/FrmPluginSearch.php (1)
is_installed(361-364)
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php
189-189: Avoid unused local variables such as '$tokens'. (undefined)
(UnusedLocalVariable)
⏰ 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). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (12)
classes/controllers/FrmEntriesController.php (1)
753-753: LGTM! Guard clause refactored correctly.The change from
empty($form_id)to! $form_idmaintains equivalent behavior since$form_idis cast to an absolute integer viaabsint. Both expressions return true for 0 and false for positive integers.tests/phpunit/misc/test_FrmCSVExportHelper.php (1)
168-168: LGTM! Test assertion simplified correctly.The truthiness check
$csv_pathis equivalent to! empty($csv_path)for string validation. The compound assertion withis_string()andfile_exists()provides adequate validation.classes/models/FrmUsage.php (1)
57-57: LGTM! UUID regeneration condition simplified correctly.The change from
empty( $uuid )to! $uuidis semantically equivalent for this use case. Both return true whenget_option()returns false (option not set) or an empty string, triggering UUID regeneration as intended.classes/models/FrmPluginSearch.php (1)
226-226: LGTM! Dismissed hints check simplified correctly.The change from
! empty( $dismissed_hints )to$dismissed_hintsmaintains equivalent behavior. Both expressions correctly handle the case whereget_option()returns false (option not set) or an empty array, returning an empty array as the fallback.classes/helpers/FrmXMLHelper.php (1)
2390-2400: Truthiness update forfromgeneration looks OK.classes/helpers/FrmAppHelper.php (2)
3209-3219:add_time_to_date: truthiness check is fine and keeps whitespace-only formats from rendering.
3575-3584:select_current_page: simplified condition reads well; behavior should be equivalent.classes/models/fields/FrmFieldUrl.php (1)
68-87:FrmFieldUrl::validate: change looks OK, but please confirm'0'should still be treated as blank.Using
! $valuekeeps PHP’s special-casing where'0'is falsy; that matches priorempty()behavior, but it’s worth confirming this is the desired input semantics for URL fields.classes/controllers/FrmAddonsController.php (1)
833-842:prepare_addons: truthy$activate_urlcheck is fine.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnAssignedVariableSniff.php (3)
254-287: LGTM!The boolean expression detection logic correctly handles both positions (before and after
empty()) and accounts for the negation operator.
297-320: LGTM!Clean implementation for finding statement boundaries with appropriate stopping conditions at function/class scopes.
359-393: LGTM!The refactored method correctly uses the statement token for scope level comparison, enabling the sniff to work with the broader set of boolean expression contexts.
Crabcyborg
deleted the
update_redundant_empty_sniff_to_catch_more_cases
branch
…_to_catch_more_cases Update redundant empty sniff to also catch conditions with && and ||
1 participant
No description provided.