New sniff to prevent redundant empty calls on defined object properties#2970
Merged
Crabcyborg merged 2 commits intoFeb 24, 2026
Merged
Conversation
DeepSource Code ReviewWe reviewed changes in See full review on DeepSource ↗ Code Review Summary
|
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
 |
|
Overall Grade Focus Area: Security |
Security  Reliability  Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Feb 24, 2026 1:32p.m. | Review ↗ |
| JavaScript |  |
Feb 24, 2026 1:32p.m. | Review ↗ |
Contributor
📝 WalkthroughWalkthroughThis PR replaces explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Crabcyborg
deleted the
new_sniff_to_prevent_redundant_empty_calls_on_object_properties
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
classes/models/FrmFieldValueSelector.php (1)
233-235:has_options()still uses! empty()on the same property — inconsistent with the PR's pattern shift.
display_dropdown()at Line 285 was updated toif ( $this->options ), buthas_options()(which guardsdisplay_dropdown()) still uses! empty( $this->options ). The new sniff should flag this as well.♻️ Proposed fix
final protected function has_options() { - return ! empty( $this->options ); + return (bool) $this->options; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@classes/models/FrmFieldValueSelector.php` around lines 233 - 235, The method has_options() still uses "! empty( $this->options )" which is inconsistent with the new pattern used in display_dropdown(); change has_options() to check the property directly and return a boolean, e.g. update final protected function has_options() to return (bool) $this->options; so it matches the new "if ( $this->options )" style and keeps the method's boolean contract.phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.php (2)
230-248:findContainingClassmisses anonymous classes and produces false negatives for traits.The method only matches
T_CLASS, soempty($this->prop)inside an anonymous class (T_ANON_CLASS) will never be flagged. Traits can also declare typed instance properties; those are silently skipped as well.Consider adding
T_ANON_CLASSto the check and, optionally,T_TRAIT:♻️ Proposed fix
- if ( $tokens[ $i ]['code'] !== T_CLASS ) { + if ( ! in_array( $tokens[ $i ]['code'], array( T_CLASS, T_ANON_CLASS ), true ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.php` around lines 230 - 248, findContainingClass currently only looks for tokens with code T_CLASS, so anonymous classes and traits are missed; update the token check in findContainingClass to consider T_ANON_CLASS (and optionally T_TRAIT) in addition to T_CLASS, ensuring the same scope_opener/scope_closer checks and the existing containment test ($stackPtr > scope_opener && $stackPtr < scope_closer) are applied to those token types so anonymous classes and trait declarations are correctly detected.
416-416:count( $tokens )re-evaluated on every loop iteration.
count()on a plain PHP array is O(1), but calling it in thewhilecondition is still unnecessary work and non-idiomatic.♻️ Proposed fix
- while ( $nextPtr < count( $tokens ) ) { + $token_count = count( $tokens ); + while ( $nextPtr < $token_count ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.php` at line 416, The while loop condition repeatedly calls count($tokens); to fix, compute the length once before the loop (e.g. $tokenCount = count($tokens);) and replace the condition in the while (currently "while ( $nextPtr < count( $tokens ) )") with "while ( $nextPtr < $tokenCount )"; update any inner code that may modify $tokens to adjust $tokenCount if necessary, and apply this change inside the RedundantEmptyOnDefinedPropertySniff class/method where $nextPtr and $tokens are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.php`:
- Around line 266-301: isDefinedProperty currently treats T_STATIC as a
standalone trigger which causes static properties like "private static $foo" to
be matched as instance properties; change the logic so static declarations are
ignored by either removing T_STATIC from the initial trigger list or
(preferably) by scanning modifiers between $i and $varToken to detect T_STATIC
and skipping the match when found. Specifically, in the loop inside
isDefinedProperty (use symbols $i, $tokens, $varToken, $phpcsFile->findNext and
$propertyName), after locating the next T_VARIABLE but before returning true,
inspect the tokens between $i and $varToken for any T_STATIC and if present
continue the outer loop (i.e., do not treat it as a matching instance property).
Ensure behavior still handles sequences like "public static $x" and "static $x"
correctly by only matching when no T_STATIC modifier is present.
---
Nitpick comments:
In `@classes/models/FrmFieldValueSelector.php`:
- Around line 233-235: The method has_options() still uses "! empty(
$this->options )" which is inconsistent with the new pattern used in
display_dropdown(); change has_options() to check the property directly and
return a boolean, e.g. update final protected function has_options() to return
(bool) $this->options; so it matches the new "if ( $this->options )" style and
keeps the method's boolean contract.
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.php`:
- Around line 230-248: findContainingClass currently only looks for tokens with
code T_CLASS, so anonymous classes and traits are missed; update the token check
in findContainingClass to consider T_ANON_CLASS (and optionally T_TRAIT) in
addition to T_CLASS, ensuring the same scope_opener/scope_closer checks and the
existing containment test ($stackPtr > scope_opener && $stackPtr < scope_closer)
are applied to those token types so anonymous classes and trait declarations are
correctly detected.
- Line 416: The while loop condition repeatedly calls count($tokens); to fix,
compute the length once before the loop (e.g. $tokenCount = count($tokens);) and
replace the condition in the while (currently "while ( $nextPtr < count( $tokens
) )") with "while ( $nextPtr < $tokenCount )"; update any inner code that may
modify $tokens to adjust $tokenCount if necessary, and apply this change inside
the RedundantEmptyOnDefinedPropertySniff class/method where $nextPtr and $tokens
are used.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
classes/helpers/FrmFieldGridHelper.phpclasses/helpers/FrmListHelper.phpclasses/models/FrmAddon.phpclasses/models/FrmCreateFile.phpclasses/models/FrmEmail.phpclasses/models/FrmEntryShortcodeFormatter.phpclasses/models/FrmEntryValues.phpclasses/models/FrmFieldFormHtml.phpclasses/models/FrmFieldValue.phpclasses/models/FrmFieldValueSelector.phpclasses/models/FrmFormApi.phpclasses/models/FrmFormMigrator.phpclasses/models/FrmSettings.phpclasses/models/FrmSolution.phpclasses/models/FrmTableHTMLGenerator.phpclasses/models/FrmValidate.phpclasses/models/fields/FrmFieldType.phpclasses/views/styles/components/FrmStyleComponent.phpphpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.phpphpcs-sniffs/Formidable/ruleset.xmlstripe/helpers/FrmStrpLiteLinkRedirectHelper.phptests/phpunit/entries/test_FrmShowEntryShortcode.php
Comment on lines
+266
to
+301
| for ( $i = $classOpener + 1; $i < $classCloser; $i++ ) { | ||
| // Skip nested class/function scopes. | ||
| if ( in_array( $tokens[ $i ]['code'], array( T_FUNCTION, T_CLOSURE ), true ) ) { | ||
| if ( isset( $tokens[ $i ]['scope_closer'] ) ) { | ||
| $i = $tokens[ $i ]['scope_closer']; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| // Look for property declarations: visibility modifier or var followed by $variable. | ||
| if ( ! in_array( $tokens[ $i ]['code'], array( T_PUBLIC, T_PROTECTED, T_PRIVATE, T_VAR, T_STATIC ), true ) ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Find the next variable token after the modifier. | ||
| $varToken = $phpcsFile->findNext( | ||
| array( T_WHITESPACE, T_STATIC, T_READONLY, T_STRING, T_NULLABLE, T_TYPE_UNION, T_TYPE_INTERSECTION, T_NULL, T_SELF, T_PARENT, T_ARRAY, T_CALLABLE, T_NS_SEPARATOR ), | ||
| $i + 1, | ||
| $classCloser, | ||
| true | ||
| ); | ||
|
|
||
| if ( false === $varToken || $tokens[ $varToken ]['code'] !== T_VARIABLE ) { | ||
| continue; | ||
| } | ||
|
|
||
| // Check if the variable name matches (strip the $). | ||
| $varName = ltrim( $tokens[ $varToken ]['content'], '$' ); | ||
|
|
||
| if ( $varName === $propertyName ) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
isDefinedProperty incorrectly matches static property declarations as instance properties.
T_STATIC appears in both the trigger list (T_PUBLIC, T_PROTECTED, T_PRIVATE, T_VAR, T_STATIC) and the findNext skip list. For a declaration like private static $foo = []:
- When
$ilands onT_PRIVATE,findNextskipsT_WHITESPACEandT_STATIC, then returnsT_VARIABLE($foo) — match. - When
$ilands onT_STATICitself,findNextalso finds$foo— match again.
If a class has only private static $foo (no instance $foo), the sniff will flag empty( $this->foo ) as redundant and produce a "fix". Because $this->foo accesses an undefined instance property, the autofix doesn't break runtime behaviour (both empty() and the truthiness check trigger the same deprecation or error in PHP 8.2+), but the detection is semantically incorrect.
🔧 Proposed fix — skip static property declarations
if ( false === $varToken || $tokens[ $varToken ]['code'] !== T_VARIABLE ) {
continue;
}
+ // Skip static properties: $this->prop accesses instance scope, not static scope.
+ $is_static_property = false;
+ for ( $j = $i; $j < $varToken; $j++ ) {
+ if ( $tokens[ $j ]['code'] === T_STATIC ) {
+ $is_static_property = true;
+ break;
+ }
+ }
+ if ( $is_static_property ) {
+ $i = $varToken;
+ continue;
+ }
+
// Check if the variable name matches (strip the $).
$varName = ltrim( $tokens[ $varToken ]['content'], '$' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/RedundantEmptyOnDefinedPropertySniff.php`
around lines 266 - 301, isDefinedProperty currently treats T_STATIC as a
standalone trigger which causes static properties like "private static $foo" to
be matched as instance properties; change the logic so static declarations are
ignored by either removing T_STATIC from the initial trigger list or
(preferably) by scanning modifiers between $i and $varToken to detect T_STATIC
and skipping the match when found. Specifically, in the loop inside
isDefinedProperty (use symbols $i, $tokens, $varToken, $phpcsFile->findNext and
$propertyName), after locating the next T_VARIABLE but before returning true,
inspect the tokens between $i and $varToken for any T_STATIC and if present
continue the outer loop (i.e., do not treat it as a matching instance property).
Ensure behavior still handles sequences like "public static $x" and "static $x"
correctly by only matching when no T_STATIC modifier is present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit