Try inline annotations for first()/firstOrFail() on magic finders. #391
Conversation
WalkthroughAdded a new annotator task that parses PHP source in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Developer/Tests
participant Task as TableFindAnnotatorTask
participant Parser as nikic/php-parser
participant Visitor as AST Visitor
participant Printer as FormatPreservingPrinter
Caller->>Task: annotate(path)
Task->>Parser: parse(file content)
Parser-->>Task: AST
Task->>Visitor: traverse(AST)
Visitor->>Visitor: find assignments with ->first()/->firstOrFail()
Visitor->>Visitor: resolve $this->TableName -> Entity class, determine nullable
Visitor-->>Task: modified AST nodes with PHPDoc
Task->>Printer: prettyPrint(modified AST)
Printer-->>Task: annotated PHP code
Task-->>Caller: output annotated code + summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 8
🧹 Nitpick comments (1)
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php (1)
98-110: Remove or utilize unused methodThe
buildUsesAnnotationsmethod is defined but never called. Either remove it or update the implementation to use it if @uses annotations are intended.If this method is not needed for the current implementation, consider removing it to reduce code complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php(1 hunks)tests/TestCase/Annotator/ClassAnnotatorTask/TableFindAnnotatorTaskTest.php(1 hunks)tests/test_files/ClassAnnotation/TableFind/before.php(1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
tests/test_files/ClassAnnotation/TableFind/before.php
14-14: Avoid unused local variables such as '$resident'. (Unused Code Rules)
(UnusedLocalVariable)
21-21: Avoid unused local variables such as '$residentOther'. (Unused Code Rules)
(UnusedLocalVariable)
28-28: Avoid unused local variables such as '$residentX'. (Unused Code Rules)
(UnusedLocalVariable)
35-35: Avoid unused local variables such as '$residentY'. (Unused Code Rules)
(UnusedLocalVariable)
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php
28-28: Avoid unused parameters such as '$content'. (Unused Code Rules)
(UnusedFormalParameter)
95-95: Avoid unused local variables such as '$annotations'. (Unused Code Rules)
(UnusedLocalVariable)
95-95: Avoid unused local variables such as '$rowToAnnotate'. (Unused Code Rules)
(UnusedLocalVariable)
🪛 GitHub Check: Coding Standard & Static Analysis
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php
[failure] 95-95:
Undefined variable: $rowToAnnotate
[failure] 95-95:
Undefined variable: $annotations
[failure] 89-89:
Parameter #1 $nodes of method PhpParser\NodeTraverser::traverse() expects array<PhpParser\Node>, array<PhpParser\Node\Stmt>|null given.
[failure] 76-76:
Access to an undefined property PhpParser\Node\Expr::$name.
[failure] 52-52:
Return type (void) of method PhpParser\NodeVisitorAbstract@anonymous/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php:51::enterNode() should be compatible with return type (array<PhpParser\Node>|int|PhpParser\Node|null) of method PhpParser\NodeVisitorAbstract::enterNode()
[failure] 52-52:
Return type (void) of method PhpParser\NodeVisitorAbstract@anonymous/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php:51::enterNode() should be compatible with return type (array<PhpParser\Node>|int|PhpParser\Node|null) of method PhpParser\NodeVisitor::enterNode()
🪛 GitHub Actions: CI
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php
[error] 51-51: PHPStan error: Return type (void) of method enterNode() should be compatible with return type (array<PhpParser\Node>|int|PhpParser\Node|null) of method PhpParser\NodeVisitor::enterNode()
🔇 Additional comments (1)
composer.json (1)
38-38: php-parser dependency verified and approvedThe
nikic/php-parseraddition at^5.5aligns with the latest stable release (5.5.0) and has no known security advisories. This dependency is safe to merge.
tests/TestCase/Annotator/ClassAnnotatorTask/TableFindAnnotatorTaskTest.php
Outdated
Show resolved
Hide resolved
| $this->assertTrue($result); | ||
|
|
||
| $content = $task->getContent(); | ||
| dd($content); |
There was a problem hiding this comment.
Remove debug statement
The dd() function call will halt test execution and should be removed.
- dd($content);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dd($content); |
🤖 Prompt for AI Agents
In tests/TestCase/Annotator/ClassAnnotatorTask/TableFindAnnotatorTaskTest.php at
line 56, remove the debug statement `dd($content);` because it stops the test
execution. Simply delete this line to allow the test to run normally without
interruption.
| dd($modifiedCode); | ||
|
|
||
| return $this->annotateInlineContent($path, $this->content, $annotations, $rowToAnnotate); | ||
| } |
There was a problem hiding this comment.
Remove debug code and fix undefined variables
The method contains debug code and references undefined variables.
-dd($modifiedCode);
+$this->content = $modifiedCode;
-return $this->annotateInlineContent($path, $this->content, $annotations, $rowToAnnotate);
+return true;🧰 Tools
🪛 GitHub Check: Coding Standard & Static Analysis
[failure] 95-95:
Undefined variable: $rowToAnnotate
[failure] 95-95:
Undefined variable: $annotations
🪛 PHPMD (2.15.0)
95-95: Avoid unused local variables such as '$annotations'. (Unused Code Rules)
(UnusedLocalVariable)
95-95: Avoid unused local variables such as '$rowToAnnotate'. (Unused Code Rules)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php around lines 93
to 96, remove the debug statement 'dd($modifiedCode);' and ensure that any
variables used in this section are properly defined before use. Clean up the
code by deleting debug code and fixing or removing references to undefined
variables to prevent runtime errors.
| $modifiedAst = $traverser->traverse($ast); | ||
| $prettyPrinter = new PrettyPrinter(); | ||
| $modifiedCode = $prettyPrinter->prettyPrintFile($modifiedAst); |
There was a problem hiding this comment.
Fix AST traversal null check and PrettyPrinter instantiation
The AST might be null and needs checking. Also fix PrettyPrinter instantiation.
+if ($ast === null) {
+ return false;
+}
$modifiedAst = $traverser->traverse($ast);
-$prettyPrinter = new PrettyPrinter();
+$prettyPrinter = new PrettyPrinter();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $modifiedAst = $traverser->traverse($ast); | |
| $prettyPrinter = new PrettyPrinter(); | |
| $modifiedCode = $prettyPrinter->prettyPrintFile($modifiedAst); | |
| if ($ast === null) { | |
| return false; | |
| } | |
| $modifiedAst = $traverser->traverse($ast); | |
| $prettyPrinter = new PrettyPrinter(); | |
| $modifiedCode = $prettyPrinter->prettyPrintFile($modifiedAst); |
🧰 Tools
🪛 GitHub Check: Coding Standard & Static Analysis
[failure] 89-89:
Parameter #1 $nodes of method PhpParser\NodeTraverser::traverse() expects array<PhpParser\Node>, array<PhpParser\Node\Stmt>|null given.
🤖 Prompt for AI Agents
In src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php around lines 89
to 91, add a null check for the $modifiedAst result from
$traverser->traverse($ast) before proceeding. Also, correct the instantiation of
PrettyPrinter by using the fully qualified class name or importing it properly
to avoid errors. Ensure that the code only calls prettyPrintFile if $modifiedAst
is not null.
|
I can write the findings to an array and reuse that then: I think with this we could build it as dual solution with minimal error (false positive) rate. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_files/ClassAnnotation/TableFind/before.php (1)
3-3: Past review comment is now obsolete.The namespace is correctly spelled as "App\Controller" - the typo mentioned in previous reviews appears to have been fixed.
🧹 Nitpick comments (1)
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php (1)
29-35: Unused parameter should be removed or documented.The
$contentparameter is not used in this method and should be removed from the signature, or if it's required by the interface, add a comment explaining why it's unused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php(1 hunks)tests/TestCase/Annotator/ClassAnnotatorTask/TableFindAnnotatorTaskTest.php(1 hunks)tests/test_files/ClassAnnotation/TableFind/before.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/TestCase/Annotator/ClassAnnotatorTask/TableFindAnnotatorTaskTest.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php
29-29: Avoid unused parameters such as '$content'. (Unused Code Rules)
(UnusedFormalParameter)
41-41: Avoid unused parameters such as '$path'. (Unused Code Rules)
(UnusedFormalParameter)
tests/test_files/ClassAnnotation/TableFind/before.php
14-14: Avoid unused local variables such as '$resident'. (Unused Code Rules)
(UnusedLocalVariable)
21-21: Avoid unused local variables such as '$residentOther'. (Unused Code Rules)
(UnusedLocalVariable)
28-28: Avoid unused local variables such as '$residentX'. (Unused Code Rules)
(UnusedLocalVariable)
35-35: Avoid unused local variables such as '$residentY'. (Unused Code Rules)
(UnusedLocalVariable)
🪛 GitHub Check: Coding Standard & Static Analysis
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php
[failure] 130-130:
Method IdeHelper\Annotator\ClassAnnotatorTask\TableFindAnnotatorTask::annotate() should return bool but return statement is missing.
[failure] 123-123:
Parameter #2 $origStmts of method PhpParser\PrettyPrinterAbstract::printFormatPreserving() expects array<PhpParser\Node>, array<PhpParser\Node\Stmt>|null given.
[failure] 119-119:
Parameter #1 $nodes of method PhpParser\NodeTraverser::traverse() expects array<PhpParser\Node>, array<PhpParser\Node\Stmt>|null given.
[failure] 62-62:
Method PhpParser\NodeVisitorAbstract@anonymous/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php:56::__construct() has parameter $array with no value type specified in iterable type array.
[failure] 57-57:
Property PhpParser\NodeVisitorAbstract@anonymous/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php:56::$array type has no value type specified in iterable type array.
[failure] 57-57:
Property PhpParser\NodeVisitorAbstract@anonymous/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php:56::$array is never read, only written.
🪛 GitHub Actions: CI
src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php
[error] 127-128: PHPUnit test failure indicated by debug output around lines 127-128. Process completed with exit code 1.
🔇 Additional comments (4)
tests/test_files/ClassAnnotation/TableFind/before.php (1)
12-41: Test fixture is correctly structured.The unused variables flagged by static analysis are expected in a test fixture file. This file serves as input for testing the
TableFindAnnotatorTaskand demonstrates the various patterns the annotator should detect and annotate.src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php (3)
70-116: Anonymous class implementation looks correct.The core logic for detecting and annotating table find operations is well implemented. The use of proper inflection and type annotation generation is good.
98-101: Entity class generation is improved.Good use of CakePHP's
Inflector::singularize()instead of crude string manipulation. The nullable type handling forfirst()vsfirstOrFail()is correct.
119-126: Add null check for traversal result.The traversal result should be checked for null before passing to the pretty printer to prevent type errors.
$modifiedAst = $traverser->traverse($ast); +if ($modifiedAst === null) { + return false; +} $printer = new Standard(); $modifiedCode = $printer->printFormatPreserving( $modifiedAst, $originalAst, $tokens, );Likely an incorrect or invalid review comment.
| public function annotate(string $path): bool { | ||
| $parser = (new ParserFactory())->createForHostVersion(); | ||
| try { | ||
| $ast = $parser->parse($this->content); | ||
| $tokens = $parser->getTokens(); | ||
| $originalAst = $ast; | ||
| } catch (Throwable $e) { | ||
| trigger_error($e); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Add null check for AST parsing result.
The AST parsing result should be checked for null before proceeding with traversal to prevent type errors.
public function annotate(string $path): bool {
$parser = (new ParserFactory())->createForHostVersion();
try {
$ast = $parser->parse($this->content);
+ if ($ast === null) {
+ return false;
+ }
$tokens = $parser->getTokens();
$originalAst = $ast;
} catch (Throwable $e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function annotate(string $path): bool { | |
| $parser = (new ParserFactory())->createForHostVersion(); | |
| try { | |
| $ast = $parser->parse($this->content); | |
| $tokens = $parser->getTokens(); | |
| $originalAst = $ast; | |
| } catch (Throwable $e) { | |
| trigger_error($e); | |
| return false; | |
| } | |
| public function annotate(string $path): bool { | |
| $parser = (new ParserFactory())->createForHostVersion(); | |
| try { | |
| $ast = $parser->parse($this->content); | |
| if ($ast === null) { | |
| return false; | |
| } | |
| $tokens = $parser->getTokens(); | |
| $originalAst = $ast; | |
| } catch (Throwable $e) { | |
| trigger_error($e); | |
| return false; | |
| } | |
| // …rest of method… | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
41-41: Avoid unused parameters such as '$path'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php around lines 41
to 51, the result of the AST parsing ($ast) is not checked for null before use,
which can cause type errors. Add a null check after parsing the AST; if $ast is
null, handle it appropriately by returning false or triggering an error before
continuing with traversal.
| debug($array); | ||
| dd($modifiedCode); | ||
|
|
||
| //return $this->annotateInlineContent($path, $this->content, $annotations, $rowToAnnotate); | ||
| } |
There was a problem hiding this comment.
Remove debug code and add proper return statement.
The debug code is causing test failures and the method is missing a return statement.
-debug($array);
-dd($modifiedCode);
+$this->content = $modifiedCode;
-//return $this->annotateInlineContent($path, $this->content, $annotations, $rowToAnnotate);
+return count($array) > 0;🧰 Tools
🪛 GitHub Check: Coding Standard & Static Analysis
[failure] 130-130:
Method IdeHelper\Annotator\ClassAnnotatorTask\TableFindAnnotatorTask::annotate() should return bool but return statement is missing.
🪛 GitHub Actions: CI
[error] 127-128: PHPUnit test failure indicated by debug output around lines 127-128. Process completed with exit code 1.
🤖 Prompt for AI Agents
In src/Annotator/ClassAnnotatorTask/TableFindAnnotatorTask.php between lines 127
and 131, remove the debug statements debug($array) and dd($modifiedCode) as they
cause test failures. Replace them with the appropriate return statement that was
commented out, specifically return $this->annotateInlineContent($path,
$this->content, $annotations, $rowToAnnotate), to ensure the method returns the
expected result.
dereuromark
changed the title
dereuromark
changed the title
Removed dealerdirect/phpcodesniffer-composer-installer from allowed plugins.
1 participant
Stab at trying to make more things without manual help
But all I get is this:
So it finds them, but cannot print out the comments it adds, also it formats it too aggressively
Should we try a mixed approach?
Using Parser to find? And then the existing token based system to add the comment?
The parser currently finds it correctly.
And when adding the comment, it also knows the startLine and token:
Summary by CodeRabbit
New Features
Tests
Chores