New sniff that changes returns to ternaries for simple logic#2762
Conversation
WalkthroughRefactors numerous files to replace multi-line if/return patterns with concise ternary or conditional returns, adds a new PHP_CodeSniffer sniff Changes
Sequence Diagram(s)(none generated) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stripe/helpers/FrmStrpLiteConnectHelper.php (1)
817-824: Avoid potential “undefined property” warnings when accessing$data->subscriptions.
post_with_authenticated_bodycan returnnew stdClass()(Line 736-740). If that object lackssubscriptions, Line 823 can emit warnings in some environments.Proposed fix
- return false === $data ? false : $data->subscriptions; + if ( false === $data ) { + return false; + } + + return $data->subscriptions ?? array();Also consider updating the docblock (currently
@return false|object) to reflect the actual return type.classes/helpers/FrmAppHelper.php (1)
5003-5021:get_doc_urldocblock doesn’t match implementation (“affiliate tracking”).
The method adds UTM params viamaybe_add_missing_utm, but does not applymake_affiliate_url. Either adjust the docblock or wrap the final URL.Docblock-only fix
- * Get a documentation URL with UTM parameters and affiliate tracking. + * Get a documentation URL with UTM parameters.If affiliate tracking is actually intended for docs, then wrap the return value in
self::make_affiliate_url(...)instead (but that’s a behavior change).
🤖 Fix all issues with AI agents
In @phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php:
- Around line 112-113: In the SimplifyIfReturnSniff class, fix the pipeline
`blank_line_before_statement` by inserting a single blank line immediately
before the comment "// Get the condition and return values to check line
length." so the comment is separated from the previous logical block (PSR-12
compliant); update the surrounding method in SimplifyIfReturnSniff.php where
that comment appears.
- Around line 122-132: Remove the unused $isNegated variable: eliminate its
declaration and any assignments related to it in the conditional block that
inspects $firstNonWs (the result of $phpcsFile->findNext). Keep the logic that
detects T_BOOLEAN_NOT, trims $condition using $phpcsFile->getTokensAsString, and
swaps $firstReturnValue and $secondReturnValue; simply delete the lines
"$isNegated = false;" and "$isNegated = true;" so the code no longer defines or
updates $isNegated.
🧹 Nitpick comments (8)
classes/models/FrmInbox.php (1)
231-233: Ternary is correct, but? true :is redundant—consider a simpler boolean return.
Current logic is fine (andempty( $message['starts'] )safely handles missing keys), but this reads cleaner as a boolean expression.Proposed refactor
private function has_started( $message ) { - return empty( $message['starts'] ) ? true : $message['starts'] <= time(); + $now = time(); + return empty( $message['starts'] ) || $message['starts'] <= $now; }classes/helpers/FrmCSVExportHelper.php (1)
477-486: Consider simplifying the ternary to a boolean short-circuit.Current line is correct, but this is equivalent and a bit cleaner:
Proposed tweak
- return $section ? FrmField::is_repeating_field( $section ) : false; + return $section && FrmField::is_repeating_field( $section );classes/views/styles/components/FrmSliderStyleComponent.php (1)
156-164: Consider applying the ternary pattern consistently across all unit checks.Line 164 now uses a ternary for 'px' detection, while lines 156-162 still use if-blocks for '%' and 'em' detection. All three checks follow the same logical structure, so applying the ternary pattern uniformly would improve consistency.
♻️ Proposed refactor for consistency
- if ( preg_match( '/%$/', $value ) ) { - return '%'; - } - - if ( preg_match( '/em$/', $value ) ) { - return 'em'; - } - - return preg_match( '/px$/', $value ) ? 'px' : ''; + return preg_match( '/%$/', $value ) ? '%' : ( preg_match( '/em$/', $value ) ? 'em' : ( preg_match( '/px$/', $value ) ? 'px' : '' ) );Alternatively, if you prefer to keep the readability of separate statements:
- if ( preg_match( '/%$/', $value ) ) { - return '%'; - } - - if ( preg_match( '/em$/', $value ) ) { - return 'em'; - } - - return preg_match( '/px$/', $value ) ? 'px' : ''; + if ( preg_match( '/%$/', $value ) ) return '%'; + if ( preg_match( '/em$/', $value ) ) return 'em'; + return preg_match( '/px$/', $value ) ? 'px' : '';classes/models/FrmSpamCheckDenylist.php (1)
159-159: Consider simplifying the ternary with the logical OR operator.The ternary expression
$this->check_ip() ? true : $this->check_values()contains a redundant? true :sincecheck_ip()already returns a boolean. Using the logical OR operator would be more concise and idiomatic.♻️ Proposed simplification
- return $this->check_ip() ? true : $this->check_values(); + return $this->check_ip() || $this->check_values();classes/controllers/FrmDeactivationFeedbackController.php (1)
42-42: Consider using strict comparison for clarity.The truthiness check works correctly but treats
0(a valid timestamp representing Unix epoch) as falsy. While this produces the correct result in practice, using!== falsewould be more explicit about checking forstrtotime()failure.♻️ More explicit comparison
- return $expired_date ? $expired_date < time() : true; + return $expired_date !== false ? $expired_date < time() : true;classes/helpers/FrmStylesHelper.php (1)
1149-1157: Consider normalizing whitespace before indexing$partsThe ternary is equivalent, but
explode( ' ', $value )can produce empty tokens with multiple spaces/tabs/newlines, which may make$parts[2]not represent the “third value”. Switching to a whitespace split makes this more robust.Proposed tweak
public static function get_bottom_value( $value ) { if ( ! $value ) { return $value; } - $parts = explode( ' ', $value ); + $parts = preg_split( '/\s+/', trim( $value ), -1, PREG_SPLIT_NO_EMPTY ); return count( $parts ) < 3 ? $parts[0] : $parts[2]; }stripe/helpers/FrmStrpLiteConnectHelper.php (1)
701-710:get_customer_idternary looks equivalent, but consider??for clarity.
Current logic is fine; a small readability alternative is$data->customer_id ?? falseif you don’t needempty()semantics.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php (1)
31-33: Consider adding return type declarations.For consistency with modern PHP practices, consider adding
: arrayreturn type declaration to this method (and: voidto theprocessmethod on line 43).♻️ Proposed refinement
- public function register() { + public function register(): array { return array( T_IF ); }And for the
processmethod:- public function process( File $phpcsFile, $stackPtr ) { + public function process( File $phpcsFile, $stackPtr ): void { $tokens = $phpcsFile->getTokens();
📜 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 (26)
classes/controllers/FrmAddonsController.phpclasses/controllers/FrmDashboardController.phpclasses/controllers/FrmDeactivationFeedbackController.phpclasses/controllers/FrmFieldsController.phpclasses/controllers/FrmFormActionsController.phpclasses/controllers/FrmXMLController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmCSVExportHelper.phpclasses/helpers/FrmEmailSummaryHelper.phpclasses/helpers/FrmFieldGdprHelper.phpclasses/helpers/FrmFormsHelper.phpclasses/helpers/FrmStylesHelper.phpclasses/models/FrmCreateFile.phpclasses/models/FrmInbox.phpclasses/models/FrmMigrate.phpclasses/models/FrmSpamCheck.phpclasses/models/FrmSpamCheckDenylist.phpclasses/models/fields/FrmFieldType.phpclasses/views/styles/components/FrmSliderStyleComponent.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.phpsquare/controllers/FrmSquareLiteEventsController.phpstripe/controllers/FrmStrpLiteActionsController.phpstripe/controllers/FrmStrpLiteEventsController.phpstripe/helpers/FrmStrpLiteConnectHelper.phpstripe/helpers/FrmTransLiteAppHelper.phpstripe/models/FrmStrpLiteAuth.php
🧰 Additional context used
🧬 Code graph analysis (3)
classes/helpers/FrmCSVExportHelper.php (1)
classes/models/FrmField.php (2)
FrmField(6-1621)is_repeating_field(1518-1521)
classes/models/FrmSpamCheck.php (2)
classes/models/FrmSpamCheckDenylist.php (1)
get_spam_message(585-587)classes/models/FrmSpamCheckWPDisallowedWords.php (1)
get_spam_message(82-84)
classes/helpers/FrmFormsHelper.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5033)is_admin_page(491-505)
🪛 GitHub Actions: Inspections
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php
[error] 1-1: PHP CS Fixer issue detected: blank_line_before_statement in SimplifyIfReturnSniff.php.
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php
122-122: Avoid unused local variables such as '$isNegated'. (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). (9)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
🔇 Additional comments (25)
classes/helpers/FrmFormsHelper.php (1)
1001-1003: Clean ternary refactoring with no semantic change.The conversion from a multi-line if/else to a concise ternary operator is straightforward and correct. The
FrmAppHelper::is_admin_page()call is safe and the return values are clearly expressed.classes/helpers/FrmFieldGdprHelper.php (1)
76-76: LGTM!The ternary expression is concise, readable, and logically equivalent to the previous if/else pattern. This change aligns well with the PR's objective of simplifying short conditional returns.
classes/controllers/FrmFormActionsController.php (1)
803-805: LGTM! Clean ternary conversion.The refactor from if/else to a ternary expression is correct and maintains the same logic while improving conciseness. The condition is simple enough that readability is preserved.
classes/models/FrmMigrate.php (1)
167-170: LGTM!The ternary expression is clean and maintains equivalent logic. Both branches correctly return strings, and the empty string is an appropriate fallback when collation is not supported.
classes/controllers/FrmFieldsController.php (1)
939-939: LGTM! Clean refactoring to ternary expression.The conversion from if/else to a ternary expression maintains the same logic while being more concise. The function correctly returns
falsewhen$error_bodymatches a default HTML template, and returns the original$error_bodyotherwise.classes/models/FrmCreateFile.php (1)
143-145: LGTM!The ternary expression correctly maintains the same logic: returns file contents when permission is granted, otherwise an empty string. The refactor is clean and readable.
classes/controllers/FrmAddonsController.php (2)
987-987: LGTM! Clean ternary refactoring.The ternary expression correctly maintains the original logic and matches the documented return type of
true|WP_Error.
1266-1266: LGTM! Concise ternary expression.The refactoring correctly simplifies the form_builder detection logic while maintaining the same behavior.
classes/models/FrmSpamCheck.php (1)
42-42: LGTM! Clean refactoring to ternary expression.The conversion from multi-line conditional return to a ternary is correct and maintains the same logic while improving conciseness. The truthiness check appropriately handles the boolean return from
check()and correctly returns either the spam message string orfalse.classes/controllers/FrmXMLController.php (1)
144-147: LGTM!The ternary expression correctly preserves the original logic: when
$formis truthy, it decodes the JSON; otherwise, it returns the unchanged value. The refactor is clean and aligns with the PR objective.classes/helpers/FrmEmailSummaryHelper.php (2)
214-216: LGTM! Clean refactor to ternary.The instanceof check and ternary conversion are logically equivalent to the original if/else pattern, and the line remains readable.
226-236: LGTM! Ternary refactor preserves logic.The truthiness check on
$date2correctly handles both the false case (fromget_date_objfailure) and the valid DateTime case, maintaining equivalent behavior to the original early return pattern.stripe/helpers/FrmTransLiteAppHelper.php (1)
379-379: LGTM! Clean refactor to ternary.The refactor from an if/else block to a ternary expression maintains the same logic while improving conciseness. This is a suitable use case for a ternary operator.
stripe/controllers/FrmStrpLiteEventsController.php (1)
505-505: LGTM! Well-structured ternary with proper type checking.The ternary expression correctly guards against non-array values before calling
in_array, and uses strict comparison. The refactor maintains the original logic while improving readability.stripe/controllers/FrmStrpLiteActionsController.php (1)
33-33: LGTM! Concise and readable ternary.The refactor simplifies the conditional logic while preserving the original behavior. The truthy check on
$actionsis appropriate for this use case.stripe/models/FrmStrpLiteAuth.php (1)
615-615: LGTM! Clear validation pattern with ternary.The ternary expression provides a clean validation pattern: return the value if valid,
falseotherwise. Note that$nameis passed by reference tostatement_descriptor_is_valid(line 649) and may be modified during validation (e.g., truncated if too long), which is correctly handled here.square/controllers/FrmSquareLiteEventsController.php (1)
102-102: LGTM! Consistent refactor across payment integrations.The ternary expression follows the same pattern used in
FrmStrpLiteEventsController::should_skip_event(line 505), maintaining consistency across the codebase. The implementation correctly guards against non-array values and uses strict comparison.classes/controllers/FrmDashboardController.php (1)
522-530: Ternary return inget_dashboard_optionsis behavior-preserving here (given the prior guard).
The early return (Line 525-527) still prevents indexing a missing key, so the new ternary at Line 529 looks safe.stripe/helpers/FrmStrpLiteConnectHelper.php (1)
757-762:create_intentternary is fine (no behavioral change).
This remains a simple “return data or false” pattern.classes/helpers/FrmAppHelper.php (3)
193-207:pull_medium_from_linkternary is fine and clearer.
No concerns with theutm_mediumextraction change at Line 206.
2264-2282:current_user_canternary is OK; mostly a readability change.
WordPress typically returns false for logged-out users anyway, so this is effectively a minor simplification.
4992-5000: UTF-8 fallback looks correct for mixed WP versions.
Theseems_utf8fallback behindfunction_exists(Line 4999) is a reasonable compatibility approach.classes/models/fields/FrmFieldType.php (1)
122-124: Code change is safe; ternary simplification is appropriate.The logic is equivalent and more concise. The concern about accessing private properties through
__getdoes not apply here—all FrmField* subclasses declare their properties asprotected, notprivate, making them accessible via dynamic property access from the parent class method. No changes needed.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php (2)
43-162: Well-structured sniff implementation.The sniff logic is comprehensive and handles edge cases well:
- Validates proper if-block structure
- Ensures no else/elseif branches
- Checks for simple conditions and return values
- Cleverly handles negated conditions by flipping operands
- Enforces reasonable length constraints
- Properly applies fixes using PHP_CodeSniffer's fixer API
The implementation follows PHP_CodeSniffer best practices and should work reliably.
173-256: Well-designed helper methods with appropriate constraints.The
isSimpleReturnandisSimpleConditionhelpers effectively enforce simplicity constraints:
- Token count limits (10 and 15 respectively) are reasonable for maintaining readability
- Conservative exclusion of complex constructs (closures, arrays, logical operators) prevents overly complex ternaries
- Clear logic and good separation of concerns
This conservative approach correctly prioritizes code readability over maximizing the number of transformations.
Crabcyborg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php:
- Line 122: Remove the unused $isNegated variable assignment in
SimplifyIfReturnSniff.php: delete the line that sets $isNegated = false and any
other references to $isNegated, and keep the existing negation logic that swaps
the return values (so behavior is unchanged). Ensure no other code depends on
$isNegated and run tests/static analysis to confirm no references remain.
🧹 Nitpick comments (2)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php (2)
139-139: Extract magic numbers to class constants.The hardcoded values
75(line 139),10(line 204), and15(line 256) should be extracted to class constants for better maintainability and configurability.♻️ Proposed refactor
Add these constants at the top of the class:
/** * Maximum length for the ternary statement. */ private const MAX_TERNARY_LENGTH = 75; /** * Maximum token count for simple return expressions. */ private const MAX_RETURN_TOKENS = 10; /** * Maximum token count for simple conditions. */ private const MAX_CONDITION_TOKENS = 15;Then update the comparisons:
- if ( strlen( $ternary ) > 75 ) { + if ( strlen( $ternary ) > self::MAX_TERNARY_LENGTH ) {- return ! $hasComplex && $tokenCount <= 10; + return ! $hasComplex && $tokenCount <= self::MAX_RETURN_TOKENS;- return ! $hasComplex && $tokenCount <= 15; + return ! $hasComplex && $tokenCount <= self::MAX_CONDITION_TOKENS;
31-31: Consider adding return type declarations.The methods have return types documented in PHPDoc but lack actual return type declarations. If the codebase targets PHP 7.0+, adding type hints would improve type safety.
Example
- public function register() { + public function register(): array {- public function process( File $phpcsFile, $stackPtr ) { + public function process( File $phpcsFile, $stackPtr ): void {- private function isSimpleReturn( File $phpcsFile, $returnStart, $returnEnd ) { + private function isSimpleReturn( File $phpcsFile, $returnStart, $returnEnd ): bool {- private function isSimpleCondition( File $phpcsFile, $ifPtr ) { + private function isSimpleCondition( File $phpcsFile, $ifPtr ): bool {Also applies to: 43-43, 174-174, 215-215
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/helpers/FrmCSVExportHelper.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.phpsquare/controllers/FrmSquareLiteEventsController.phpstripe/controllers/FrmStrpLiteEventsController.php
🚧 Files skipped from review as they are similar to previous changes (2)
- stripe/controllers/FrmStrpLiteEventsController.php
- square/controllers/FrmSquareLiteEventsController.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/helpers/FrmCSVExportHelper.php (1)
classes/models/FrmField.php (2)
FrmField(6-1621)is_repeating_field(1518-1521)
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php
122-122: Avoid unused local variables such as '$isNegated'. (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 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (2)
classes/helpers/FrmCSVExportHelper.php (1)
477-486: LGTM: short-circuit return preserves behavior and avoids callingis_repeating_fieldon missing sections (Line 485).
$section && ...stays boolean and keeps the guard againstFrmField::getOne()returning a falsey value.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/SimplifyIfReturnSniff.php (1)
143-162: LGTM: Fix application logic is well-structured.The fix correctly uses changesets and replaces the entire if-return pattern with a single ternary statement. The implementation properly handles the token replacement.
Crabcyborg
deleted the
new_sniff_that_changes_returns_to_ternaries_for_short_lines
branch
…ns_to_ternaries_for_short_lines New sniff that changes returns to ternaries for simple logic
1 participant
No description provided.