New sniff for adding missing return void comments#2814
Conversation
|
Warning Rate limit exceeded
⌛ 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)
📝 WalkthroughWalkthroughAdds a new PHP_CodeSniffer sniff (Formidable.Commenting.AddReturnVoid) that inserts Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (runs PHPCS)
participant PHPCS as PHP_CodeSniffer
participant Sniff as AddReturnVoidSniff
participant File as Source File
participant Fixer as PHPCS Fixer
Dev->>PHPCS: run analysis (with ruleset)
PHPCS->>File: tokenize & parse
PHPCS->>Sniff: invoke on T_FUNCTION
Sniff->>File: inspect docblock, scan for returns/terminators
alt Eligible (no return, no terminating calls, docblock present, no `@return`)
Sniff->>PHPCS: report fixable error MissingReturnVoid
PHPCS->>Fixer: apply fix (insert "@return void" at computed position)
Fixer->>File: update docblock with proper indentation
else Not eligible
Sniff-->>PHPCS: no action
end
PHPCS->>Dev: report results / apply fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🤖 Fix all issues with AI agents
In `@classes/models/FrmOnSubmitAction.php`:
- Around line 45-51: The override FrmOnSubmitAction::form currently declares
`@return` void and returns nothing, which conflicts with the parent
FrmFormAction::form that returns a string; change the FrmOnSubmitAction::form
PHPDoc to `@return` string and ensure the method returns the appropriate string
(e.g., return 'noform' or the same sentinel the parent uses) at the end of the
method so the return contract matches the parent.
In `@classes/views/frm-form-actions/email_action.php`:
- Around line 25-26: FrmEmailAction::form() has a docblock saying `@return` void
while the parent FrmFormAction::form() declares `@return` string; update the child
to match the parent's contract by changing the docblock to `@return` string and
having FrmEmailAction::form() return a string (e.g., an empty string) at the end
of the method so the signature and annotation align with FrmFormAction::form();
do not change FrmFormAction::form() here.
In `@phpcs-sniffs/Formidable/Sniffs/Commenting/AddReturnVoidSniff.php`:
- Around line 212-228: The findInsertPosition method declares an unused variable
$tokens which PHPMD flags; remove the unused declaration (the line assigning
$tokens = $phpcsFile->getTokens();) from the method in function
findInsertPosition(File $phpcsFile, $docStart, $docEnd) so the method only uses
$phpcsFile->findPrevious and returns $lastContent or false.
- Around line 239-255: The applyFix method declares an unused variable $tokens
by calling $phpcsFile->getTokens(); remove that unused declaration to satisfy
PHPMD — delete the line "$tokens = $phpcsFile->getTokens();" from the applyFix
function (leaving all other uses of $phpcsFile and $fixer intact) so there are
no side effects or behavioral changes.
In `@stripe/models/FrmTransLiteAction.php`:
- Around line 23-29: The override of form in FrmTransLiteAction::form declares
`@return` void but the parent FrmFormAction::form declares `@return` string, causing
Psalm errors; fix by making the return contract consistent—either change
FrmTransLiteAction::form to return a string (e.g., build and return the expected
HTML/string instead of void) or, if the parent is wrong, update
FrmFormAction::form to `@return` void and its signature accordingly; locate the
form method in FrmTransLiteAction and the parent FrmFormAction::form to apply
the matching return type and update docblocks and return statements so both
signatures agree.
🧹 Nitpick comments (1)
phpcs-sniffs/Formidable/Sniffs/Commenting/AddReturnVoidSniff.php (1)
125-142: Note the behavior with barereturn;statements.This method returns
truefor anyT_RETURNtoken, including barereturn;(return without value). This means functions with onlyreturn;won't have@return voidadded automatically, even thoughreturn;is semantically void.This is likely intentional (conservative approach), but worth documenting in the class docblock if so.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
classes/controllers/FrmAddonsController.phpclasses/controllers/FrmFormActionsController.phpclasses/controllers/FrmFormsController.phpclasses/helpers/FrmAppHelper.phpclasses/helpers/FrmListHelper.phpclasses/helpers/FrmXMLHelper.phpclasses/models/FrmInstallerSkin.phpclasses/models/FrmMigrate.phpclasses/models/FrmOnSubmitAction.phpclasses/models/fields/FrmFieldCheckbox.phpclasses/models/fields/FrmFieldRadio.phpclasses/models/fields/FrmFieldSubmit.phpclasses/views/frm-form-actions/email_action.phpphpcs-sniffs/Formidable/Sniffs/Commenting/AddReturnVoidSniff.phpphpcs.xmlstripe/models/FrmTransLiteAction.php
✅ Files skipped from review due to trivial changes (6)
- classes/helpers/FrmXMLHelper.php
- classes/models/FrmMigrate.php
- classes/controllers/FrmFormsController.php
- classes/models/fields/FrmFieldRadio.php
- classes/controllers/FrmAddonsController.php
- classes/controllers/FrmFormActionsController.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a PHP doc comment, but also like 'x.x' when functions like _deprecated_function() are used.
- Treat these placeholders as acceptable in this codebase.
Files:
classes/helpers/FrmListHelper.phpclasses/views/frm-form-actions/email_action.phpclasses/models/FrmInstallerSkin.phpclasses/models/fields/FrmFieldCheckbox.phpphpcs-sniffs/Formidable/Sniffs/Commenting/AddReturnVoidSniff.phpclasses/models/FrmOnSubmitAction.phpstripe/models/FrmTransLiteAction.phpclasses/helpers/FrmAppHelper.phpclasses/models/fields/FrmFieldSubmit.php
🧬 Code graph analysis (1)
classes/models/FrmInstallerSkin.php (1)
classes/models/FrmSolution.php (1)
header(283-314)
🪛 GitHub Actions: Psalm Code Analysis
classes/models/FrmOnSubmitAction.php
[error] 49-49: Psalm: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmOnSubmitAction::form 'null' (see https://psalm.dev/123)
🪛 GitHub Check: Psalm
classes/views/frm-form-actions/email_action.php
[failure] 26-26: ImplementedReturnTypeMismatch
classes/views/frm-form-actions/email_action.php:26:13: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmEmailAction::form 'null' (see https://psalm.dev/123)
classes/models/FrmOnSubmitAction.php
[failure] 49-49: ImplementedReturnTypeMismatch
classes/models/FrmOnSubmitAction.php:49:13: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmOnSubmitAction::form 'null' (see https://psalm.dev/123)
stripe/models/FrmTransLiteAction.php
[failure] 27-27: ImplementedReturnTypeMismatch
stripe/models/FrmTransLiteAction.php:27:13: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmTransLiteAction::form 'null' (see https://psalm.dev/123)
🪛 PHPMD (2.15.0)
phpcs-sniffs/Formidable/Sniffs/Commenting/AddReturnVoidSniff.php
213-213: Avoid unused local variables such as '$tokens'. (undefined)
(UnusedLocalVariable)
240-240: 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 (23)
classes/models/fields/FrmFieldCheckbox.php (1)
123-133: Docblock update looks good.
Adds the missing@return voidwithout changing behavior.classes/models/fields/FrmFieldSubmit.php (1)
79-83: Docblock update looks good.
The@return voidannotation matches the no-op implementation.classes/models/FrmInstallerSkin.php (3)
49-57: Docblock update looks good.
@return voidis consistent with an emptyheader()implementation.
60-66: Docblock update looks good.
@return voidis consistent with an emptyfooter()implementation.
99-110: Docblock update looks good.
@return voidis consistent with an emptyfeedback()implementation.classes/helpers/FrmListHelper.php (5)
267-268: LGTM!The
@return voidannotation is correct forno_items()which only echoes content.
530-531: LGTM!The
@return voidannotation is correct forview_switcher()which outputs HTML without returning a value.
1185-1186: LGTM!The
@return voidannotation is appropriate for this empty hook method.
1195-1196: LGTM!The
@return voidannotation is correct fordisplay_rows_or_placeholder().
1227-1228: LGTM!The
@return voidannotation is correct forsingle_row_columns().classes/helpers/FrmAppHelper.php (9)
983-984: LGTM!Appropriate
@return voidannotation for a method that modifies the value by reference.
2922-2923: LGTM!Correct
@return voidannotation formaybe_autocomplete_options()which echoes output.
2020-2021: LGTM!Appropriate
@return voidannotation for this dropdown rendering method.
2162-2163: LGTM!Correct
@return voidfor this dropdown rendering method.
2180-2181: LGTM!Appropriate
@return voidannotation.
2193-2194: LGTM!Correct
@return voidfor this method that echoes options.
4245-4246: LGTM!Appropriate
@return voidannotation forimages_dropdown().
4668-4669: LGTM!Correct
@return voidfor this utility method.
4688-4689: LGTM!Appropriate
@return voidannotation forshow_pill_text().phpcs-sniffs/Formidable/Sniffs/Commenting/AddReturnVoidSniff.php (3)
1-28: LGTM!Well-structured sniff with clear purpose and proper PHPDoc. The registration for
T_FUNCTIONtokens is appropriate.
38-114: LGTM!The
process()method has comprehensive checks including:
- Scope validation
- Constructor and test method exclusions
- Test file exclusions
- Return statement detection
- Terminating call detection
- Docblock ownership verification
- Existing
@returntag checkThe logic flow is sound and the fixable error approach is appropriate for an auto-fixing sniff.
265-280: LGTM!The
getDocblockIndent()method correctly handles indentation extraction for proper formatting of the inserted annotation.phpcs.xml (1)
232-235: LGTM!The new rule is properly added with an appropriate exclusion for
FrmListHelper.phpwhich has manual annotations in this PR.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /** | ||
| * @param object $instance | ||
| * @param array $args | ||
| * | ||
| * @return void | ||
| */ | ||
| public function form( $instance, $args = array() ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for FrmFormAction class and its form() method
rg -n "class FrmFormAction" --type php -A 5
# Also search for form method in FrmFormAction
rg -B5 -A10 "function form\s*\(" --type php | grep -A10 "FrmFormAction" | head -40Repository: Strategy11/formidable-forms
Length of output: 2879
🏁 Script executed:
# More targeted search for the form method in FrmFormAction
fd -e php | xargs grep -l "class FrmFormAction" | head -5Repository: Strategy11/formidable-forms
Length of output: 193
🏁 Script executed:
# Search directly for FrmFormAction class definition
rg -n "class FrmFormAction\b" --type phpRepository: Strategy11/formidable-forms
Length of output: 127
🏁 Script executed:
# View the full FrmOnSubmitAction::form method implementation
cat -n classes/models/FrmOnSubmitAction.php | sed -n '45,70p'Repository: Strategy11/formidable-forms
Length of output: 1095
🏁 Script executed:
# Double-check by viewing the parent method one more time to confirm return type
cat -n classes/models/FrmFormAction.php | sed -n '35,55p'Repository: Strategy11/formidable-forms
Length of output: 511
🏁 Script executed:
# Find the form() method in FrmFormAction with better context
rg -n "function form\(" classes/models/FrmFormAction.php -B 8 -A 5Repository: Strategy11/formidable-forms
Length of output: 457
Resolve return-type mismatch with parent FrmFormAction::form.
The parent method declares @return string (line 93 in FrmFormAction.php) and returns 'noform', but this override declares @return void and doesn't return anything. This violates the Liskov Substitution Principle and causes the Psalm error. Either update the parent contract to void if this is the intended behavior, or return a string here.
🧰 Tools
🪛 GitHub Actions: Psalm Code Analysis
[error] 49-49: Psalm: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmOnSubmitAction::form 'null' (see https://psalm.dev/123)
🪛 GitHub Check: PHPStan
[failure] 51-51:
Return type (void) of method FrmOnSubmitAction::form() should be compatible with return type (string) of method FrmFormAction::form()
🪛 GitHub Check: Psalm
[failure] 49-49: ImplementedReturnTypeMismatch
classes/models/FrmOnSubmitAction.php:49:13: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmOnSubmitAction::form 'null' (see https://psalm.dev/123)
🪛 PHPMD (2.15.0)
51-51: Avoid unused parameters such as '$instance'. (undefined)
(UnusedFormalParameter)
51-51: Avoid unused parameters such as '$args'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In `@classes/models/FrmOnSubmitAction.php` around lines 45 - 51, The override
FrmOnSubmitAction::form currently declares `@return` void and returns nothing,
which conflicts with the parent FrmFormAction::form that returns a string;
change the FrmOnSubmitAction::form PHPDoc to `@return` string and ensure the
method returns the appropriate string (e.g., return 'noform' or the same
sentinel the parent uses) at the end of the method so the return contract
matches the parent.
| * | ||
| * @return void |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the FrmFormAction class definition
fd -e php | xargs rg -l "class FrmFormAction"Repository: Strategy11/formidable-forms
Length of output: 193
🏁 Script executed:
# Search for the form method in FrmFormAction
rg "class FrmFormAction" -A 100 -t php | head -200Repository: Strategy11/formidable-forms
Length of output: 13001
🏁 Script executed:
# Also check the specific file mentioned in the review
cat -n classes/views/frm-form-actions/email_action.php | head -50Repository: Strategy11/formidable-forms
Length of output: 1729
🏁 Script executed:
# Search for calls to form() method to see if the return value is used
rg "->form\(" -B 2 -A 2 -t php | head -100Repository: Strategy11/formidable-forms
Length of output: 475
🏁 Script executed:
# Check other action subclasses to see how they implement form()
rg "public function form" classes/views/frm-form-actions/ -A 3 -t phpRepository: Strategy11/formidable-forms
Length of output: 477
🏁 Script executed:
# Search for calls to form() method
rg "->form\\\(" -B 2 -A 2 -t php | head -150Repository: Strategy11/formidable-forms
Length of output: 475
🏁 Script executed:
# Check other action subclasses to see how they implement form()
fd -e php "classes/views/frm-form-actions/" | xargs grep -l "public function form"Repository: Strategy11/formidable-forms
Length of output: 502
🏁 Script executed:
# Get a sample of other action implementations
fd -e php "classes/views/frm-form-actions/" -name "*_action.php" | head -5Repository: Strategy11/formidable-forms
Length of output: 301
🏁 Script executed:
# Search for calls to form() method properly
rg "form\(" classes/ -t php | grep -E "->form\(" | head -50Repository: Strategy11/formidable-forms
Length of output: 181
🏁 Script executed:
# List action files
fd . classes/views/frm-form-actions -name "*_action.php" -type fRepository: Strategy11/formidable-forms
Length of output: 301
🏁 Script executed:
# Check other action implementations
cat -n classes/views/frm-form-actions/on_submit_action.php | head -60Repository: Strategy11/formidable-forms
Length of output: 154
🏁 Script executed:
# List all action files in the directory
find classes/views/frm-form-actions -name "*_action.php" -type fRepository: Strategy11/formidable-forms
Length of output: 165
🏁 Script executed:
# Search for form method implementations in all php files
rg "public function form" classes/ -t php -A 5Repository: Strategy11/formidable-forms
Length of output: 2226
🏁 Script executed:
# Check the parent class return statement
cat classes/models/FrmFormAction.php | grep -A 5 "public function form"Repository: Strategy11/formidable-forms
Length of output: 271
🏁 Script executed:
# Check the other action file
cat classes/views/frm-form-actions/form_action.php | head -60Repository: Strategy11/formidable-forms
Length of output: 2761
🏁 Script executed:
# Check FrmOnSubmitAction to see if it has return type annotation
cat classes/models/FrmOnSubmitAction.php | grep -B 5 -A 10 "public function form"Repository: Strategy11/formidable-forms
Length of output: 590
🏁 Script executed:
# Search for where form() return value is actually used
rg "form\(" classes/ -t php -B 3 -A 3 | grep -A 5 -B 5 "= .*form\(" | head -80Repository: Strategy11/formidable-forms
Length of output: 94
🏁 Script executed:
# Check if return value of form() method is ever captured
rg "\$.*=.*\-\>form\(" classes/ -t phpRepository: Strategy11/formidable-forms
Length of output: 161
🏁 Script executed:
# Look at where form() is called in controllers
rg "form_action\-\>" classes/controllers -t php -A 2 | head -100Repository: Strategy11/formidable-forms
Length of output: 165
🏁 Script executed:
# Search for where form() method is called
rg "form_action.*form" classes/controllers -t php -B 2 -A 2 | head -100Repository: Strategy11/formidable-forms
Length of output: 6546
🏁 Script executed:
# Look for action_control variable usage
rg "action_control.*form" classes/ -t php -B 2 -A 2 | head -100Repository: Strategy11/formidable-forms
Length of output: 7076
🏁 Script executed:
# Check the parent class return type more carefully
rg -A 15 "public function form" classes/models/FrmFormAction.phpRepository: Strategy11/formidable-forms
Length of output: 440
🏁 Script executed:
# Check how action_control->form() is called
cat classes/views/frm-form-actions/_action_inside.php | grep -B 5 -A 5 "action_control->form"Repository: Strategy11/formidable-forms
Length of output: 473
🏁 Script executed:
# Check if any other implementations have different return type annotations
rg "public function form" classes/models -A 5 -t phpRepository: Strategy11/formidable-forms
Length of output: 1377
🏁 Script executed:
# Check the FrmOnSubmitAction documentation
cat classes/models/FrmOnSubmitAction.php | grep -B 10 "public function form"Repository: Strategy11/formidable-forms
Length of output: 293
🏁 Script executed:
# Look for any other form() implementations to see the pattern
fd . classes/ -name "*.php" -type f -exec grep -l "extends FrmFormAction" {} \;Repository: Strategy11/formidable-forms
Length of output: 301
🏁 Script executed:
# Find all classes that extend FrmFormAction
rg "extends FrmFormAction" classes/ -t phpRepository: Strategy11/formidable-forms
Length of output: 2277
🏁 Script executed:
# Check if the return value from form() is ever actually used
rg "form\(" classes/views/frm-form-actions/ -B 1 -A 1 -t phpRepository: Strategy11/formidable-forms
Length of output: 575
🏁 Script executed:
# Check how many child classes already have `@return` void
rg "extends FrmFormAction" classes/ -A 100 -t php | grep -B 5 "@return void"Repository: Strategy11/formidable-forms
Length of output: 512
🏁 Script executed:
# Count classes with `@return` string vs `@return` void
rg "extends FrmFormAction" classes/ -t php | while read -r line; do
file=$(echo "$line" | cut -d: -f1)
echo "Checking $file..."
grep -A 10 "public function form" "$file" | grep "@return"
done | sort | uniq -cRepository: Strategy11/formidable-forms
Length of output: 292
🏁 Script executed:
# Check the default_actions.php file for form() method return types
cat classes/views/frm-form-actions/default_actions.php | grep -B 5 "public function form" | grep -E "(class |@return)"Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Check more specifically in default_actions.php
rg "public function form" classes/views/frm-form-actions/default_actions.php -B 10 | grep -E "@return|class Frm"Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Let's check the default_actions.php file directly
wc -l classes/views/frm-form-actions/default_actions.phpRepository: Strategy11/formidable-forms
Length of output: 125
🏁 Script executed:
# Search for form method implementations in default_actions
rg "function form" classes/views/frm-form-actions/default_actions.php -A 8Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Check if return value from form() is ever captured or used
rg "action_control->form\(" classes/ -B 2 -A 2 -t phpRepository: Strategy11/formidable-forms
Length of output: 546
🏁 Script executed:
# Look at the complete _action_inside.php to see full context
cat classes/views/frm-form-actions/_action_inside.phpRepository: Strategy11/formidable-forms
Length of output: 5853
Return type mismatch with parent class.
FrmEmailAction::form() is annotated with @return void, but the parent class FrmFormAction::form() declares @return string. This creates a type annotation inconsistency that violates the Liskov Substitution Principle. While the return value is never actually used in the codebase (the method is called only for its side effects), the type annotation should match the parent's contract. Consider whether the parent class's return type should be changed to void or if this child implementation should return a value to maintain type consistency.
🧰 Tools
🪛 GitHub Check: Psalm
[failure] 26-26: ImplementedReturnTypeMismatch
classes/views/frm-form-actions/email_action.php:26:13: ImplementedReturnTypeMismatch: The inherited return type 'string' for FrmFormAction::form is different to the implemented return type for FrmEmailAction::form 'null' (see https://psalm.dev/123)
🤖 Prompt for AI Agents
In `@classes/views/frm-form-actions/email_action.php` around lines 25 - 26,
FrmEmailAction::form() has a docblock saying `@return` void while the parent
FrmFormAction::form() declares `@return` string; update the child to match the
parent's contract by changing the docblock to `@return` string and having
FrmEmailAction::form() return a string (e.g., an empty string) at the end of the
method so the signature and annotation align with FrmFormAction::form(); do not
change FrmFormAction::form() here.
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.