-
-
Notifications
You must be signed in to change notification settings - Fork 39
Try inline annotations for first()/firstOrFail() on magic finders. #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| <?php | ||
|
|
||
| namespace IdeHelper\Annotator\ClassAnnotatorTask; | ||
|
|
||
| use Cake\Utility\Inflector; | ||
| use IdeHelper\Annotation\AnnotationFactory; | ||
| use IdeHelper\Annotation\UsesAnnotation; | ||
| use PhpParser\Comment\Doc; | ||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\Assign; | ||
| use PhpParser\NodeTraverser; | ||
| use PhpParser\NodeVisitorAbstract; | ||
| use PhpParser\ParserFactory; | ||
| use PhpParser\PrettyPrinter\Standard; | ||
| use Throwable; | ||
|
|
||
| /** | ||
| * Usage of find() or findOrFail() should have inline annotations added. | ||
| */ | ||
| class TableFindAnnotatorTask extends AbstractClassAnnotatorTask implements ClassAnnotatorTaskInterface { | ||
|
|
||
| /** | ||
| * Deprecated: $content, use $this->content instead. | ||
| * | ||
| * @param string $path | ||
| * @param string $content | ||
| * @return bool | ||
| */ | ||
| public function shouldRun(string $path, string $content): bool { | ||
| if (!str_contains($path, DS . 'src' . DS)) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * @param string $path | ||
| * @return bool | ||
| */ | ||
| 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; | ||
| } | ||
|
|
||
| $array = []; | ||
|
|
||
| $traverser = new NodeTraverser(); | ||
| $traverser->addVisitor(new class($array) extends NodeVisitorAbstract { | ||
| private array $array; | ||
|
|
||
| /** | ||
| * @param array $array | ||
| */ | ||
| public function __construct(array &$array) { | ||
| $this->array = &$array; | ||
| } | ||
|
|
||
| /** | ||
| * @param \PhpParser\Node $node | ||
| * @return \PhpParser\Node|null | ||
| */ | ||
| public function enterNode(Node $node): ?Node { | ||
| if ($node instanceof Assign && $node->expr instanceof Node\Expr\MethodCall) { | ||
| $methodCall = $node->expr; | ||
|
|
||
| // Check for ->first() or ->firstOrFail() | ||
| if ( | ||
| $methodCall->name instanceof Node\Identifier && | ||
| in_array($methodCall->name->toString(), ['first', 'firstOrFail']) | ||
| ) { | ||
| $method = $methodCall->name->toString(); | ||
|
|
||
| // Traverse back to $this->TableName->find() | ||
| $callChain = $methodCall->var; | ||
| while ($callChain instanceof Node\Expr\MethodCall) { | ||
| $callChain = $callChain->var; | ||
| } | ||
|
|
||
| if ( | ||
| $callChain instanceof Node\Expr\PropertyFetch && | ||
| $callChain->var instanceof Node\Expr\Variable && | ||
| $callChain->var->name === 'this' && | ||
| $callChain->name instanceof Node\Identifier | ||
| ) { | ||
| $tableName = $callChain->name->toString(); // e.g., "Residents" | ||
| $varName = ($node->var instanceof Node\Expr\Variable && is_string($node->var->name)) | ||
| ? $node->var->name | ||
| : 'unknown'; | ||
|
|
||
| $entityName = Inflector::singularize($tableName); | ||
| $entityClass = '\\App\\Model\\Entity\\' . $entityName; | ||
| $nullable = $method === 'first' ? '|null' : ''; | ||
| $doc = new Doc("/** @var {$entityClass}{$nullable} \${$varName} */"); | ||
| $node->setDocComment($doc); | ||
|
|
||
| $this->array[] = [ | ||
| 'line' => $node->getStartLine(), | ||
| 'content' => $doc->getText(), | ||
| 'varName' => $varName, | ||
| 'tableName' => $tableName, | ||
| 'entityName' => $entityName, | ||
| ]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| }); | ||
|
|
||
| $modifiedAst = $traverser->traverse($ast); | ||
| $printer = new Standard(); | ||
| $modifiedCode = $printer->printFormatPreserving( | ||
| $modifiedAst, | ||
| $originalAst, | ||
| $tokens, | ||
| ); | ||
|
|
||
| debug($array); | ||
| dd($modifiedCode); | ||
|
|
||
| //return $this->annotateInlineContent($path, $this->content, $annotations, $rowToAnnotate); | ||
| } | ||
|
Comment on lines
+127
to
+131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 🪛 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 |
||
|
|
||
| /** | ||
| * @param array<string> $classes | ||
| * @return array<\IdeHelper\Annotation\AbstractAnnotation> | ||
| */ | ||
| protected function buildUsesAnnotations(array $classes): array { | ||
| $annotations = []; | ||
|
|
||
| foreach ($classes as $className) { | ||
| $annotations[] = AnnotationFactory::createOrFail(UsesAnnotation::TAG, '\\' . $className); | ||
| } | ||
|
|
||
| return $annotations; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||
| <?php | ||||
|
|
||||
| namespace IdeHelper\Test\TestCase\Annotator\ClassAnnotatorTask; | ||||
|
|
||||
| use Cake\Console\ConsoleIo; | ||||
| use IdeHelper\Annotator\AbstractAnnotator; | ||||
| use IdeHelper\Annotator\ClassAnnotatorTask\TableFindAnnotatorTask; | ||||
| use IdeHelper\Console\Io; | ||||
| use Shim\TestSuite\ConsoleOutput; | ||||
| use Shim\TestSuite\TestCase; | ||||
|
|
||||
| class TableFindAnnotatorTaskTest extends TestCase { | ||||
|
|
||||
| protected ConsoleOutput $out; | ||||
|
|
||||
| protected ConsoleOutput $err; | ||||
|
|
||||
| protected ?Io $io = null; | ||||
|
|
||||
| /** | ||||
| * @return void | ||||
| */ | ||||
| protected function setUp(): void { | ||||
| parent::setUp(); | ||||
|
|
||||
| $this->out = new ConsoleOutput(); | ||||
| $this->err = new ConsoleOutput(); | ||||
| $consoleIo = new ConsoleIo($this->out, $this->err); | ||||
| $this->io = new Io($consoleIo); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * @return void | ||||
| */ | ||||
| public function testShouldRun() { | ||||
| $task = $this->getTask(''); | ||||
|
|
||||
| $result = $task->shouldRun('/src/Foo.php', ''); | ||||
| $this->assertTrue($result); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * @return void | ||||
| */ | ||||
| public function testAnnotate() { | ||||
| $content = file_get_contents(TEST_FILES . 'ClassAnnotation/TableFind/before.php'); | ||||
| $task = $this->getTask($content); | ||||
| $path = '/src/Controller/TestMeController.php'; | ||||
|
|
||||
| $result = $task->annotate($path); | ||||
| $this->assertTrue($result); | ||||
|
|
||||
| $content = $task->getContent(); | ||||
| dd($content); | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove debug statement The - dd($content);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||
| $this->assertTextContains('/** @var \App\Model\Entity\Resident $resident */', $content); | ||||
|
|
||||
| $output = $this->out->output(); | ||||
| $this->assertTextContains(' -> 1 annotation added.', $output); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * @param string $content | ||||
| * @param array $params | ||||
| * | ||||
| * @return \IdeHelper\Annotator\ClassAnnotatorTask\TableFindAnnotatorTask | ||||
| */ | ||||
| protected function getTask(string $content, array $params = []) { | ||||
| $params += [ | ||||
| AbstractAnnotator::CONFIG_DRY_RUN => true, | ||||
| AbstractAnnotator::CONFIG_VERBOSE => true, | ||||
| ]; | ||||
|
|
||||
| return new TableFindAnnotatorTask($this->io, $params, $content); | ||||
| } | ||||
|
|
||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| <?php | ||
|
|
||
| namespace App\Controller; | ||
|
|
||
| use Cake\ORM\Locator\LocatorAwareTrait; | ||
|
|
||
| /** @property \Cake\ORM\Table $Residents */ | ||
| class TestMeController | ||
| { | ||
| use LocatorAwareTrait; | ||
|
|
||
| public function test(): void { | ||
| $residentsTable = $this->fetchTable('Residents'); | ||
| $resident = $residentsTable->find( | ||
| 'all', | ||
| [ | ||
| 'contain' => ['Units', 'Rooms'], | ||
| ], | ||
| )->first(); | ||
|
|
||
| $residentOther = $residentsTable->find( | ||
| 'all', | ||
| [ | ||
| 'contain' => ['Units', 'Rooms'], | ||
| ], | ||
| )->firstOrFail(); | ||
|
|
||
| $residentX = $this->Residents->find( | ||
| 'all', | ||
| [ | ||
| 'contain' => ['Units', 'Rooms'], | ||
| ], | ||
| )->first(); | ||
|
|
||
| $residentY = $this->Residents->find( | ||
| 'all', | ||
| [ | ||
| 'contain' => ['Units', 'Rooms'], | ||
| ], | ||
| )->firstOrFail(); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
🧰 Tools
🪛 PHPMD (2.15.0)
41-41: Avoid unused parameters such as '$path'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents