[#17] Fixed variable references in comments not being updated during auto-fix.#18
Merged
AlexSkrypnyk merged 4 commits intomainfrom Mar 12, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comment-aware auto-fix support to LocalVariableNamingSniff: a public Changes
Sequence DiagramsequenceDiagram
participant Sniff as LocalVariableNamingSniff
participant File as PHP Token Stream
participant Comments as Comment Tokens
participant Fixer as PHPCBF/Fixer
Sniff->>File: process() detect NotSnakeCase variable (oldName)
Sniff->>Comments: findPrecedingComment(stackPtr)
Comments-->>Sniff: comment token range or empty
alt comment(s) found and eligible per fixCommentTypes
Sniff->>Fixer: fixCommentVariable(file, commentTokens, oldName, newName)
Fixer->>Comments: scan comment text for variable references
alt single matching variable reference
Fixer->>Comments: replace oldName -> newName in comment
Comments-->>Fixer: replacement recorded
else multiple distinct variables referenced
Fixer-->>Fixer: skip comment (ambiguous)
end
Fixer-->>Sniff: comment fix applied or skipped
end
Sniff->>File: update variable token to newName
Sniff-->>Fixer: submit fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 420 444 +24
=========================================
+ Hits 420 444 +24 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php`:
- Around line 133-147: The backward scan in LocalVariableNamingSniff currently
treats any preceding T_COMMENT/T_DOC as attachable and can capture trailing
comments from the previous statement; update the loop that walks from $stackPtr
backwards to only collect comment tokens when they are actually adjacent to the
variable (e.g. same line for end-of-line comments or immediately preceding lines
with no intervening non-whitespace tokens). Concretely, inside the for loop that
inspects $tokens[$i] (the scan in LocalVariableNamingSniff around $stackPtr),
add checks using $tokens[$i]['line'] vs $tokens[$stackPtr]['line'] (and allow a
one-line difference for a directly preceding block comment) and stop the scan
when a comment token is on an earlier non-adjacent line so you don’t attach
comments from the previous statement.
- Around line 178-190: The guard that builds $all_text from $commentTokens and
then checks $unique_vars before calling fixCommentTypes is too broad and can
block valid `@var` fixes because it aggregates neighboring comments; instead,
restrict the distinct-variable check to the single comment being processed (use
the individual token content rather than concatenating $commentTokens) or move
the unique-variable logic into the per-comment processing path (so
fixCommentTypes inspects only that token's content); update the logic around
$all_text, $commentTokens and the unique_vars check so that fixCommentTypes is
called for a comment token unless that specific token contains multiple distinct
$variables.
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 106-186: Add a fix-mode assertion to the existing
testCommentVariableNaming() functional test: after the current runPhpcs(...)
violation-only check for the CommentVariableNaming.php fixture, invoke the PHPCS
fix runner (e.g. the project helper used for tests that runs phpcbf/fix mode) on
the same fixture and assert the resulting file contents (or fixed output)
include the corrected snake_case variable names in comments (e.g.
"$module_handler", "$first_handler", "$second_handler") and that comment
references were rewritten; update the test to call the fix-runner for
CommentVariableNaming.php and assert the fixed comment text matches expected
replacements.
In `@tests/Unit/LocalVariableNamingSniffTest.php`:
- Around line 175-244: Add a regression case to dataProviderFindPrecedingComment
that covers a previous statement with a trailing inline comment (e.g. "$other =
1; // $myVar") and assert findPrecedingComment() does not attach that trailing
comment to the following variable; specifically add a provider entry (similar to
'code_between_comment_and_variable') where the source has a line with code plus
a trailing "// $myVar" followed by "$myVar = get_service();" and set the
expected boolean to FALSE so the test ensures findPrecedingComment() returns
empty for the next variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e950c51-ec14-432c-a30d-21a0c1b644d8
📒 Files selected for processing (5)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.phptests/Fixtures/CommentVariableNaming.phptests/Fixtures/VariableNaming.phptests/Functional/LocalVariableNamingSniffFunctionalTest.phptests/Unit/LocalVariableNamingSniffTest.php
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php
Outdated
Show resolved
Hide resolved
AlexSkrypnyk
force-pushed
the
feature/17-fix-comment-vars
branch
from
953d8ef to
b555a97
Compare
… with unqualified FQN types.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php (1)
188-207:⚠️ Potential issue | 🟠 MajorScope the multi-variable skip check to each comment token, not the whole collected block.
The current
$all_textaggregation can still block a valid@varrewrite when an adjacent eligible comment references a different variable. Skip should be decided per token being rewritten.💡 Proposed fix
- // Collect text only from eligible comment token types to check for - // multiple variables. This prevents unrelated neighboring comments - // from blocking a valid fix. - $all_text = ''; - foreach ($commentTokens as $ptr) { - $code = $tokens[$ptr]['code']; - $is_doc = ($code === T_DOC_COMMENT_STRING); - $is_inline = ($code === T_COMMENT); - if (($is_doc && in_array('doc', $fix_types, TRUE)) || ($is_inline && in_array('inline', $fix_types, TRUE))) { - $all_text .= $tokens[$ptr]['content']; - } - } - - // Count distinct $variable references in the eligible comment text. - preg_match_all('/\$[a-zA-Z_]\w*/', $all_text, $matches); - $unique_vars = array_unique($matches[0]); - if (count($unique_vars) > 1) { - // Multiple variables in comment — skip (requires manual fix). - return; - } - $pattern = '/\$' . preg_quote($oldName, '/') . '(?![a-zA-Z0-9_])/'; @@ // Only process tokens that can contain variable references. if (!$is_doc && !$is_inline) { continue; } + + // Skip only this token when it references multiple distinct variables. + preg_match_all('/\$[a-zA-Z_]\w*/', $content, $matches); + $unique_vars = array_unique($matches[0]); + if (count($unique_vars) > 1) { + continue; + } if (preg_match($pattern, $content) === 1) { $new_content = (string) preg_replace($pattern, '$' . $newName, $content);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php` around lines 188 - 207, The current multi-variable skip uses an aggregated $all_text across $commentTokens which can wrongfully block a rewrite when only an adjacent comment contains another variable; change the logic to scope the check per token: inside the loop over $commentTokens (using $ptr and $tokens[$ptr]['content']) run the variable extraction (preg_match_all('/\$[a-zA-Z_]\\w*/', ...)) and compute array_unique on that single token's matches, and if count($unique_vars) > 1 for that token then skip rewriting that token only; otherwise allow the rewrite for that token (remove the global $all_text aggregation and use per-token $matches/$unique_vars checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Functional/FunctionalTestCase.php`:
- Around line 147-158: The temp-file handling in runPhpcbf() is fragile: verify
tempnam() succeeded and that copy() returned true before calling processRun,
read contents only after confirming file exists, and always remove the temp file
in a finally block to avoid leaks; specifically, check the result of tempnam()
(temp_file) and throw/abort if false, check copy($file_path, $temp_file)
returned true and handle/fail on error, call processRun(...) as before, use try
{ ... $content = file_get_contents($temp_file); } finally { if
(isset($temp_file) && file_exists($temp_file)) unlink($temp_file); } so unlink
always runs, and handle file_get_contents returning false appropriately in
runPhpcbf().
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 211-214: The test uses brittle exact string assertions in
LocalVariableNamingSniffFunctionalTest (assertStringContainsString against
$fixed) — replace the exact check on the `@var` line (currently checking for
'@var \Drupal\Core\Extension\ModuleHandler $module_handler */') with a regex
assertion that validates the renamed variable reference (allowing an optional
leading backslash and any namespace) and the new variable name ($moduleHandler);
for example swap the assertStringContainsString call for a PHPUnit regex
assertion (assertMatchesRegularExpression or assertRegExp depending on the test
suite) against $fixed using a pattern like
'/@var\s+\\?\S*ModuleHandler\s+\$moduleHandler\b/' so the test accepts
namespaced or non-namespaced `ModuleHandler` and the renamed variable.
---
Duplicate comments:
In `@src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.php`:
- Around line 188-207: The current multi-variable skip uses an aggregated
$all_text across $commentTokens which can wrongfully block a rewrite when only
an adjacent comment contains another variable; change the logic to scope the
check per token: inside the loop over $commentTokens (using $ptr and
$tokens[$ptr]['content']) run the variable extraction
(preg_match_all('/\$[a-zA-Z_]\\w*/', ...)) and compute array_unique on that
single token's matches, and if count($unique_vars) > 1 for that token then skip
rewriting that token only; otherwise allow the rewrite for that token (remove
the global $all_text aggregation and use per-token $matches/$unique_vars
checks).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f85e9d82-841b-4db4-ae9a-9ed13c267e00
📒 Files selected for processing (6)
src/DrevOps/Sniffs/NamingConventions/LocalVariableNamingSniff.phptests/Fixtures/CommentVariableNaming.phptests/Fixtures/VariableNaming.phptests/Functional/FunctionalTestCase.phptests/Functional/LocalVariableNamingSniffFunctionalTest.phptests/Unit/LocalVariableNamingSniffTest.php
Comment on lines
+147
to
+158
| $temp_file = tempnam(sys_get_temp_dir(), 'phpcbf_test_'); | ||
| copy($file_path, $temp_file); | ||
|
|
||
| $this->processRun( | ||
| $phpcbf_bin, | ||
| ['--standard=DrevOps', '--sniffs=' . $sniff, $temp_file], | ||
| timeout: 120 | ||
| ); | ||
|
|
||
| $content = file_get_contents($temp_file); | ||
| unlink($temp_file); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Harden temp-file handling in runPhpcbf() to avoid leaks and silent IO failures.
Guard tempnam()/copy() and move unlink() into finally so cleanup still happens on assertion/process failures.
♻️ Proposed refactor
- $temp_file = tempnam(sys_get_temp_dir(), 'phpcbf_test_');
- copy($file_path, $temp_file);
-
- $this->processRun(
- $phpcbf_bin,
- ['--standard=DrevOps', '--sniffs=' . $sniff, $temp_file],
- timeout: 120
- );
-
- $content = file_get_contents($temp_file);
- unlink($temp_file);
+ $temp_file = tempnam(sys_get_temp_dir(), 'phpcbf_test_');
+ $this->assertNotFalse($temp_file, 'Failed to create temp file');
+ $this->assertTrue(copy($file_path, $temp_file), 'Failed to copy fixture to temp file');
+
+ try {
+ $this->processRun(
+ $phpcbf_bin,
+ ['--standard=DrevOps', '--sniffs=' . $sniff, $temp_file],
+ timeout: 120
+ );
+
+ $content = file_get_contents($temp_file);
+ }
+ finally {
+ if (is_file($temp_file)) {
+ unlink($temp_file);
+ }
+ }🧰 Tools
🪛 PHPMD (2.15.0)
[error] 147-147: The variable $temp_file is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Functional/FunctionalTestCase.php` around lines 147 - 158, The
temp-file handling in runPhpcbf() is fragile: verify tempnam() succeeded and
that copy() returned true before calling processRun, read contents only after
confirming file exists, and always remove the temp file in a finally block to
avoid leaks; specifically, check the result of tempnam() (temp_file) and
throw/abort if false, check copy($file_path, $temp_file) returned true and
handle/fail on error, call processRun(...) as before, use try { ... $content =
file_get_contents($temp_file); } finally { if (isset($temp_file) &&
file_exists($temp_file)) unlink($temp_file); } so unlink always runs, and handle
file_get_contents returning false appropriately in runPhpcbf().
Comment on lines
+211
to
+214
| $this->assertStringContainsString('@var \SomeClass $module_handler */', $fixed); | ||
| $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler */', $fixed); | ||
| $this->assertStringContainsString('@var Drupal\Core\Extension\ModuleHandler $module_handler */', $fixed); | ||
| $this->assertStringContainsString('@var \Very\Long\Namespace\Path\To\SomeClass $module_handler */', $fixed); |
There was a problem hiding this comment.
Fix the brittle assertion causing the current CI failure (Line 213).
The check is too exact for @var text shape. Assert on the renamed variable reference with a regex (optionally allowing a leading \) so the test validates the intended behavior reliably.
💡 Proposed fix
- $this->assertStringContainsString('@var Drupal\Core\Extension\ModuleHandler $module_handler */', $fixed);
+ $this->assertMatchesRegularExpression(
+ '/@var\s+\\\\?Drupal\\\\Core\\\\Extension\\\\ModuleHandler\s+\$module_handler\b/',
+ $fixed
+ );🧰 Tools
🪛 GitHub Actions: Test PHP
[error] 213-213: PHPUnit failure: testPhpcbfFixesCommentVariableReferences failed. Script 'phpunit' exited with code 1 during test-coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php` around lines 211
- 214, The test uses brittle exact string assertions in
LocalVariableNamingSniffFunctionalTest (assertStringContainsString against
$fixed) — replace the exact check on the `@var` line (currently checking for
'@var \Drupal\Core\Extension\ModuleHandler $module_handler */') with a regex
assertion that validates the renamed variable reference (allowing an optional
leading backslash and any namespace) and the new variable name ($moduleHandler);
for example swap the assertStringContainsString call for a PHPUnit regex
assertion (assertMatchesRegularExpression or assertRegExp depending on the test
suite) against $fixed using a pattern like
'/@var\s+\\?\S*ModuleHandler\s+\$moduleHandler\b/' so the test accepts
namespaced or non-namespaced `ModuleHandler` and the renamed variable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php (1)
211-213:⚠️ Potential issue | 🔴 CriticalReplace brittle exact
@varassertions with regex matching.These exact full-line string checks are fragile and are already causing CI instability; assert the semantic contract (type + renamed variable) instead.
💡 Proposed fix
- $this->assertStringContainsString('@var \SomeClass $module_handler */', $fixed); - $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler */', $fixed); - $this->assertStringContainsString('@var \Very\Long\Namespace\Path\To\SomeClass $module_handler */', $fixed); + $this->assertMatchesRegularExpression( + '/@var\s+\\\\?SomeClass\s+\$module_handler\b/', + $fixed + ); + $this->assertMatchesRegularExpression( + '/@var\s+\\\\?Drupal\\\\Core\\\\Extension\\\\ModuleHandler\s+\$module_handler\b/', + $fixed + ); + $this->assertMatchesRegularExpression( + '/@var\s+\\\\?Very\\\\Long\\\\Namespace\\\\Path\\\\To\\\\SomeClass\s+\$module_handler\b/', + $fixed + );#!/bin/bash set -euo pipefail test_file="$(fd -p 'LocalVariableNamingSniffFunctionalTest.php' tests | head -n1)" nl -ba "$test_file" | sed -n '204,222p' # Expectation after fix: no exact `@var` string assertions for $module_handler remain. rg -n "assertStringContainsString\('@var .*\\$module_handler" "$test_file" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php` around lines 211 - 213, The three brittle exact-string assertions in LocalVariableNamingSniffFunctionalTest that call assertStringContainsString (checking '@var \SomeClass $module_handler */', '@var \Drupal\Core\Extension\ModuleHandler $module_handler */', and '@var \Very\Long\Namespace\Path\To\SomeClass $module_handler */') should be replaced with assertions that use a regex to validate "type + renamed variable" semantics; update the tests to use PHPUnit's assertMatchesRegularExpression (or assertRegExp) against $fixed to match a pattern like /^@var\s+[\w\\]+\s+\$module_handler\b/ (or equivalent) so the test checks the type token and $module_handler rather than exact full-line strings, and remove the three exact assertStringContainsString calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 205-242: The local variable $fixed in the test method violates the
snake_case rule; rename it to a snake_case identifier (e.g., $fixed_content)
where it is assigned from runPhpcbf and update every subsequent reference in the
method (all assertions and str_contains calls) to use the new name; ensure the
symbol to change is the variable assigned from runPhpcbf in
LocalVariableNamingSniffFunctionalTest (the $fixed variable) and update only its
usages within that test method so behavior remains identical.
---
Duplicate comments:
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 211-213: The three brittle exact-string assertions in
LocalVariableNamingSniffFunctionalTest that call assertStringContainsString
(checking '@var \SomeClass $module_handler */', '@var
\Drupal\Core\Extension\ModuleHandler $module_handler */', and '@var
\Very\Long\Namespace\Path\To\SomeClass $module_handler */') should be replaced
with assertions that use a regex to validate "type + renamed variable"
semantics; update the tests to use PHPUnit's assertMatchesRegularExpression (or
assertRegExp) against $fixed to match a pattern like
/^@var\s+[\w\\]+\s+\$module_handler\b/ (or equivalent) so the test checks the
type token and $module_handler rather than exact full-line strings, and remove
the three exact assertStringContainsString calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47263a86-13a7-4a99-9214-087d521a23f9
📒 Files selected for processing (1)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php
Comment on lines
+205
to
+242
| $fixed = $this->runPhpcbf( | ||
| static::$fixtures . DIRECTORY_SEPARATOR . 'CommentVariableNaming.php', | ||
| 'DrevOps.NamingConventions.LocalVariableNaming' | ||
| ); | ||
|
|
||
| // Doc comments should be fixed. | ||
| $this->assertStringContainsString('@var \SomeClass $module_handler */', $fixed); | ||
| $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler */', $fixed); | ||
| $this->assertStringContainsString('@var \Very\Long\Namespace\Path\To\SomeClass $module_handler */', $fixed); | ||
|
|
||
| // FQN without leading backslash: PHPCS 3 tokenizes doc comment strings | ||
| // differently, so the fix may not apply. Assert either form is present. | ||
| $this->assertTrue( | ||
| str_contains($fixed, '@var Drupal\Core\Extension\ModuleHandler $module_handler */') | ||
| || str_contains($fixed, '@var Drupal\Core\Extension\ModuleHandler $moduleHandler */'), | ||
| 'FQN without leading backslash: variable should be fixed or original preserved.' | ||
| ); | ||
|
|
||
| // Multi-line doc comment should be fixed. | ||
| $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler', $fixed); | ||
|
|
||
| // Mixed: doc comment var should be fixed. | ||
| $this->assertStringContainsString('@var \SomeClass $first_handler */', $fixed); | ||
|
|
||
| // Inline comments should NOT be fixed (default fixCommentTypes = 'doc'). | ||
| $this->assertStringContainsString('// $moduleHandler holds the module handler.', $fixed); | ||
| $this->assertStringContainsString('/* $moduleHandler holds the module handler. */', $fixed); | ||
| $this->assertStringContainsString('// $secondHandler holds the second handler.', $fixed); | ||
|
|
||
| // Multi-variable comment should NOT be fixed. | ||
| $this->assertStringContainsString('@var \SomeClass $moduleHandler See also $otherService */', $fixed); | ||
|
|
||
| // Code between comment and variable: comment should NOT be fixed. | ||
| $this->assertStringContainsString('@var \SomeClass $moduleHandler */', $fixed); | ||
|
|
||
| // Trailing comment on previous statement should NOT be fixed. | ||
| $this->assertStringContainsString('// $moduleHandler trailing.', $fixed); | ||
| } |
There was a problem hiding this comment.
Use snake_case for the local fixed-content variable.
$fixed violates the repository naming rule for local variables in PHP files.
♻️ Proposed rename
- $fixed = $this->runPhpcbf(
+ $fixed_output = $this->runPhpcbf(
static::$fixtures . DIRECTORY_SEPARATOR . 'CommentVariableNaming.php',
'DrevOps.NamingConventions.LocalVariableNaming'
);
@@
- $this->assertStringContainsString('@var \SomeClass $module_handler */', $fixed);
+ $this->assertStringContainsString('@var \SomeClass $module_handler */', $fixed_output);
@@
- str_contains($fixed, '@var Drupal\Core\Extension\ModuleHandler $module_handler */')
- || str_contains($fixed, '@var Drupal\Core\Extension\ModuleHandler $moduleHandler */'),
+ str_contains($fixed_output, '@var Drupal\Core\Extension\ModuleHandler $module_handler */')
+ || str_contains($fixed_output, '@var Drupal\Core\Extension\ModuleHandler $moduleHandler */'),
@@
- $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler', $fixed);
+ $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler', $fixed_output);🧰 Tools
🪛 GitHub Actions: Test PHP
[error] 213-213: PHPUnit test 'testPhpcbfFixesCommentVariableReferences' failed: assertion did not pass for LocalVariableNamingSniffFunctionalTest. The generated fixture output contains a long FQN in a @var docblock ("@var \Very\Long\Namespace\Path\To\SomeClass $module_handler */"), causing the mismatch in the expected versus actual code after phpcbf autofix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php` around lines 205
- 242, The local variable $fixed in the test method violates the snake_case
rule; rename it to a snake_case identifier (e.g., $fixed_content) where it is
assigned from runPhpcbf and update every subsequent reference in the method (all
assertions and str_contains calls) to use the new name; ensure the symbol to
change is the variable assigned from runPhpcbf in
LocalVariableNamingSniffFunctionalTest (the $fixed variable) and update only its
usages within that test method so behavior remains identical.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php (1)
216-218:⚠️ Potential issue | 🟠 MajorPipeline failure: Assertion on line 218 is too brittle for cross-version compatibility.
The CI failure indicates the exact string match fails because PHPCS versions may produce different FQN formatting in the output. Since PHPCS 3 tokenization can vary (as noted in the commit message and line 210-211 comments), consider using a more flexible assertion.
🛠️ Proposed fix using regex assertion
// Doc comments should be fixed. $this->assertStringContainsString('@var \SomeClass $module_handler */', $fixed); $this->assertStringContainsString('@var \Drupal\Core\Extension\ModuleHandler $module_handler */', $fixed); - $this->assertStringContainsString('@var \Very\Long\Namespace\Path\To\SomeClass $module_handler */', $fixed); + $this->assertMatchesRegularExpression( + '/@var\s+\\\\?Very\\\\Long\\\\Namespace\\\\Path\\\\To\\\\SomeClass\s+\$module_handler\b/', + $fixed + );Alternatively, if the FQN output is inconsistent across PHPCS versions, consider checking only the essential part (the renamed variable):
$this->assertMatchesRegularExpression( '/@var\s+.*SomeClass\s+\$module_handler\s/', $fixed );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php` around lines 216 - 218, Replace the brittle exact-FQN assertion that uses $this->assertStringContainsString with a regex-based assertion to tolerate PHPCS FQN formatting differences: update the check against $fixed to use $this->assertMatchesRegularExpression('/@var\s+.*SomeClass\s+\$module_handler\s/', $fixed) (or a similar pattern) so the test only verifies the essential renamed variable and class token (SomeClass and $module_handler) rather than an exact fully-qualified namespace string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 210-212: Add a brief note about which PHPCS versions support
auto-fix for this sniff so future maintainers understand why the test condition
around $fixes_applied uses str_contains($fixed, '$module_handler =
get_service('). Add this documentation either as a one-line comment immediately
above the $fixes_applied check in LocalVariableNamingSniffFunctionalTest (to
explain the test behavior) and/or in the docblock of the
LocalVariableNamingSniff class (to document the sniff capability), stating the
minimum PHPCS version that provides auto-fix for this sniff.
---
Duplicate comments:
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 216-218: Replace the brittle exact-FQN assertion that uses
$this->assertStringContainsString with a regex-based assertion to tolerate PHPCS
FQN formatting differences: update the check against $fixed to use
$this->assertMatchesRegularExpression('/@var\s+.*SomeClass\s+\$module_handler\s/',
$fixed) (or a similar pattern) so the test only verifies the essential renamed
variable and class token (SomeClass and $module_handler) rather than an exact
fully-qualified namespace string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 236f8836-6e75-4a61-bfd8-cbca86a5f67c
📒 Files selected for processing (1)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php (1)
205-208: 🛠️ Refactor suggestion | 🟠 MajorUse snake_case for the
$fixedvariable.The local variable
$fixedshould follow the snake_case convention. Consider renaming to$fixed_outputor$fixed_content.♻️ Proposed rename
- $fixed = $this->runPhpcbf( + $fixed_output = $this->runPhpcbf( static::$fixtures . DIRECTORY_SEPARATOR . 'CommentVariableNaming.php', 'DrevOps.NamingConventions.LocalVariableNaming' );Then update all references to
$fixedin the method to use$fixed_output.As per coding guidelines:
**/*.php: Use snake_case for local variables and method parameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php` around lines 205 - 208, The local variable $fixed in LocalVariableNamingSniffFunctionalTest should use snake_case; rename $fixed to $fixed_output and update every reference in this test method (including the call to $this->runPhpcbf(...) and any subsequent assertions or uses) to $fixed_output so the variable name follows the project's snake_case convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/Functional/LocalVariableNamingSniffFunctionalTest.php`:
- Around line 205-208: The local variable $fixed in
LocalVariableNamingSniffFunctionalTest should use snake_case; rename $fixed to
$fixed_output and update every reference in this test method (including the call
to $this->runPhpcbf(...) and any subsequent assertions or uses) to $fixed_output
so the variable name follows the project's snake_case convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95562606-b008-4df8-bcb1-5138cbe401e9
📒 Files selected for processing (1)
tests/Functional/LocalVariableNamingSniffFunctionalTest.php
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.
Closes #17
Summary by CodeRabbit
New Features
Tests