New rector rule PreparedValueToEarlyReturnRector#2670
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 24 minutes and 46 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 (1)
WalkthroughThis PR contains refactoring changes across the codebase focusing on code simplification: removing intermediate local variables, implementing early returns, and updating Rector and PHPStan configurations. Changes span controller and helper classes, test files, and configuration files, with updated PHPDoc annotations included. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![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 (1)
rector.php (1)
102-133: Syntax error in withPreparedSets() call - pipeline is failing.The pipeline reports a PHP parse error on line 133. The
withPreparedSets()method uses named parameters, not positional boolean arguments. The current syntax with inline comments followed bytrue/falsevalues is invalid.The correct syntax for
withPreparedSets()uses named parameters:->withPreparedSets( - // deadCode - true, - // codeQuality - true, - // codingStyle - true, - // typeDeclarations - true, - // privatization - true, - // naming - true, - // instanceOf - true, - // earlyReturn - true, - // strictBooleans - true, - // carbon - true, - // rectorPreset - true, - // phpunitCodeQuality - true, - // doctrineCodeQuality - false, - // symfonyCodeQuality - false, - // symfonyConfigs - false, + deadCode: true, + codeQuality: true, + codingStyle: true, + typeDeclarations: true, + privatization: true, + naming: true, + instanceOf: true, + earlyReturn: true, + strictBooleans: true, + carbon: true, + rectorPreset: true, + phpunitCodeQuality: true, )Note: Parameters like
doctrineCodeQuality,symfonyCodeQuality, andsymfonyConfigsset tofalsecan typically be omitted sincefalseis usually the default.
🧹 Nitpick comments (2)
classes/models/FrmAddon.php (1)
152-156:get_addonsimplification is fine; only edge case is explicitnullentriesUsing the null coalescing operator to return
$plugins[ $plugin_slug ] ?? falseis a clean simplification and is equivalent for the expected case where installed addons are non‑null objects/arrays.The only behavioral change is if a filter intentionally sets
$plugins[ $plugin_slug ]tonull; that would now be treated as “not found” (false) instead of returningnull. If you don’t rely onnullas a distinct state anywhere, this is perfectly safe.If you do care about distinguishing
nullfromfalse, you could preserve the old semantics with:public static function get_addon( $plugin_slug ) { $plugins = apply_filters( 'frm_installed_addons', array() ); - return $plugins[ $plugin_slug ] ?? false; + if ( array_key_exists( $plugin_slug, $plugins ) ) { + return $plugins[ $plugin_slug ]; + } + + return false; }classes/helpers/FrmAppHelper.php (1)
3203-3215:add_time_to_dateearly-return is correct; condition could be slightly simplified (optional)The new version still:
- Falls back to the option value when
$time_formatis empty.- Skips appending a time string when the format is empty or only whitespace.
- Returns the same formatted
" at <time>"suffix as before.If you want to tighten it further, you could rely solely on the trimmed value:
- $trimmed_format = trim( $time_format ); - - if ( $time_format && ! empty( $trimmed_format ) ) { + $trimmed_format = trim( $time_format ); + + if ( $trimmed_format !== '' ) { return ' ' . __( 'at', 'formidable' ) . ' ' . self::get_localized_date( $time_format, $date ); }Not required, but slightly reduces duplication in the condition.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.jsonis excluded by!**/*.json
📒 Files selected for processing (9)
classes/controllers/FrmAppController.php(1 hunks)classes/helpers/FrmAppHelper.php(2 hunks)classes/helpers/FrmFieldsHelper.php(1 hunks)classes/models/FrmAddon.php(1 hunks)classes/models/FrmCreateFile.php(1 hunks)classes/models/FrmFormApi.php(2 hunks)classes/models/fields/FrmFieldType.php(2 hunks)rector.php(3 hunks)stripe/helpers/FrmTransLiteAppHelper.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmAppController.php (1)
classes/helpers/FrmAppHelper.php (3)
FrmAppHelper(6-5051)simple_get(806-815)is_view_builder_page(519-535)
classes/helpers/FrmFieldsHelper.php (1)
classes/models/FrmField.php (2)
FrmField(6-1619)is_multiple_select(1344-1349)
🪛 GitHub Actions: PHP Syntax Check
rector.php
[error] 133-133: PHP Parse error: syntax error, unexpected ')' in ./rector.php on line 133.
🪛 GitHub Actions: Typo Checks
rector.php
[warning] 205-205: Symplify should be 'Simplify'.
[error] 205-205: typos failed: 'Symplify' should be 'Simplify'. Command './typos .' exited with code 2.
🪛 GitHub Check: Spell Check with Typos
rector.php
[warning] 205-205:
"Symplify" should be "Simplify".
⏰ 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). (1)
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (10)
stripe/helpers/FrmTransLiteAppHelper.php (1)
382-387: Early-return refactor inget_user_id_for_current_paymentlooks goodThe new implementation is behaviorally equivalent to the prior version and reads more clearly by avoiding the temporary variable. No issues spotted.
classes/models/FrmCreateFile.php (1)
143-148:get_file_contentsearly return keeps existing behaviorThe method now short-circuits when permissions are present and otherwise returns an empty string, matching the prior logic while being simpler. No functional concerns.
classes/helpers/FrmAppHelper.php (1)
437-445:is_formidable_adminrefactor preserves behavior and improves readabilityInlining the checks and using early return for the view‑builder case keeps the logic equivalent while making the control flow clearer. No issues found.
classes/controllers/FrmAppController.php (1)
273-282:get_current_pageearly-return keeps view-builder behavior intactReturning
'frm_display'immediately whenis_view_builder_page()is true, and otherwise falling back to the existing$page/$post_typelogic, is behaviorally equivalent to the old implementation and easier to read. Looks good.classes/helpers/FrmFieldsHelper.php (1)
753-758: Clean early-return refactor.The simplified control flow correctly handles the
selecttype case with an early return and falls through to return the field type directly for other types. The logic maintains behavioral equivalence with the previous implementation.classes/models/FrmFormApi.php (2)
371-376: Early return for rate-limited responses.The refactor correctly short-circuits to return a shorter cache timeout for 429 responses, improving readability while preserving the fallback behavior.
397-402: Simplified error_for_license with direct returns.Removing the intermediate
$errorsvariable and returning directly fromget_error_from_response()is a clean simplification that maintains identical behavior.classes/models/fields/FrmFieldType.php (2)
122-127: Direct returns in magic getter.The early return pattern here is cleaner and avoids unnecessary variable assignment when the property exists.
432-437: Simplified field_settings_for_type.Returning directly instead of assigning to an intermediate variable removes unnecessary complexity while maintaining the same behavior.
rector.php (1)
205-205: False positive from spell checker -SymplifyQuoteEscapeRectoris the correct class name.The static analysis warning about "Symplify" being a typo is incorrect.
SymplifyQuoteEscapeRectoris the actual class name from the Rector package - it's named after the Symplify organization. This can be safely ignored or added to the typos exclusion list.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
rector.php (2)
237-240: Duplicate skip rule entry (previously flagged).
ReturnTypeFromStrictConstantReturnRector::classis listed twice (lines 237 and 240). This was flagged in a previous review.
245-247: Duplicate skip rule entry (previously flagged).
MakeInheritedMethodVisibilitySameAsParentRector::classis listed twice (lines 245 and 247). This was flagged in a previous review.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rector.php(3 hunks)
⏰ 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). (1)
- GitHub Check: Cypress
🔇 Additional comments (2)
rector.php (2)
35-102: Imports look good, but verify PreparedValueToEarlyReturnRector is intentionally active.The import at line 49 (
PreparedValueToEarlyReturnRector) is not added to the skip list, meaning this rule will be active. This aligns with the PR title, so this appears intentional.
119-147: Configuration enables a comprehensive set of Rector rule categories.The
withPreparedSetsconfiguration now enables many rule categories (codeQuality, codingStyle, typeDeclarations, naming, earlyReturn, etc.) with appropriate exclusions for strictBooleans and carbon. The inline comments documenting each parameter are helpful.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2670 +/- ##
=========================================
Coverage 26.62% 26.62%
+ Complexity 8873 8872 -1
=========================================
Files 145 145
Lines 29832 29820 -12
=========================================
- Hits 7942 7940 -2
+ Misses 21890 21880 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
rector.php (1)
238-241: Remove duplicate skip entry.
ReturnTypeFromStrictConstantReturnRector::classappears twice (lines 238 and 241). This was flagged in previous reviews but remains in the code.🔎 Apply this diff to remove the duplicate:
StringReturnTypeFromStrictScalarReturnsRector::class, ReturnTypeFromStrictConstantReturnRector::class, PostIncDecToPreIncDecRector::class, MultiDirnameRector::class, - ReturnTypeFromStrictConstantReturnRector::class, ReturnTypeFromStrictTernaryRector::class,
🧹 Nitpick comments (4)
tests/phpunit/fields/test_FrmFieldGridHelper.php.php (1)
87-89: Good cleanup of unused variable.The removal of the
$quarter_width_fieldvariable assignment is appropriate since the variable is not referenced anywhere in the test method. The field creation is still executed (preserving any setup side effects), but eliminating the unused variable improves code clarity.If the field created on line 89 is not needed for the test at all (not just the variable reference), consider adding a comment explaining why it's created, or removing the creation entirely if it serves no purpose.
tests/phpunit/fields/test_FrmField.php (1)
8-8: Good addition to formalize the property declaration.The property is already used throughout the test methods (lines 16, 20, 41), so declaring it explicitly is correct. However, consider adding type information and documentation for better IDE support and maintainability.
🔎 Suggested improvements
If the factory type is known (appears to be a test factory with
formandfieldproperties), add type hint and documentation:+ /** + * @var WP_UnitTest_Factory|null Factory instance for creating test data. + */ - public $factory; + public $factory;Or if using PHP 7.4+ with proper type:
- public $factory; + public ?WP_UnitTest_Factory $factory;tests/phpunit/forms/test_FrmFormsController.php (1)
57-57: Consider removing unused function call.The
ob_get_contents()call captures output but the result is immediately discarded. Sinceob_end_clean()on line 58 clears the buffer anyway, this call serves no purpose and could be removed entirely.🔎 Apply this diff to remove the unused call:
ob_start(); FrmFormsController::update(); -ob_get_contents(); ob_end_clean();tests/phpunit/misc/test_FrmAppHelper.php (1)
7-7: Unused property declaration.The
$factoryproperty is declared but never used in this test file. Consider removing it to avoid clutter.🔎 Apply this diff to remove the unused property:
- public $factory;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
rector.php(3 hunks)tests/phpunit/base/FrmUnitTest.php(1 hunks)tests/phpunit/bootstrap.php(1 hunks)tests/phpunit/database/test_FrmMigrate.php(1 hunks)tests/phpunit/emails/test_FrmEmail.php(3 hunks)tests/phpunit/entries/test_FrmEntriesAJAXSubmitController.php(1 hunks)tests/phpunit/entries/test_FrmEntriesController.php(1 hunks)tests/phpunit/entries/test_FrmEntriesHelper.php(1 hunks)tests/phpunit/entries/test_FrmEntriesListHelper.php(1 hunks)tests/phpunit/entries/test_FrmEntry.php(1 hunks)tests/phpunit/entries/test_FrmEntryFormatter.php(1 hunks)tests/phpunit/entries/test_FrmEntryMeta.php(1 hunks)tests/phpunit/entries/test_FrmEntryValidate.php(1 hunks)tests/phpunit/entries/test_FrmPersonalData.php(2 hunks)tests/phpunit/entries/test_FrmShowEntryShortcode.php(11 hunks)tests/phpunit/fields/test_FrmField.php(1 hunks)tests/phpunit/fields/test_FrmFieldCombo.php(1 hunks)tests/phpunit/fields/test_FrmFieldFormHtml.php(1 hunks)tests/phpunit/fields/test_FrmFieldGridHelper.php.php(2 hunks)tests/phpunit/fields/test_FrmFieldName.php(1 hunks)tests/phpunit/fields/test_FrmFieldType.php(1 hunks)tests/phpunit/fields/test_FrmFieldValidate.php(1 hunks)tests/phpunit/fields/test_FrmFieldsAjax.php(1 hunks)tests/phpunit/fields/test_FrmFieldsController.php(1 hunks)tests/phpunit/fields/test_FrmFieldsHelper.php(1 hunks)tests/phpunit/fields/test_FrmSubmitHelper.php(1 hunks)tests/phpunit/forms/test_FrmForm.php(1 hunks)tests/phpunit/forms/test_FrmFormAction.php(1 hunks)tests/phpunit/forms/test_FrmFormsController.php(3 hunks)tests/phpunit/forms/test_FrmFormsControllerAjax.php(1 hunks)tests/phpunit/forms/test_FrmFormsHelper.php(1 hunks)tests/phpunit/forms/test_FrmOnSubmitAction.php(1 hunks)tests/phpunit/misc/test_FrmAntiSpam.php(1 hunks)tests/phpunit/misc/test_FrmAppHelper.php(4 hunks)tests/phpunit/misc/test_FrmCSVExportHelper.php(2 hunks)tests/phpunit/misc/test_FrmOverlayController.php(0 hunks)tests/phpunit/stripe/FrmStrpLiteUnitTest.php(5 hunks)tests/phpunit/stripe/test_FrmTransLiteActionsController.php(3 hunks)tests/phpunit/styles/test_FrmStylesController.php(1 hunks)tests/phpunit/styles/test_FrmStylesHelper.php(1 hunks)tests/phpunit/test_ajax.php(1 hunks)tests/phpunit/views/shortcodes/test_FrmFieldShortcodes.php(2 hunks)
💤 Files with no reviewable changes (1)
- tests/phpunit/misc/test_FrmOverlayController.php
✅ Files skipped from review due to trivial changes (2)
- tests/phpunit/fields/test_FrmFieldFormHtml.php
- tests/phpunit/fields/test_FrmFieldValidate.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/phpunit/stripe/test_FrmTransLiteActionsController.php (1)
stripe/controllers/FrmTransLiteActionsController.php (2)
FrmTransLiteActionsController(6-646)get_actions_for_form(454-464)
tests/phpunit/styles/test_FrmStylesController.php (1)
tests/phpunit/entries/test_FrmShowEntryShortcode.php (1)
get_defaults(798-803)
tests/phpunit/misc/test_FrmAppHelper.php (1)
classes/helpers/FrmAppHelper.php (4)
FrmAppHelper(6-5051)get_unique_key(2700-2747)get_ip_address(660-682)clip(1538-1554)
🪛 GitHub Actions: PHPUnit
tests/phpunit/bootstrap.php
[error] 26-26: PHP Warning: require(/tmp/wordpress/src/wp-content/plugins/formidable/tests/phpunit/../../../../tests/phpunit/includes/bootstrap.php): Failed to open stream: No such file or directory in /tmp/wordpress/src/wp-content/plugins/formidable/tests/phpunit/bootstrap.php on line 26. Error in bootstrap script: Failed opening required '/tmp/wordpress/src/wp-content/plugins/formidable/tests/phpunit/../../../../tests/phpunit/includes/bootstrap.php' (include_path='.:/usr/share/php')
⏰ 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). (1)
- GitHub Check: Cypress
🔇 Additional comments (47)
tests/phpunit/fields/test_FrmFieldsController.php (1)
8-8: LGTM! Property declaration improves code clarity.Explicitly declaring the
$factoryproperty is good practice, especially since it's already used throughout the test methods. This improves IDE support and makes the code more maintainable.tests/phpunit/fields/test_FrmFieldGridHelper.php.php (1)
8-8: No issues found with the$factoryproperty declaration.The explicit declaration of
public $factory;is a valid and beneficial practice. It inherits fromWP_UnitTestCase(the WordPress unit test base class) and explicit declaration enables IDE autocomplete, static analysis, and type hinting. This pattern is consistently used throughout the test suite.tests/phpunit/fields/test_FrmField.php (1)
8-8: Remove redundant property declaration.The
$factoryproperty is already properly initialized in the parent classFrmUnitTest::setUp()method (line 53 of tests/phpunit/base/FrmUnitTest.php), which executes before each test method. This declaration is unnecessary and should be removed.tests/phpunit/fields/test_FrmFieldsHelper.php (1)
10-10: This property declaration is valid and necessary—no conflicts exist with the parent class.The parent class
FrmUnitTestdoes not declare a$factoryproperty; instead, it dynamically assigns it in thesetUp()method (line 53:$this->factory = new FrmUnitTestFactory()). The explicitpublic $factory;declaration in child test classes is a necessary improvement for static analysis tool compatibility and to prevent PHP 8.2+ dynamic property deprecation warnings.tests/phpunit/fields/test_FrmFieldsAjax.php (1)
9-9: Verify that exposing$factoryas public is intentional.The
$factoryproperty is now explicitly declared aspublicacross multiple test classes in this PR. Since the parent classsetUp()already initializes this property, the explicit declaration appears to be for visibility control or IDE support.Making test fixtures public exposes internal test state that could potentially be modified by external code. If the intent is to provide access for test helpers or utilities, this is acceptable. However, if it's only used within the test class itself,
protectedvisibility would be more appropriate.Consider whether public visibility is necessary, or if these declarations could be
protectedinstead. Additionally, for better type safety in PHP 7.4+, consider adding type hints (e.g.,protected FrmUnitTestFactory $factory;).This same pattern appears in:
- tests/phpunit/fields/test_FrmFieldCombo.php
- tests/phpunit/entries/test_FrmEntryMeta.php
- tests/phpunit/forms/test_FrmOnSubmitAction.php
- tests/phpunit/fields/test_FrmFieldType.php
- tests/phpunit/entries/test_FrmEntry.php
- tests/phpunit/entries/test_FrmEntryValidate.php
tests/phpunit/base/FrmUnitTest.php (1)
604-604: LGTM: Using canonical function name.Changed from
fputs()tofwrite(). Sincefputs()is simply an alias offwrite(), this change uses the canonical function name without any functional difference. Minor code quality improvement.tests/phpunit/entries/test_FrmEntriesAJAXSubmitController.php (1)
8-8: LGTM! Good practice to declare the property explicitly.Making the
$factoryproperty explicit improves IDE support and type hinting, even though it's inherited from the parentFrmUnitTestclass.tests/phpunit/database/test_FrmMigrate.php (1)
8-8: LGTM! Consistent property declaration.Explicitly declaring the
$factoryproperty aligns with the broader test suite pattern and improves code clarity.tests/phpunit/fields/test_FrmFieldName.php (1)
8-8: LGTM! Property declaration follows the established pattern.tests/phpunit/styles/test_FrmStylesHelper.php (1)
8-8: LGTM! Explicit property declaration improves code maintainability.tests/phpunit/forms/test_FrmFormsHelper.php (1)
8-8: LGTM! Consistent with the test suite enhancement.tests/phpunit/entries/test_FrmEntriesListHelper.php (1)
8-8: LGTM! Property declaration aligns with test suite standards.tests/phpunit/fields/test_FrmSubmitHelper.php (1)
5-5: LGTM! Factory property declared explicitly.This follows the same pattern as the other test class updates in this PR.
tests/phpunit/entries/test_FrmEntryFormatter.php (1)
8-8: LGTM!The explicit
$factoryproperty declaration aligns with the broader pattern across the test suite to make factory access explicit.tests/phpunit/emails/test_FrmEmail.php (3)
9-9: LGTM!The explicit
$factoryproperty declaration is consistent with the broader test suite pattern.
433-435: LGTM!Direct return simplifies the method without changing behavior.
505-507: LGTM!Direct return of the literal string is cleaner than assigning to an intermediate variable.
tests/phpunit/views/shortcodes/test_FrmFieldShortcodes.php (2)
10-10: LGTM!The explicit
$factoryproperty declaration follows the established pattern.
74-88: LGTM!Directly returning the array eliminates the unnecessary intermediate
$expectedvariable.tests/phpunit/test_ajax.php (1)
7-7: LGTM!The explicit
$factoryproperty declaration is consistent with the test suite pattern.tests/phpunit/stripe/test_FrmTransLiteActionsController.php (2)
8-8: LGTM!The explicit
$factoryproperty declaration follows the established pattern.
18-32: LGTM!Removing the unused
$action_idassignment is a valid cleanup. The test retrieves the action viaget_actions_for_form()on line 31 anyway.tests/phpunit/misc/test_FrmCSVExportHelper.php (2)
7-7: LGTM!The explicit
$factoryproperty declaration follows the established pattern.
98-106: LGTM!Removing the unused
$embed_fieldassignment is a valid cleanup. The embed field is created for its side effect (being stored in the database), and the returned object isn't needed.tests/phpunit/stripe/FrmStrpLiteUnitTest.php (5)
5-5: LGTM!The explicit
$factoryproperty declaration follows the established pattern.
237-239: LGTM!Using the null coalescing operator for a direct return is cleaner than the previous intermediate variable approach.
291-296: LGTM!Inlining the
get_stripe_account_id_details()call directly as the parameter is a valid simplification.
301-310: LGTM!Directly returning the array eliminates the unnecessary
$new_chargeintermediate variable.
451-455: LGTM!The
get_customer()call is invoked for its side effect (ensuring a customer exists). Removing the unused$customerassignment is correct.tests/phpunit/entries/test_FrmShowEntryShortcode.php (4)
13-13: LGTM! Explicit property declaration improves clarity.Making the
$factoryproperty explicitly public is good practice and improves IDE support and code clarity.
758-758: LGTM! Clean refactor to early return.The direct return eliminates unnecessary intermediate variables and improves readability.
899-905: The change from strict equality to truthy evaluation is safe.The
$includevariable is guaranteed to always contain a boolean value. All assignments are either boolean literals (true/false) or results ofin_array()with strict comparison, which always returns a boolean. Theis_self_or_parent_in_array()method returnsin_array(..., true), ensuring a boolean result. Using loose evaluationif ( $include )instead ofif ( $include === true )is actually more idiomatic PHP and poses no risk of unexpected behavior from truthy non-boolean values.
30-34: No issues found. The parent classFrmUnitTestdoes not have a constructor with initialization logic—all setup occurs in thesetUp()method, which is automatically invoked by PHPUnit and does not require an explicit call in the child class's constructor.tests/phpunit/entries/test_FrmEntriesHelper.php (1)
8-8: LGTM! Explicit property declaration.Consistent with the pattern across test files to make the
$factoryproperty explicitly public.tests/phpunit/forms/test_FrmFormAction.php (1)
8-8: LGTM! Consistent property declaration.Aligns with the project-wide pattern of explicitly declaring the
$factoryproperty.tests/phpunit/forms/test_FrmForm.php (1)
8-8: LGTM! Property declaration for consistency.Consistent with other test classes in this PR.
tests/phpunit/entries/test_FrmEntriesController.php (1)
8-8: LGTM! Explicit factory property.Good practice for test fixture management.
tests/phpunit/forms/test_FrmFormsControllerAjax.php (1)
8-8: LGTM! Property declaration pattern.Maintains consistency across test suite.
tests/phpunit/entries/test_FrmPersonalData.php (2)
8-8: LGTM! Explicit property for test factory.Consistent with the broader test suite improvements.
65-70: LGTM! Documentation improvement.Adding PHPDoc improves code documentation and IDE support.
tests/phpunit/forms/test_FrmFormsController.php (2)
9-9: LGTM! Consistent property declaration.Aligns with the test factory pattern across the codebase.
115-115: LGTM! Simplified assignment.The direct assignment is clearer than a conditional and preserves the same boolean result.
tests/phpunit/misc/test_FrmAppHelper.php (2)
570-570: LGTM: Modern class reference syntax.The refactor from
__CLASS__toself::classimproves consistency and follows modern PHP best practices.Also applies to: 578-578
700-700: LGTM: Consistent class reference syntax.The refactor from
__CLASS__toself::classfor building callable strings is consistent with modern PHP practices and matches the changes elsewhere in the file.Also applies to: 706-706
rector.php (3)
35-102: LGTM: Imports align with configuration.The new imports support the expanded Rector configuration below. Notably,
PreparedValueToEarlyReturnRector(line 49) is imported but not included in the skip array, meaning it will be enabled as intended per the PR objectives.
112-112: No action needed—including the tests directory in Rector paths is standard practice and ensures consistent code quality across the entire project, including test code.
120-148: This is a new Rector configuration file, not modifications to an existing configuration.The file introduces a new rector.php with multiple prepared sets enabled (codeQuality, codingStyle, typeDeclarations, typeDeclarationDocblocks, privatization, naming, instanceOf, earlyReturn, rectorPreset, phpunitCodeQuality) along with an extensive skip list of 64+ specific rules (lines 169-274). This is a valid approach that enables broad rule sets while selectively excluding problematic rules. The configuration should be tested to ensure it produces the desired refactoring behavior.
Likely an incorrect or invalid review comment.
Crabcyborg
deleted the
new_rector_rule_PreparedValueToEarlyReturnRector
branch
…eToEarlyReturnRector New rector rule PreparedValueToEarlyReturnRector
1 participant
No description provided.