Add rector rule to explicitly return null#2668
Conversation
|
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis PR systematically updates implicit return paths to explicit null returns across controllers, helpers, models, and views. Return type annotations in docblocks are updated from "void" or "array|void" to their nullable equivalents (e.g., "mixed[]|null", "string|null"). The rector.php configuration is adjusted to enable explicit null return generation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes The changes follow a highly consistent and repetitive pattern—adding explicit null returns and updating docblock return type annotations across the codebase. Each individual change is straightforward and low-risk. Areas requiring attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2668 +/- ##
============================================
+ Coverage 26.30% 26.62% +0.32%
- Complexity 8853 8873 +20
============================================
Files 144 145 +1
Lines 29934 29832 -102
============================================
+ Hits 7873 7942 +69
+ Misses 22061 21890 -171 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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/FrmListHelper.php (1)
234-249: Update the docblock return type to reflect nullable return.The method now explicitly returns
nullwhen the key doesn't exist in$this->_pagination_args, but the docblock still indicates@return int. Per the PR objectives to update return type annotations to their nullable equivalents, this should be updated.Apply this diff to update the docblock:
/** * Access the pagination args. * * @since 2.0.18 * * @param string $key Pagination argument to retrieve. Common values include 'total_items', * 'total_pages', 'per_page', or 'infinite_scroll'. * - * @return int Number of items that correspond to the given pagination argument. + * @return int|null Number of items that correspond to the given pagination argument, or null if not set. */ public function get_pagination_arg( $key ) {classes/helpers/FrmXMLHelper.php (1)
2041-2047: Return the original$post_contentinstead ofnullwhen no IDs need switching.When
$frm_duplicate_idsis empty, returningnullwill replace the caller's original data. At lines 2010 and 2336, the return value is directly assigned, sonullwould overwrite the existing$post_content. Since the function's purpose is to switch field IDs in content, it should return the original$post_contentunchanged when no switching is needed.Additionally, the docblock at line 2039 incorrectly states
@return string $post_content, but the function actually returns an array (line 2069) or null (line 2046).Apply this diff:
/** * Switch old field IDs for new field IDs in emails and post * * @since 2.0 * * @param array $post_content Check for old field IDs. * @param array $basic_fields Fields with string or int saved. * @param array $array_fields Fields with arrays saved. * - * @return string $post_content - new field IDs + * @return mixed[]|null new field IDs or null */ private static function switch_action_field_ids( $post_content, $basic_fields, $array_fields = array() ) { global $frm_duplicate_ids; // If there aren't IDs that were switched, end now if ( ! $frm_duplicate_ids ) { - return null; + return $post_content; }
🧹 Nitpick comments (3)
classes/helpers/FrmFormMigratorsHelper.php (1)
117-136: Minor inconsistency in return values.The method returns
''(empty string) at line 120 butnullat line 135. While both are falsy and the return value appears unused by callers, this creates a subtle inconsistency. Consider returningnullin both paths for consistency, or''if an empty string is semantically more appropriate.That said, since the return value is ignored by the caller in
maybe_show_download_link(), this is not a functional issue.private static function install_banner( $install ) { if ( empty( $install['link'] ) ) { - return ''; + return null; }classes/views/styles/components/FrmStyleComponent.php (1)
90-103: Atypical singleton pattern - consider documenting the intent.The method returns
nullwhen the instance already exists but returns the instance when first created. This is an unconventional singleton pattern. Typically,get_instance()would always return the instance:protected static function get_instance() { if ( ! self::$instance ) { self::$instance = new FrmStyleComponent(); } return self::$instance; }However, since the return value is discarded at line 114 (
self::get_instance();called for side effects only), this works correctly. The docblock update to\stdClass|nullaccurately reflects the current behavior.If the current behavior is intentional, a brief comment explaining why would help future maintainers understand the design choice.
classes/controllers/FrmAddonsController.php (1)
1207-1241: Includeboolinhandle_addon_actiondocblock return type
handle_addon_action()can return:
- an array on error,
true(e.g., successful uninstall), ornull(no-op / deactivate / successful activate).The new docblock only declares
mixed[]|null, which omits thetruecase and may confuse static analysis or future readers.Consider updating the annotation for accuracy:
- * @return mixed[]|null + * @return mixed[]|bool|null
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
classes/controllers/FrmAddonsController.php(3 hunks)classes/controllers/FrmEntriesController.php(1 hunks)classes/controllers/FrmFormsController.php(2 hunks)classes/controllers/FrmStylesController.php(1 hunks)classes/helpers/FrmAppHelper.php(2 hunks)classes/helpers/FrmFormMigratorsHelper.php(1 hunks)classes/helpers/FrmFormsHelper.php(1 hunks)classes/helpers/FrmListHelper.php(1 hunks)classes/helpers/FrmXMLHelper.php(1 hunks)classes/models/FrmFormAction.php(2 hunks)classes/models/FrmStyle.php(1 hunks)classes/models/fields/FrmFieldType.php(1 hunks)classes/views/styles/components/FrmStyleComponent.php(1 hunks)rector.php(0 hunks)stripe/controllers/FrmTransLiteSubscriptionsController.php(1 hunks)
💤 Files with no reviewable changes (1)
- rector.php
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmFormsController.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-5052)get_param(737-774)
classes/views/styles/components/FrmStyleComponent.php (1)
stubs.php (1)
instance(502-503)
🪛 PHPMD (2.15.0)
classes/models/fields/FrmFieldType.php
1195-1195: Avoid unused local variables such as '$frm_vars'. (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 trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (11)
stripe/controllers/FrmTransLiteSubscriptionsController.php (1)
161-161: LGTM! Explicit null returns improve clarity.The changes correctly convert implicit void returns to explicit null returns in the guard clauses. This aligns with the PR objective and improves code clarity. For WordPress shortcode handlers, returning null appropriately produces no output when the user is not logged in or has no subscriptions.
Also applies to: 168-168
classes/models/fields/FrmFieldType.php (1)
1192-1192: LGTM! Explicit null return improves clarity.The changes make the return behavior explicit: updating the docblock from
string|voidtostring|nulland adding an explicitreturn null;statement. This aligns with the PR objective to systematically replace implicit returns with explicit null returns.Also applies to: 1200-1200
classes/helpers/FrmFormsHelper.php (1)
1007-1007: LGTM! Explicit null return improves code clarity.This change correctly makes the implicit null return explicit when
$styleis empty and the current page is not the admin entries page. The function's documented return type already supportsstring|null, so this change enhances readability without altering behavior.Note: The AI summary contains a logic error. It states the function "now returns null instead of the previously accumulated class string ($class)" when on the admin entries page, but the code shows the opposite: Line 1005 returns
$classwhen it is the admin entries page, and line 1007 returnsnullwhen it's not the admin entries page.classes/models/FrmStyle.php (1)
590-596: LGTM!The explicit
return nullaligns with the existing docblock (@return object|null) and makes the fallback path clear when no default style is found in the loop.classes/controllers/FrmEntriesController.php (1)
121-139: LGTM!The explicit
return nullstatements in the route method are consistent with the@return mixeddocblock and make the control flow explicit in both the filter short-circuit path and the default case afterdisplay_list().classes/controllers/FrmStylesController.php (1)
1282-1289: LGTM!The explicit
return nullmakes the fallback behavior clear when either the style doesn't exist or the requested value key is not found.classes/models/FrmFormAction.php (1)
461-478: LGTM!The explicit
return nullstatements align with the@return array|nulldocblock and clearly indicate the early exit conditions when the update was already processed or when the POST data is not properly formatted.classes/controllers/FrmAddonsController.php (1)
1009-1025: Explicit null return fromdownload_and_activateis safeMaking the success path explicitly
return null;matches the previous implicit behavior and aligns with the updatedmixed[]|nulldocblock. Callers only branch on the array error case and otherwise ignore the return value, so there’s no behavioral change here.classes/helpers/FrmAppHelper.php (2)
1365-1408:icon_by_classexplicit null return matches existing usageFor
$echo === true, the function previously returnednullimplicitly; the new explicitreturn null;simply makes that contract clearer and consistent with thestring|nulldocblock. All existing call sites either rely on echo or pass'echo' => falsewhen they need the string, so this is safe.
1539-1554:clipreturn type is now explicit without changing behaviorAdding
return null;in the$echo === truebranch keepsclip()’s semantics intact (string when capturing, null when echoing) while making the control flow explicit. Callers likearray_to_html_paramsstill behave correctly in both echo and capture scenarios.classes/controllers/FrmFormsController.php (1)
2131-2185: Explicitreturn nullexits inroute()preserve existing control flowReplacing the prior implicit exits/breaks with explicit
return null;in thelite-reports,views, bulk-action, and message-handling paths doesn’t change behavior: there is no logic after the switch, androute()’s return value is ignored when used as an admin page callback. The new returns just make the early-exit semantics clearer and consistent with the rest of the PR.
…turn_null Add rector rule to explicitly return null
1 participant
No description provided.