Fix ClassDependencyManipulator to add dependency on right position (#…

…6413)

src/NodeManipulator/ClassDependencyManipulator.php

@@ -25,6 +25,9 @@
 use Rector\ValueObject\MethodName;
 use Rector\ValueObject\PhpVersionFeature;
 
+/**
+ * @see \Rector\Tests\NodeManipulator\ClassDependencyManipulatorTest
+ */
 final readonly class ClassDependencyManipulator
 {
     public function __construct(
@@ -143,6 +146,7 @@ private function addPromotedProperty(Class_ $class, PropertyMetadata $propertyMe
         } else {
             $constructClassMethod = $this->nodeFactory->createPublicMethod(MethodName::CONSTRUCT);
             $constructClassMethod->params[] = $param;
+
             $this->classInsertManipulator->addAsFirstMethod($class, $constructClassMethod);
         }
     }

src/NodeManipulator/ClassInsertManipulator.php

@@ -4,7 +4,6 @@
 
 namespace Rector\NodeManipulator;
 
-use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassConst;
 use PhpParser\Node\Stmt\ClassMethod;
@@ -23,20 +22,54 @@ public function __construct(
     /**
      * @api
      */
-    public function addAsFirstMethod(Class_ $class, Property | ClassConst | ClassMethod $stmt): void
+    public function addAsFirstMethod(Class_ $class, Property | ClassConst | ClassMethod $addedStmt): void
     {
         $scope = $class->getAttribute(AttributeKey::SCOPE);
-        $stmt->setAttribute(AttributeKey::SCOPE, $scope);
+        $addedStmt->setAttribute(AttributeKey::SCOPE, $scope);
 
-        if ($this->isSuccessToInsertBeforeFirstMethod($class, $stmt)) {
+        // no stmts? add this one
+        if ($class->stmts === []) {
+            $class->stmts[] = $addedStmt;
             return;
         }
 
-        if ($this->isSuccessToInsertAfterLastProperty($class, $stmt)) {
+        $newClassStmts = [];
+        $isAdded = false;
+
+        foreach ($class->stmts as $key => $classStmt) {
+            $nextStmt = $class->stmts[$key + 1] ?? null;
+
+            if ($isAdded === false) {
+                // first class method
+                if ($classStmt instanceof ClassMethod) {
+                    $newClassStmts[] = $addedStmt;
+                    $newClassStmts[] = $classStmt;
+                    $isAdded = true;
+                    continue;
+                }
+
+                // after last property
+                if ($classStmt instanceof Property && ! $nextStmt instanceof Property) {
+                    $newClassStmts[] = $classStmt;
+                    $newClassStmts[] = $addedStmt;
+                    $isAdded = true;
+                    continue;
+                }
+            }
+
+            $newClassStmts[] = $classStmt;
+        }
+
+        // still not added? try after last trait
+        // @todo
+
+        if ($isAdded) {
+            $class->stmts = $newClassStmts;
             return;
         }
 
-        $class->stmts[] = $stmt;
+        // keep added at least as first stmt
+        $class->stmts = array_merge([$addedStmt], $class->stmts);
     }
 
     /**
@@ -52,46 +85,4 @@ public function addPropertyToClass(Class_ $class, string $name, ?Type $type): vo
         $property = $this->nodeFactory->createPrivatePropertyFromNameAndType($name, $type);
         $this->addAsFirstMethod($class, $property);
     }
-
-    /**
-     * @param Stmt[] $stmts
-     * @return Stmt[]
-     */
-    private function insertBefore(array $stmts, Stmt $stmt, int $key): array
-    {
-        array_splice($stmts, $key, 0, [$stmt]);
-
-        return $stmts;
-    }
-
-    private function isSuccessToInsertBeforeFirstMethod(Class_ $class, ClassConst|ClassMethod|Property $stmt): bool
-    {
-        foreach ($class->stmts as $key => $classStmt) {
-            if (! $classStmt instanceof ClassMethod) {
-                continue;
-            }
-
-            $class->stmts = $this->insertBefore($class->stmts, $stmt, $key);
-
-            return true;
-        }
-
-        return false;
-    }
-
-    private function isSuccessToInsertAfterLastProperty(Class_ $class, ClassConst|ClassMethod|Property $stmt): bool
-    {
-        $previousElement = null;
-        foreach ($class->stmts as $key => $classStmt) {
-            if ($previousElement instanceof Property && ! $classStmt instanceof Property) {
-                $class->stmts = $this->insertBefore($class->stmts, $stmt, $key);
-
-                return true;
-            }
-
-            $previousElement = $classStmt;
-        }
-
-        return false;
-    }
 }

tests/NodeManipulator/ClassDependencyManipulatorTest.php

@@ -0,0 +1,102 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\NodeManipulator;
+
+use PhpParser\Node\Const_;
+use PhpParser\Node\Identifier;
+use PhpParser\Node\Scalar\String_;
+use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\ClassConst;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Property;
+use PhpParser\Node\Stmt\PropertyProperty;
+use PhpParser\PrettyPrinter\Standard;
+use PHPStan\Type\ObjectType;
+use Rector\NodeManipulator\ClassDependencyManipulator;
+use Rector\PostRector\ValueObject\PropertyMetadata;
+use Rector\Testing\PHPUnit\AbstractLazyTestCase;
+
+final class ClassDependencyManipulatorTest extends AbstractLazyTestCase
+{
+    private ClassDependencyManipulator $classDependencyManipulator;
+
+    private Standard $printerStandard;
+
+    protected function setUp(): void
+    {
+        $this->classDependencyManipulator = $this->make(ClassDependencyManipulator::class);
+        $this->printerStandard = new Standard();
+    }
+
+    public function testEmptyClass(): void
+    {
+        $someClass = new Class_(new Identifier('EmptyClass'));
+
+        $this->addSingleDependency($someClass);
+        $this->asssertClassEqualsFile($someClass, __DIR__ . '/Fixture/expected_empty_class.php.inc');
+    }
+
+    public function testSingleMethod(): void
+    {
+        $someClass = new Class_(new Identifier('SingleMethodClass'));
+        $someClass->stmts[] = new ClassMethod('firstMethod');
+
+        $this->addSingleDependency($someClass);
+
+        $this->asssertClassEqualsFile($someClass, __DIR__ . '/Fixture/expected_single_method.php.inc');
+    }
+
+    public function testWithProperty(): void
+    {
+        $someClass = new Class_(new Identifier('ClassWithSingleProperty'));
+        $someClass->stmts[] = new Property(Class_::MODIFIER_PRIVATE, [new PropertyProperty('someProperty')]);
+
+        $this->addSingleDependency($someClass);
+
+        $this->asssertClassEqualsFile($someClass, __DIR__ . '/Fixture/expected_single_property.php.inc');
+
+    }
+
+    public function testWithMethodAndProperty(): void
+    {
+        $someClass = new Class_(new Identifier('ClassWithMethodAndProperty'));
+        $someClass->stmts[] = new Property(Class_::MODIFIER_PRIVATE, [new PropertyProperty('someProperty')]);
+        $someClass->stmts[] = new ClassMethod(new Identifier('someMethod'));
+
+        $this->addSingleDependency($someClass);
+
+        $this->asssertClassEqualsFile($someClass, __DIR__ . '/Fixture/expected_method_and_property.php.inc');
+    }
+
+    public function testConstantProperties(): void
+    {
+        $someClass = new Class_(new Identifier('ConstantProperties'));
+        $someClass->stmts[] = new ClassConst([new Const_('SOME_CONST', new String_('value'))]);
+        $someClass->stmts[] = new Property(Class_::MODIFIER_PUBLIC, [new PropertyProperty('someProperty')]);
+        $someClass->stmts[] = new Property(Class_::MODIFIER_PUBLIC, [new PropertyProperty('anotherProperty')]);
+
+        $this->addSingleDependency($someClass);
+
+        $this->asssertClassEqualsFile($someClass, __DIR__ . '/Fixture/expected_class_const_property.php.inc');
+    }
+
+    private function addSingleDependency(Class_ $class): void
+    {
+        $this->classDependencyManipulator->addConstructorDependency($class, new PropertyMetadata(
+            'eventDispatcher',
+            new ObjectType('EventDispatcherInterface')
+        ));
+    }
+
+    private function asssertClassEqualsFile(Class_ $class, string $expectedFilePath): void
+    {
+        $printedClass = $this->printerStandard->prettyPrintFile([$class]);
+
+        // normalize newline in Windows
+        $printedClass = str_replace("\n", PHP_EOL, $printedClass . "\n");
+
+        $this->assertStringEqualsFile($expectedFilePath, $printedClass);
+    }
+}

tests/NodeManipulator/Fixture/expected_class_const_property.php.inc

@@ -0,0 +1,11 @@
+<?php
+
+class ConstantProperties
+{
+    const SOME_CONST = 'value';
+    public $someProperty;
+    public $anotherProperty;
+    public function __construct(private \EventDispatcherInterface $eventDispatcher)
+    {
+    }
+}

tests/NodeManipulator/Fixture/expected_empty_class.php.inc

@@ -0,0 +1,8 @@
+<?php
+
+class EmptyClass
+{
+    public function __construct(private \EventDispatcherInterface $eventDispatcher)
+    {
+    }
+}

tests/NodeManipulator/Fixture/expected_method_and_property.php.inc

@@ -0,0 +1,12 @@
+<?php
+
+class ClassWithMethodAndProperty
+{
+    private $someProperty;
+    public function __construct(private \EventDispatcherInterface $eventDispatcher)
+    {
+    }
+    function someMethod()
+    {
+    }
+}

tests/NodeManipulator/Fixture/expected_single_method.php.inc

@@ -0,0 +1,11 @@
+<?php
+
+class SingleMethodClass
+{
+    public function __construct(private \EventDispatcherInterface $eventDispatcher)
+    {
+    }
+    function firstMethod()
+    {
+    }
+}

tests/NodeManipulator/Fixture/expected_single_property.php.inc

@@ -0,0 +1,9 @@
+<?php
+
+class ClassWithSingleProperty
+{
+    private $someProperty;
+    public function __construct(private \EventDispatcherInterface $eventDispatcher)
+    {
+    }
+}