From f5872405e5e829c58080b81119d1ef302d7025a8 Mon Sep 17 00:00:00 2001 From: Artyom Osepyan Date: Wed, 6 May 2026 12:43:37 +0300 Subject: [PATCH 1/6] fix: remarks --- src/Contracts/InsertNodeContract.php | 10 --- src/Contracts/InsertNodesContract.php | 11 ++++ src/Visitors/AddImports.php | 13 +--- src/Visitors/AddTraits.php | 7 +-- src/Visitors/BaseNodeVisitorAbstract.php | 19 +++--- src/Visitors/InsertNodesAbstractVisitor.php | 63 ++++--------------- src/Visitors/PropertyVisitors/SetProperty.php | 18 +++--- 7 files changed, 45 insertions(+), 96 deletions(-) delete mode 100644 src/Contracts/InsertNodeContract.php create mode 100644 src/Contracts/InsertNodesContract.php diff --git a/src/Contracts/InsertNodeContract.php b/src/Contracts/InsertNodeContract.php deleted file mode 100644 index 70a38ad..0000000 --- a/src/Contracts/InsertNodeContract.php +++ /dev/null @@ -1,10 +0,0 @@ -filter() - ->unique(); + ->unique() + ->map(fn ($import) => new Use_([new UseItem(new Name($import))])); parent::__construct( nodesToInsert: $nodesToInsert, @@ -24,11 +25,6 @@ public function __construct(array $imports) ); } - public function leaveNode(Node $node): Node - { - return $node; - } - public function afterTraverse(array $nodes): ?array { $targetNamespace = array_find($nodes, fn ($node) => $node instanceof Namespace_); @@ -50,9 +46,4 @@ protected function getChildNodes(Node $node): array { return $node->uses; } - - protected function getInsertableNode(string $name): Node - { - return new Use_([new UseItem(new Name($name))]); - } } diff --git a/src/Visitors/AddTraits.php b/src/Visitors/AddTraits.php index c743372..aa44377 100644 --- a/src/Visitors/AddTraits.php +++ b/src/Visitors/AddTraits.php @@ -22,7 +22,7 @@ public function __construct(array $traits) $nodesToInsert = collect($traits) ->filter() ->unique() - ->map(fn ($trait) => class_basename($trait)); + ->map(fn ($trait) => new TraitUse([new Name(class_basename($trait))])); parent::__construct( nodesToInsert: $nodesToInsert, @@ -35,9 +35,4 @@ protected function getChildNodes(Node $node): array { return $node->traits; } - - protected function getInsertableNode(string $name): Node - { - return new TraitUse([new Name($name)]); - } } diff --git a/src/Visitors/BaseNodeVisitorAbstract.php b/src/Visitors/BaseNodeVisitorAbstract.php index 3adf8fe..08ce6cf 100644 --- a/src/Visitors/BaseNodeVisitorAbstract.php +++ b/src/Visitors/BaseNodeVisitorAbstract.php @@ -7,7 +7,7 @@ use PhpParser\Node\Stmt\Enum_; use PhpParser\Node\Stmt\Trait_; use PhpParser\NodeVisitorAbstract; -use RonasIT\Larabuilder\Contracts\InsertNodeContract; +use RonasIT\Larabuilder\Contracts\InsertNodesContract; use RonasIT\Larabuilder\Contracts\UpdateNodeContract; use RonasIT\Larabuilder\Exceptions\InvalidStructureTypeException; use RonasIT\Larabuilder\Support\NodeInserter; @@ -71,24 +71,23 @@ protected function modify(Node $node): Node $this->updatableNotFoundHook(); - return ($this instanceof InsertNodeContract) - ? $this->insertNode($node) - : $node; + if ($this instanceof InsertNodesContract) { + $this->insertNodes($node->stmts); + } + + return $node; } protected function updatableNotFoundHook(): void { } - /** @param Class_|Trait_|Enum_ $node */ - private function insertNode(Node $node): Node + protected function insertNodes(array &$nodes): void { $this->nodeInserter ??= new NodeInserter(); - $newNode = $this->getInsertableNode(); + $newNodes = $this->getInsertableNodes($nodes); - $this->nodeInserter->insertNodes($node->stmts, [$newNode], true); - - return $node; + $this->nodeInserter->insertNodes($nodes, $newNodes, true); } } diff --git a/src/Visitors/InsertNodesAbstractVisitor.php b/src/Visitors/InsertNodesAbstractVisitor.php index ac129ea..7ddda6f 100644 --- a/src/Visitors/InsertNodesAbstractVisitor.php +++ b/src/Visitors/InsertNodesAbstractVisitor.php @@ -4,76 +4,37 @@ use Illuminate\Support\Collection; use PhpParser\Node; -use PhpParser\Node\Stmt\Class_; -use PhpParser\Node\Stmt\Enum_; -use PhpParser\Node\Stmt\Trait_; -use PhpParser\Node\Stmt\TraitUse; -use PhpParser\Node\Stmt\Use_; -use RonasIT\Larabuilder\Support\NodeInserter; +use RonasIT\Larabuilder\Contracts\InsertNodesContract; -abstract class InsertNodesAbstractVisitor extends BaseNodeVisitorAbstract +abstract class InsertNodesAbstractVisitor extends BaseNodeVisitorAbstract implements InsertNodesContract { - abstract protected function getInsertableNode(string $name): Node; - abstract protected function getChildNodes(Node $node): array; - protected NodeInserter $nodeInserter; - public function __construct( protected Collection $nodesToInsert, protected string $targetNodeClass, ) { - $this->nodeInserter = new NodeInserter(); } - /** @param Class_|Enum_|Trait_ $node */ - protected function modify(Node $node): Node + public function getInsertableNodes(array $nodes): array { - $this->insertNodes($node->stmts); - - return $node; - } - - protected function insertNodes(array &$nodes): void - { - $newNodes = $this->getNodesToAdd($nodes); - - if (!empty($newNodes)) { - $nodes = $this->addNodes($nodes, $newNodes); - } - } - - protected function getNodesToAdd(array $nodes): Collection - { - $existingNodes = []; + $existingNames = []; foreach ($nodes as $node) { if (!($node instanceof $this->targetNodeClass)) { continue; } - /** @var TraitUse|Use_ $node */ - $childNodes = $this->getChildNodes($node); - - foreach ($childNodes as $childNode) { - if ($this->nodesToInsert->contains($childNode->name)) { - $existingNodes[] = $childNode->name; - } + foreach ($this->getChildNodes($node) as $childNode) { + $existingNames[] = (string) $childNode->name; } } - return $this - ->nodesToInsert - ->diff($existingNodes) - ->values(); - } - - protected function addNodes(array $nodes, Collection $newNodes): array - { - $insertableNodes = $newNodes->map(fn ($node) => $this->getInsertableNode($node))->all(); - - $this->nodeInserter->insertNodes($nodes, $insertableNodes); - - return $nodes; + return $this->nodesToInsert->filter( + fn (Node $newNode) => !in_array( + (string) $this->getChildNodes($newNode)[0]->name, + $existingNames, + ), + )->values()->all(); } } diff --git a/src/Visitors/PropertyVisitors/SetProperty.php b/src/Visitors/PropertyVisitors/SetProperty.php index b8c29ee..6e12f4f 100644 --- a/src/Visitors/PropertyVisitors/SetProperty.php +++ b/src/Visitors/PropertyVisitors/SetProperty.php @@ -6,13 +6,13 @@ use PhpParser\Node\Identifier; use PhpParser\Node\PropertyItem; use PhpParser\Node\Stmt\Property; -use RonasIT\Larabuilder\Contracts\InsertNodeContract; +use RonasIT\Larabuilder\Contracts\InsertNodesContract; use RonasIT\Larabuilder\Contracts\UpdateNodeContract; use RonasIT\Larabuilder\Enums\AccessModifierEnum; use RonasIT\Larabuilder\Support\ParentNodeLinker; use RonasIT\Larabuilder\Support\ValueNodeFactory; -class SetProperty extends AbstractPropertyVisitor implements InsertNodeContract, UpdateNodeContract +class SetProperty extends AbstractPropertyVisitor implements InsertNodesContract, UpdateNodeContract { protected PropertyItem $propertyItem; protected Identifier $typeIdentifier; @@ -51,12 +51,14 @@ public function updateNode(Node $node): void } } - public function getInsertableNode(): Node + public function getInsertableNodes(array $nodes): array { - return new Property( - flags: ($this->accessModifier ?? AccessModifierEnum::Public)->value, - props: [$this->propertyItem], - type: $this->typeIdentifier, - ); + return [ + new Property( + flags: ($this->accessModifier ?? AccessModifierEnum::Public)->value, + props: [$this->propertyItem], + type: $this->typeIdentifier, + ), + ]; } } From 5ff20d74a3d2de4ee536b2bb8df523183caa8134 Mon Sep 17 00:00:00 2001 From: Artyom Osepyan Date: Wed, 6 May 2026 15:58:21 +0300 Subject: [PATCH 2/6] refactor: replace abstract visitor hierarchy with contract-based polymorphic dispatch --- ARCHITECTURE.md | 14 +++--- src/Contracts/InsertNodeContract.php | 10 +++++ src/Contracts/InsertNodesContract.php | 5 ++- src/Visitors/AddImports.php | 29 ++++++------ src/Visitors/AddTraits.php | 23 +++++----- src/Visitors/BaseNodeVisitorAbstract.php | 45 ++++++++++++++++--- src/Visitors/InsertNodesAbstractVisitor.php | 40 ----------------- src/Visitors/PropertyVisitors/SetProperty.php | 18 ++++---- 8 files changed, 95 insertions(+), 89 deletions(-) create mode 100644 src/Contracts/InsertNodeContract.php delete mode 100644 src/Visitors/InsertNodesAbstractVisitor.php diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 5e04652..be65532 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -43,11 +43,10 @@ For modifying classes, traits, enums, etc. The base class handles: #### Contracts - **`UpdateNodeContract`** — The visitor can update an existing node. Requires `shouldUpdateNode(Node): bool` and `updateNode(Node): void`. The base class iterates over child statements and calls `updateNode()` on the first match. -- **`InsertNodeContract`** — The visitor can insert a new node. Requires `getInsertableNode(): Node`. The base class handles positioning (via `NodeInserter`) and empty line insertion. +- **`InsertNodeContract`** — The visitor inserts a single node. Requires `getInsertableNode(): Node`. The base class handles positioning (via `NodeInserter`) and empty line insertion. +- **`InsertNodesContract`** — The visitor inserts multiple nodes with duplicate filtering. Requires `getInsertableNodes(): array` (returns all candidate nodes) and `getSubNodes(Node): array` (returns child nodes used for duplicate detection). The base class calls `filterExistingNodes()` to exclude already-present nodes before inserting. -A visitor may implement both contracts. In that case, update is attempted first — insertion happens only if no existing node matched. - -**Bulk insertion visitors** extend `InsertNodesAbstractVisitor` (which extends `BaseNodeVisitorAbstract`) and handle inserting multiple nodes with built-in duplicate filtering. +A visitor may implement `UpdateNodeContract` together with `InsertNodeContract` or `InsertNodesContract`. In that case, update is attempted first — insertion happens only if no existing node matched. ### App bootstrap visitors (`AbstractAppBootstrapVisitor`) @@ -72,8 +71,11 @@ Located in `src/Support/`: ## Creating a New Visitor -1. Extend `BaseNodeVisitorAbstract` (or `InsertNodesAbstractVisitor` for bulk insertions). +1. Extend `BaseNodeVisitorAbstract`. 2. Set `$allowedParentNodesTypes` to the node types your visitor targets. -3. Implement `InsertNodeContract`, `UpdateNodeContract`, or both. +3. Implement the appropriate contract(s): + - `InsertNodeContract` — for inserting a single node, implement `getInsertableNode(): Node` + - `InsertNodesContract` — for inserting multiple nodes with dedup, implement `getInsertableNodes(): array` and `getSubNodes(Node): array` + - `UpdateNodeContract` — for updating an existing node, implement `shouldUpdateNode(Node): bool` and `updateNode(Node): void` 4. Add a corresponding fluent method in `PHPFileBuilder` that creates and registers the visitor. 5. Add fixture files and a test case. diff --git a/src/Contracts/InsertNodeContract.php b/src/Contracts/InsertNodeContract.php new file mode 100644 index 0000000..70a38ad --- /dev/null +++ b/src/Contracts/InsertNodeContract.php @@ -0,0 +1,10 @@ +filter() - ->unique() - ->map(fn ($import) => new Use_([new UseItem(new Name($import))])); - - parent::__construct( - nodesToInsert: $nodesToInsert, - targetNodeClass: Use_::class, - ); + public function __construct( + protected array $imports, + ) { } public function afterTraverse(array $nodes): ?array { $targetNamespace = array_find($nodes, fn ($node) => $node instanceof Namespace_); - if (!is_null($targetNamespace)) { + if ($targetNamespace !== null) { /** @var Namespace_ $targetNamespace */ $targetNodes = &$targetNamespace->stmts; } else { @@ -41,8 +34,16 @@ public function afterTraverse(array $nodes): ?array return $nodes; } + public function getInsertableNodes(): array + { + return array_map( + fn ($import) => new Use_([new UseItem(new Name($import))]), + array_unique(array_filter($this->imports)), + ); + } + /** @param Use_ $node */ - protected function getChildNodes(Node $node): array + public function getSubNodes(Node $node): array { return $node->uses; } diff --git a/src/Visitors/AddTraits.php b/src/Visitors/AddTraits.php index aa44377..2ed69d3 100644 --- a/src/Visitors/AddTraits.php +++ b/src/Visitors/AddTraits.php @@ -8,8 +8,9 @@ use PhpParser\Node\Stmt\Enum_; use PhpParser\Node\Stmt\Trait_; use PhpParser\Node\Stmt\TraitUse; +use RonasIT\Larabuilder\Contracts\InsertNodesContract; -class AddTraits extends InsertNodesAbstractVisitor +class AddTraits extends BaseNodeVisitorAbstract implements InsertNodesContract { protected array $allowedParentNodesTypes = [ Class_::class, @@ -17,21 +18,21 @@ class AddTraits extends InsertNodesAbstractVisitor Enum_::class, ]; - public function __construct(array $traits) - { - $nodesToInsert = collect($traits) - ->filter() - ->unique() - ->map(fn ($trait) => new TraitUse([new Name(class_basename($trait))])); + public function __construct( + protected array $traits, + ) { + } - parent::__construct( - nodesToInsert: $nodesToInsert, - targetNodeClass: TraitUse::class, + public function getInsertableNodes(): array + { + return array_map( + fn ($trait) => new TraitUse([new Name(class_basename($trait))]), + array_unique(array_filter($this->traits)), ); } /** @param TraitUse $node */ - protected function getChildNodes(Node $node): array + public function getSubNodes(Node $node): array { return $node->traits; } diff --git a/src/Visitors/BaseNodeVisitorAbstract.php b/src/Visitors/BaseNodeVisitorAbstract.php index 08ce6cf..a68cd7c 100644 --- a/src/Visitors/BaseNodeVisitorAbstract.php +++ b/src/Visitors/BaseNodeVisitorAbstract.php @@ -7,6 +7,7 @@ use PhpParser\Node\Stmt\Enum_; use PhpParser\Node\Stmt\Trait_; use PhpParser\NodeVisitorAbstract; +use RonasIT\Larabuilder\Contracts\InsertNodeContract; use RonasIT\Larabuilder\Contracts\InsertNodesContract; use RonasIT\Larabuilder\Contracts\UpdateNodeContract; use RonasIT\Larabuilder\Exceptions\InvalidStructureTypeException; @@ -71,9 +72,7 @@ protected function modify(Node $node): Node $this->updatableNotFoundHook(); - if ($this instanceof InsertNodesContract) { - $this->insertNodes($node->stmts); - } + $this->insertNodes($node->stmts); return $node; } @@ -84,10 +83,44 @@ protected function updatableNotFoundHook(): void protected function insertNodes(array &$nodes): void { - $this->nodeInserter ??= new NodeInserter(); + $newNodes = match (true) { + $this instanceof InsertNodesContract => $this->filterExistingNodes($nodes), + $this instanceof InsertNodeContract => [$this->getInsertableNode()], + default => null, + }; + + if (!empty($newNodes)) { + $this->nodeInserter ??= new NodeInserter(); + + $this->nodeInserter->insertNodes($nodes, $newNodes, true); + } + } + + private function filterExistingNodes(array $nodes): array + { + $insertableNodes = $this->getInsertableNodes(); - $newNodes = $this->getInsertableNodes($nodes); + if (empty($insertableNodes)) { + return []; + } + + $targetNodeClass = get_class($insertableNodes[0]); + + $existingNames = []; + + foreach ($nodes as $node) { + if (!($node instanceof $targetNodeClass)) { + continue; + } + + foreach ($this->getSubNodes($node) as $childNode) { + $existingNames[] = (string) $childNode->name; + } + } - $this->nodeInserter->insertNodes($nodes, $newNodes, true); + return array_values(array_filter( + $insertableNodes, + fn (Node $newNode) => !in_array((string) $this->getSubNodes($newNode)[0]->name, $existingNames), + )); } } diff --git a/src/Visitors/InsertNodesAbstractVisitor.php b/src/Visitors/InsertNodesAbstractVisitor.php deleted file mode 100644 index 7ddda6f..0000000 --- a/src/Visitors/InsertNodesAbstractVisitor.php +++ /dev/null @@ -1,40 +0,0 @@ -targetNodeClass)) { - continue; - } - - foreach ($this->getChildNodes($node) as $childNode) { - $existingNames[] = (string) $childNode->name; - } - } - - return $this->nodesToInsert->filter( - fn (Node $newNode) => !in_array( - (string) $this->getChildNodes($newNode)[0]->name, - $existingNames, - ), - )->values()->all(); - } -} diff --git a/src/Visitors/PropertyVisitors/SetProperty.php b/src/Visitors/PropertyVisitors/SetProperty.php index 6e12f4f..b8c29ee 100644 --- a/src/Visitors/PropertyVisitors/SetProperty.php +++ b/src/Visitors/PropertyVisitors/SetProperty.php @@ -6,13 +6,13 @@ use PhpParser\Node\Identifier; use PhpParser\Node\PropertyItem; use PhpParser\Node\Stmt\Property; -use RonasIT\Larabuilder\Contracts\InsertNodesContract; +use RonasIT\Larabuilder\Contracts\InsertNodeContract; use RonasIT\Larabuilder\Contracts\UpdateNodeContract; use RonasIT\Larabuilder\Enums\AccessModifierEnum; use RonasIT\Larabuilder\Support\ParentNodeLinker; use RonasIT\Larabuilder\Support\ValueNodeFactory; -class SetProperty extends AbstractPropertyVisitor implements InsertNodesContract, UpdateNodeContract +class SetProperty extends AbstractPropertyVisitor implements InsertNodeContract, UpdateNodeContract { protected PropertyItem $propertyItem; protected Identifier $typeIdentifier; @@ -51,14 +51,12 @@ public function updateNode(Node $node): void } } - public function getInsertableNodes(array $nodes): array + public function getInsertableNode(): Node { - return [ - new Property( - flags: ($this->accessModifier ?? AccessModifierEnum::Public)->value, - props: [$this->propertyItem], - type: $this->typeIdentifier, - ), - ]; + return new Property( + flags: ($this->accessModifier ?? AccessModifierEnum::Public)->value, + props: [$this->propertyItem], + type: $this->typeIdentifier, + ); } } From 0520d5e81847c25421547680a88125595a25a5ea Mon Sep 17 00:00:00 2001 From: Artyom Osepyan Date: Wed, 6 May 2026 16:36:26 +0300 Subject: [PATCH 3/6] fix: remarks --- src/Visitors/AddImports.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Visitors/AddImports.php b/src/Visitors/AddImports.php index 155f6ff..935b6e9 100644 --- a/src/Visitors/AddImports.php +++ b/src/Visitors/AddImports.php @@ -22,7 +22,7 @@ public function afterTraverse(array $nodes): ?array { $targetNamespace = array_find($nodes, fn ($node) => $node instanceof Namespace_); - if ($targetNamespace !== null) { + if (!is_null($targetNamespace)) { /** @var Namespace_ $targetNamespace */ $targetNodes = &$targetNamespace->stmts; } else { From c2bed30d62f40356fed5972b12e931d923aaa02d Mon Sep 17 00:00:00 2001 From: Artyom Osepyan Date: Thu, 7 May 2026 17:11:48 +0300 Subject: [PATCH 4/6] fix: remarks --- src/Visitors/AbstractNodeVisitor.php | 19 ++++++------------- src/Visitors/AddImports.php | 2 +- src/Visitors/AddTraits.php | 2 +- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/Visitors/AbstractNodeVisitor.php b/src/Visitors/AbstractNodeVisitor.php index 4a8b9cf..21de695 100644 --- a/src/Visitors/AbstractNodeVisitor.php +++ b/src/Visitors/AbstractNodeVisitor.php @@ -85,9 +85,6 @@ protected function updatableNodeNotFoundHook(): void { } -<<<<<<< HEAD:src/Visitors/BaseNodeVisitorAbstract.php - protected function insertNodes(array &$nodes): void -======= protected function linkParents(Node $parent): void { foreach ($parent->getSubNodeNames() as $name) { @@ -100,9 +97,7 @@ protected function linkParents(Node $parent): void } } - /** @param Class_|Trait_|Enum_ $node */ - private function insertNode(Node $node): Node ->>>>>>> origin/master:src/Visitors/AbstractNodeVisitor.php + protected function insertNodes(array &$nodes): void { $newNodes = match (true) { $this instanceof InsertNodesContract => $this->filterExistingNodes($nodes), @@ -113,15 +108,13 @@ private function insertNode(Node $node): Node if (!empty($newNodes)) { $this->nodeInserter ??= new NodeInserter(); -<<<<<<< HEAD:src/Visitors/BaseNodeVisitorAbstract.php - $this->nodeInserter->insertNodes($nodes, $newNodes, true); + foreach ($newNodes as $newNode) { + $this->linkParents($newNode); + } + + $this->nodeInserter->insertNodes($nodes, $newNodes); } } -======= - $this->linkParents($newNode); - - $this->nodeInserter->insertNodes($node->stmts, [$newNode]); ->>>>>>> origin/master:src/Visitors/AbstractNodeVisitor.php private function filterExistingNodes(array $nodes): array { diff --git a/src/Visitors/AddImports.php b/src/Visitors/AddImports.php index ac07678..4c1d6c7 100644 --- a/src/Visitors/AddImports.php +++ b/src/Visitors/AddImports.php @@ -9,7 +9,7 @@ use PhpParser\Node\UseItem; use RonasIT\Larabuilder\Contracts\InsertNodesContract; -class AddImports extends AbstractInsertNodesVisitor implements InsertNodesContract +class AddImports extends AbstractNodeVisitor implements InsertNodesContract { protected array $allowedParentNodesTypes = self::ANY_TYPE; diff --git a/src/Visitors/AddTraits.php b/src/Visitors/AddTraits.php index 7a9f7e5..3723187 100644 --- a/src/Visitors/AddTraits.php +++ b/src/Visitors/AddTraits.php @@ -10,7 +10,7 @@ use PhpParser\Node\Stmt\TraitUse; use RonasIT\Larabuilder\Contracts\InsertNodesContract; -class AddTraits extends AbstractInsertNodesVisitor implements InsertNodesContract +class AddTraits extends AbstractNodeVisitor implements InsertNodesContract { protected array $allowedParentNodesTypes = [ Class_::class, From d13ce0838918548ba581b1d81c333d54077f34b6 Mon Sep 17 00:00:00 2001 From: Artyom Osepyan Date: Thu, 7 May 2026 17:14:17 +0300 Subject: [PATCH 5/6] fix: remarks --- ARCHITECTURE.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 2fabaa1..06da59a 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -70,8 +70,7 @@ Located in `src/Support/`: ## Creating a New Visitor -1. Extend `AbstractNodeVisitor` (or `AbstractInsertNodesVisitor` for bulk insertions). -1. Extend `BaseNodeVisitorAbstract`. +1. Extend `AbstractNodeVisitor`. 2. Set `$allowedParentNodesTypes` to the node types your visitor targets. 3. Implement the appropriate contract(s): - `InsertNodeContract` — for inserting a single node, implement `getInsertableNode(): Node` From 7e18bad8788a111ec3cd468650dc9b7c280107e8 Mon Sep 17 00:00:00 2001 From: Artyom Osepyan Date: Fri, 22 May 2026 16:34:59 +0300 Subject: [PATCH 6/6] fix: remarks from reviewer --- ARCHITECTURE.md | 4 +- src/Builders/PHPFileBuilder.php | 12 ++- src/Contracts/InsertNodesContract.php | 12 --- src/Support/NodeInserter.php | 15 ++-- src/Visitors/AbstractNodeVisitor.php | 66 +++++---------- .../{AddImports.php => AddImport.php} | 32 +++++--- src/Visitors/AddTrait.php | 48 +++++++++++ src/Visitors/AddTraits.php | 39 --------- tests/NodeInserterTest.php | 81 ------------------- tests/PHPFileBuilderTest.php | 2 +- 10 files changed, 103 insertions(+), 208 deletions(-) delete mode 100644 src/Contracts/InsertNodesContract.php rename src/Visitors/{AddImports.php => AddImport.php} (50%) create mode 100644 src/Visitors/AddTrait.php delete mode 100644 src/Visitors/AddTraits.php delete mode 100644 tests/NodeInserterTest.php diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 06da59a..7fa033d 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -44,9 +44,8 @@ For modifying classes, traits, enums, etc. The base class handles: - **`UpdateNodeContract`** — The visitor can update an existing node. Requires `shouldUpdateNode(Node): bool` and `updateNode(Node): void`. The base class iterates over child statements and calls `updateNode()` on the first match. - **`InsertNodeContract`** — The visitor inserts a single node. Requires `getInsertableNode(): Node`. The base class handles positioning (via `NodeInserter`) and empty line insertion. -- **`InsertNodesContract`** — The visitor inserts multiple nodes with duplicate filtering. Requires `getInsertableNodes(): array` (returns all candidate nodes) and `getSubNodes(Node): array` (returns child nodes used for duplicate detection). The base class calls `filterExistingNodes()` to exclude already-present nodes before inserting. -A visitor may implement `UpdateNodeContract` together with `InsertNodeContract` or `InsertNodesContract`. In that case, update is attempted first — insertion happens only if no existing node matched. +A visitor may implement both contracts. Update is attempted first — insertion happens only if no existing node matched. ### App bootstrap visitors (`AbstractAppBootstrapVisitor`) @@ -74,7 +73,6 @@ Located in `src/Support/`: 2. Set `$allowedParentNodesTypes` to the node types your visitor targets. 3. Implement the appropriate contract(s): - `InsertNodeContract` — for inserting a single node, implement `getInsertableNode(): Node` - - `InsertNodesContract` — for inserting multiple nodes with dedup, implement `getInsertableNodes(): array` and `getSubNodes(Node): array` - `UpdateNodeContract` — for updating an existing node, implement `shouldUpdateNode(Node): bool` and `updateNode(Node): void` 4. Add a corresponding fluent method in `PHPFileBuilder` that creates and registers the visitor. 5. Add fixture files and a test case. diff --git a/src/Builders/PHPFileBuilder.php b/src/Builders/PHPFileBuilder.php index 0f9128e..0eebd3a 100644 --- a/src/Builders/PHPFileBuilder.php +++ b/src/Builders/PHPFileBuilder.php @@ -10,8 +10,8 @@ use RonasIT\Larabuilder\Exceptions\InvalidPHPFileException; use RonasIT\Larabuilder\NodeTraverser; use RonasIT\Larabuilder\Printer; -use RonasIT\Larabuilder\Visitors\AddImports; -use RonasIT\Larabuilder\Visitors\AddTraits; +use RonasIT\Larabuilder\Visitors\AddImport; +use RonasIT\Larabuilder\Visitors\AddTrait; use RonasIT\Larabuilder\Visitors\InsertCodeToMethod; use RonasIT\Larabuilder\Visitors\PropertyVisitors\AddArrayPropertyItem; use RonasIT\Larabuilder\Visitors\PropertyVisitors\RemoveArrayPropertyItem; @@ -64,14 +64,18 @@ public function removeArrayPropertyItem(string $propertyName, array $values): se public function addImports(array $imports): self { - $this->traverser->addVisitor(new AddImports($imports)); + foreach ($imports as $import) { + $this->traverser->addVisitor(new AddImport($import)); + } return $this; } public function addTraits(array $traits): self { - $this->traverser->addVisitor(new AddTraits($traits)); + foreach ($traits as $trait) { + $this->traverser->addVisitor(new AddTrait($trait)); + } $this->addImports($traits); diff --git a/src/Contracts/InsertNodesContract.php b/src/Contracts/InsertNodesContract.php deleted file mode 100644 index da64427..0000000 --- a/src/Contracts/InsertNodesContract.php +++ /dev/null @@ -1,12 +0,0 @@ -getInsertIndex($stmts, $newNodeClass); + $insertIndex = $this->getInsertIndex($stmts, $newNodeClass); - $newNode->setAttribute(StatementAttributeEnum::Previous->value, Arr::get($stmts, $insertIndex - 1)); + $newNode->setAttribute(StatementAttributeEnum::Previous->value, Arr::get($stmts, $insertIndex - 1)); - array_splice($stmts, $insertIndex, 0, [$newNode]); + array_splice($stmts, $insertIndex, 0, [$newNode]); - $this->insertEmptyLineIfNeeded($stmts, $insertIndex + 1, $newNodeClass); - } + $this->insertEmptyLineIfNeeded($stmts, $insertIndex + 1, $newNodeClass); } protected function getInsertIndex(array $statements, string $insertType): int diff --git a/src/Visitors/AbstractNodeVisitor.php b/src/Visitors/AbstractNodeVisitor.php index 21de695..3299a7f 100644 --- a/src/Visitors/AbstractNodeVisitor.php +++ b/src/Visitors/AbstractNodeVisitor.php @@ -9,7 +9,6 @@ use PhpParser\Node\Stmt\Trait_; use PhpParser\NodeVisitorAbstract; use RonasIT\Larabuilder\Contracts\InsertNodeContract; -use RonasIT\Larabuilder\Contracts\InsertNodesContract; use RonasIT\Larabuilder\Contracts\UpdateNodeContract; use RonasIT\Larabuilder\Enums\StatementAttributeEnum; use RonasIT\Larabuilder\Exceptions\InvalidStructureTypeException; @@ -76,7 +75,7 @@ protected function modify(Node $node): Node $this->updatableNodeNotFoundHook(); } - $this->insertNodes($node->stmts); + $this->insertNode($node->stmts); return $node; } @@ -85,62 +84,33 @@ protected function updatableNodeNotFoundHook(): void { } - protected function linkParents(Node $parent): void - { - foreach ($parent->getSubNodeNames() as $name) { - foreach (Arr::wrap($parent->$name) as $child) { - if ($child instanceof Node) { - $child->setAttribute(StatementAttributeEnum::Parent->value, $parent); - $this->linkParents($child); - } - } - } - } - - protected function insertNodes(array &$nodes): void + protected function insertNode(array &$stmts): void { - $newNodes = match (true) { - $this instanceof InsertNodesContract => $this->filterExistingNodes($nodes), - $this instanceof InsertNodeContract => [$this->getInsertableNode()], - default => null, - }; - - if (!empty($newNodes)) { + if ($this instanceof InsertNodeContract && !$this->isDuplicate($stmts)) { $this->nodeInserter ??= new NodeInserter(); - foreach ($newNodes as $newNode) { - $this->linkParents($newNode); - } + $newNode = $this->getInsertableNode(); - $this->nodeInserter->insertNodes($nodes, $newNodes); + $this->linkParents($newNode); + + $this->nodeInserter->insertNode($stmts, $newNode); } } - private function filterExistingNodes(array $nodes): array + protected function isDuplicate(array $stmts): bool { - $insertableNodes = $this->getInsertableNodes(); - - if (empty($insertableNodes)) { - return []; - } - - $targetNodeClass = get_class($insertableNodes[0]); - - $existingNames = []; - - foreach ($nodes as $node) { - if (!($node instanceof $targetNodeClass)) { - continue; - } + return false; + } - foreach ($this->getSubNodes($node) as $childNode) { - $existingNames[] = (string) $childNode->name; + protected function linkParents(Node $parent): void + { + foreach ($parent->getSubNodeNames() as $name) { + foreach (Arr::wrap($parent->$name) as $child) { + if ($child instanceof Node) { + $child->setAttribute(StatementAttributeEnum::Parent->value, $parent); + $this->linkParents($child); + } } } - - return array_values(array_filter( - $insertableNodes, - fn (Node $newNode) => !in_array((string) $this->getSubNodes($newNode)[0]->name, $existingNames), - )); } } diff --git a/src/Visitors/AddImports.php b/src/Visitors/AddImport.php similarity index 50% rename from src/Visitors/AddImports.php rename to src/Visitors/AddImport.php index 4c1d6c7..2e2828d 100644 --- a/src/Visitors/AddImports.php +++ b/src/Visitors/AddImport.php @@ -7,14 +7,14 @@ use PhpParser\Node\Stmt\Namespace_; use PhpParser\Node\Stmt\Use_; use PhpParser\Node\UseItem; -use RonasIT\Larabuilder\Contracts\InsertNodesContract; +use RonasIT\Larabuilder\Contracts\InsertNodeContract; -class AddImports extends AbstractNodeVisitor implements InsertNodesContract +class AddImport extends AbstractNodeVisitor implements InsertNodeContract { protected array $allowedParentNodesTypes = self::ANY_TYPE; public function __construct( - protected array $imports, + protected string $import, ) { } @@ -29,22 +29,30 @@ public function afterTraverse(array $nodes): ?array $targetNodes = &$nodes; } - $this->insertNodes($targetNodes); + $this->insertNode($targetNodes); return $nodes; } - public function getInsertableNodes(): array + public function getInsertableNode(): Node { - return array_map( - fn ($import) => new Use_([new UseItem(new Name($import))]), - array_unique(array_filter($this->imports)), - ); + return new Use_([new UseItem(new Name($this->import))]); } - /** @param Use_ $node */ - public function getSubNodes(Node $node): array + protected function isDuplicate(array $stmts): bool { - return $node->uses; + foreach ($stmts as $stmt) { + if (!($stmt instanceof Use_)) { + continue; + } + + foreach ($stmt->uses as $useItem) { + if ($useItem->name->toString() === $this->import) { + return true; + } + } + } + + return false; } } diff --git a/src/Visitors/AddTrait.php b/src/Visitors/AddTrait.php new file mode 100644 index 0000000..2fcd754 --- /dev/null +++ b/src/Visitors/AddTrait.php @@ -0,0 +1,48 @@ +trait = class_basename($this->trait); + } + + public function getInsertableNode(): Node + { + return new TraitUse([new Name($this->trait)]); + } + + protected function isDuplicate(array $stmts): bool + { + foreach ($stmts as $stmt) { + if (!($stmt instanceof TraitUse)) { + continue; + } + + foreach ($stmt->traits as $traitName) { + if ($traitName->getLast() === $this->trait) { + return true; + } + } + } + + return false; + } +} diff --git a/src/Visitors/AddTraits.php b/src/Visitors/AddTraits.php deleted file mode 100644 index 3723187..0000000 --- a/src/Visitors/AddTraits.php +++ /dev/null @@ -1,39 +0,0 @@ - new TraitUse([new Name(class_basename($trait))]), - array_unique(array_filter($this->traits)), - ); - } - - /** @param TraitUse $node */ - public function getSubNodes(Node $node): array - { - return $node->traits; - } -} diff --git a/tests/NodeInserterTest.php b/tests/NodeInserterTest.php deleted file mode 100644 index fd1278f..0000000 --- a/tests/NodeInserterTest.php +++ /dev/null @@ -1,81 +0,0 @@ -inserter = new NodeInserter(); - } - - public function testInsertMixedNodes(): void - { - $this->prepareFixture('class.php'); - - $classNode = (new NodeFinder())->findFirstInstanceOf($this->newSyntaxTree, Class_::class); - - $this->inserter->insertNodes($classNode->stmts, [ - new Property(Modifiers::PUBLIC, [new PropertyItem('anotherProperty')]), - new ClassMethod('newMethod', ['flags' => Modifiers::PUBLIC]), - new ClassConst([new Const_('NEW_CONST', new Int_(42))], Modifiers::PUBLIC), - new ClassMethod('anotherMethod', ['flags' => Modifiers::PUBLIC]), - new Property(Modifiers::PUBLIC, [new PropertyItem('newProperty')]), - new TraitUse([new Name('NewTrait')]), - new ClassConst([new Const_('ANOTHER_CONST', new Int_(0))], Modifiers::PUBLIC), - new TraitUse([new Name('AnotherTrait')]), - ], true); - - $this->assertSame( - $this->getFixture('class_with_mixed_nodes_inserted.php'), - $this->printResult(), - ); - } - - protected function prepareFixture(string $fixture): void - { - $parser = (new ParserFactory())->createForHostVersion(); - - $code = file_get_contents($this->generateOriginalStructurePath($fixture)); - - $syntaxTree = $parser->parse($code); - - $this->oldTokens = $parser->getTokens(); - - $traverser = new NodeTraverser(); - - $traverser->addVisitor(new CloningVisitor()); - - $this->oldSyntaxTree = $syntaxTree; - $this->newSyntaxTree = $traverser->traverse($syntaxTree); - } - - protected function printResult(): string - { - return (new Printer())->printFormatPreserving($this->newSyntaxTree, $this->oldSyntaxTree, $this->oldTokens); - } -} diff --git a/tests/PHPFileBuilderTest.php b/tests/PHPFileBuilderTest.php index d6148d1..bcf26ca 100644 --- a/tests/PHPFileBuilderTest.php +++ b/tests/PHPFileBuilderTest.php @@ -359,7 +359,7 @@ public function testAddTraitsNotClassTraitEnum(): void { $file = $this->generateOriginalStructurePath('interface.php'); - $this->assertExceptionThrew(InvalidStructureTypeException::class, "'AddTraits' operation may only be applied to: Class, Trait, Enum."); + $this->assertExceptionThrew(InvalidStructureTypeException::class, "'AddTrait' operation may only be applied to: Class, Trait, Enum."); new PHPFileBuilder($file) ->addTraits([