Refactor the OrderByKeyToClassConstRector to use the new enum only in…

… Criteria::orderBy method call. Relates to: - doctrine/collections#389; - doctrine/orm#11313 (comment)
julienfastre · Jul 31, 2024 · 4c9ac77 · 4c9ac77
1 parent 3ba7adb
commit 4c9ac77
Show file tree

Hide file tree

Showing 9 changed files with 140 additions and 93 deletions.
diff --git a/...rty/OrderByKeyToClassConstRector/Fixture/criteria_to_order_from_constant_criteria.php.inc b/...rty/OrderByKeyToClassConstRector/Fixture/criteria_to_order_from_constant_criteria.php.inc
@@ -0,0 +1,17 @@
+<?php
+
+use Doctrine\Common\Collections\Criteria;
+
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => Criteria::ASC, 'desc' => Criteria::DESC]);
+
+?>
+-----
+<?php
+
+use Doctrine\Common\Collections\Criteria;
+
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => \Doctrine\Common\Collections\Order::Ascending, 'desc' => \Doctrine\Common\Collections\Order::Descending]);
+
+?>
diff --git a/...Property/OrderByKeyToClassConstRector/Fixture/criteria_to_order_from_string_lower.php.inc b/...Property/OrderByKeyToClassConstRector/Fixture/criteria_to_order_from_string_lower.php.inc
@@ -0,0 +1,17 @@
+<?php
+
+use Doctrine\Common\Collections\Criteria;
+
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => 'asc', 'desc' => 'desc']);
+
+?>
+-----
+<?php
+
+use Doctrine\Common\Collections\Criteria;
+
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => \Doctrine\Common\Collections\Order::Ascending, 'desc' => \Doctrine\Common\Collections\Order::Descending]);
+
+?>
diff --git a/...Property/OrderByKeyToClassConstRector/Fixture/criteria_to_order_from_string_upper.php.inc b/...Property/OrderByKeyToClassConstRector/Fixture/criteria_to_order_from_string_upper.php.inc
@@ -0,0 +1,17 @@
+<?php
+
+use Doctrine\Common\Collections\Criteria;
+
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => 'ASC', 'desc' => 'DESC']);
+
+?>
+-----
+<?php
+
+use Doctrine\Common\Collections\Criteria;
+
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => \Doctrine\Common\Collections\Order::Ascending, 'desc' => \Doctrine\Common\Collections\Order::Descending]);
+
+?>
diff --git a/...derByKeyToClassConstRector/Fixture/replace_order_by_asc_lower_with_class_constant.php.inc b/...derByKeyToClassConstRector/Fixture/replace_order_by_asc_lower_with_class_constant.php.inc
@@ -10,16 +10,3 @@ class ReplaceOrderByAscWithClassConstant
     protected \DateTimeInterface $messages;
 }
 ?>
------
-<?php
-
-namespace Rector\Doctrine\Tests\CodeQuality\Rector\Property\OrderByKeyToClassConstRector\Fixture;
-
-use Doctrine\ORM\Mapping as ORM;
-
-class ReplaceOrderByAscWithClassConstant
-{
-    #[ORM\OrderBy(['createdAt' => \Doctrine\Common\Collections\Criteria::ASC])]
-    protected \DateTimeInterface $messages;
-}
-?>
diff --git a/...rty/OrderByKeyToClassConstRector/Fixture/replace_order_by_asc_with_class_constant.php.inc b/...rty/OrderByKeyToClassConstRector/Fixture/replace_order_by_asc_with_class_constant.php.inc
@@ -10,16 +10,3 @@ class ReplaceOrderByAscWithClassConstant
     protected \DateTimeInterface $messages;
 }
 ?>
------
-<?php
-
-namespace Rector\Doctrine\Tests\CodeQuality\Rector\Property\OrderByKeyToClassConstRector\Fixture;
-
-use Doctrine\ORM\Mapping as ORM;
-
-class ReplaceOrderByAscWithClassConstant
-{
-    #[ORM\OrderBy(['createdAt' => \Doctrine\Common\Collections\Criteria::ASC])]
-    protected \DateTimeInterface $messages;
-}
-?>
diff --git a/...ty/OrderByKeyToClassConstRector/Fixture/replace_order_by_desc_with_class_constant.php.inc b/...ty/OrderByKeyToClassConstRector/Fixture/replace_order_by_desc_with_class_constant.php.inc
@@ -10,16 +10,3 @@ class ReplaceOrderByAscWithClassConstant
     protected \DateTimeInterface $messages;
 }
 ?>
------
-<?php
-
-namespace Rector\Doctrine\Tests\CodeQuality\Rector\Property\OrderByKeyToClassConstRector\Fixture;
-
-use Doctrine\ORM\Mapping as ORM;
-
-class ReplaceOrderByAscWithClassConstant
-{
-    #[ORM\OrderBy(['createdAt' => \Doctrine\Common\Collections\Criteria::DESC])]
-    protected \DateTimeInterface $messages;
-}
-?>
diff --git a/...ity/Rector/Property/OrderByKeyToClassConstRector/Fixture/skip_already_using_const.php.inc b/...ity/Rector/Property/OrderByKeyToClassConstRector/Fixture/skip_already_using_const.php.inc
@@ -1,13 +1,8 @@
 <?php
 
-namespace Rector\Doctrine\Tests\CodeQuality\Rector\Property\OrderByKeyToClassConstRector\Fixture;
-
 use Doctrine\Common\Collections\Criteria;
-use Doctrine\ORM\Mapping as ORM;
 
-class SkipAlreadyUsingConst
-{
-    #[ORM\OrderBy(['createdAt' => Criteria::ASC])]
-    protected \DateTimeInterface $messages;
-}
+$criteria = new Criteria();
+$criteria->orderBy(['asc' => \Doctrine\Common\Collections\Order::Ascending, 'desc' => \Doctrine\Common\Collections\Order::Descending]);
+
 ?>
diff --git a/rules/CodeQuality/Rector/Property/OrderByKeyToClassConstRector.php b/rules/CodeQuality/Rector/Property/OrderByKeyToClassConstRector.php
@@ -5,10 +5,8 @@
 namespace Rector\Doctrine\CodeQuality\Rector\Property;
 
 use PhpParser\Node;
-use PhpParser\Node\Attribute;
 use PhpParser\Node\Expr\Array_;
 use PhpParser\Node\Scalar\String_;
-use PhpParser\Node\Stmt\Property;
 use Rector\Rector\AbstractRector;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
@@ -21,31 +19,30 @@ final class OrderByKeyToClassConstRector extends AbstractRector
     public function getRuleDefinition(): RuleDefinition
     {
         return new RuleDefinition(
-            'Replace OrderBy Attribute ASC/DESC with class constant from Criteria',
+            'Replace OrderBy Attribute ASC/DESC with enum Ordering from Criteria',
             [
                 new CodeSample(
                     <<<'CODE_SAMPLE'
-use Doctrine\ORM\Mapping as ORM;
+                    <?php
 
-class ReplaceOrderByAscWithClassConstant
-{
-    #[ORM\OrderBy(['createdAt' => 'ASC'])]
-    protected \DateTimeInterface $messages;
-}
-?>
-CODE_SAMPLE
+                    use Doctrine\Common\Collections\Criteria;
+
+                    $criteria = new Criteria();
+                    $criteria->orderBy(['someProperty' => 'ASC', 'anotherProperty' => 'DESC']);
 
+                    ?>
+                    CODE_SAMPLE
                     ,
                     <<<'CODE_SAMPLE'
-use Doctrine\ORM\Mapping as ORM;
+                    <?php
 
-class ReplaceOrderByAscWithClassConstant
-{
-    #[ORM\OrderBy(['createdAt' => \Doctrine\Common\Collections\Criteria::ASC])]
-    protected \DateTimeInterface $messages;
-}
-?>
-CODE_SAMPLE
+                    use Doctrine\Common\Collections\Criteria;
+
+                    $criteria = new Criteria();
+                    $criteria->orderBy(['someProperty' => \Doctrine\Common\Collections\Order::Ascending, 'anotherProperty' => \Doctrine\Common\Collections\Order::Descending]);
+
+                    ?>
+                    CODE_SAMPLE
                 ),
             ]
         );
@@ -56,56 +53,87 @@ class ReplaceOrderByAscWithClassConstant
      */
     public function getNodeTypes(): array
     {
-        return [Property::class];
+        return [Node\Expr\MethodCall::class];
     }
 
     /**
-     * @param Property $node
+     * @param Node\Expr\MethodCall $node
      */
     public function refactor(Node $node): ?Node
     {
-        $nodeAttribute = null;
-        foreach ($node->attrGroups as $attrGroup) {
-            foreach ($attrGroup->attrs as $attr) {
-                if ($attr->name->toString() === 'Doctrine\ORM\Mapping\OrderBy') {
-                    $nodeAttribute = $attr;
-                    break 2;
-                }
-            }
-        }
-
-        // If Attribute is not OrderBy, return null
-        if (! $nodeAttribute instanceof Attribute) {
+        // TODO this rule should not apply on PHP versions earlier that 8.1.0 and doctrine collection greater that 2.2.0
+        if (! $node->name instanceof Node\Identifier) {
             return null;
         }
 
-        if (! isset($nodeAttribute->args[0])) {
+        if ((string) $node->name !== 'orderBy') {
             return null;
         }
 
-        if (! $nodeAttribute->args[0]->value instanceof Array_) {
+        if (count($node->args) < 1) {
             return null;
         }
 
-        if (! isset($nodeAttribute->args[0]->value->items[0])) {
+        // TODO find a way to ensure that the 'orderBy' method does apply on a \Doctrine\Common\Collections\Criteria instance
+
+        $arg = $node->args[0];
+        if (! $arg instanceof Node\Arg) {
             return null;
         }
 
-        if (! $nodeAttribute->args[0]->value->items[0]->value instanceof String_) {
+        if (! $arg->value instanceof Array_) {
             return null;
         }
 
-        // If Attribute value from key is not `ASC` or `DESC`, return null
-        if (! in_array($nodeAttribute->args[0]->value->items[0]->value->value, ['ASC', 'asc', 'DESC', 'desc'], true)) {
-            return null;
+        $nodeHasChange = false;
+        // we parse the array here
+        foreach ($arg->value->items as $key => $item) {
+            if ($item === null) {
+                continue;
+            }
+
+            if ($item->value instanceof String_) {
+                $value = $item->value->value;
+                if ($value === 'ASC' || $value === 'asc') {
+                    $node->args[0]->value->items[$key]->value = $this->nodeFactory->createClassConstFetch(
+                        'Doctrine\Common\Collections\Order',
+                        'Ascending'
+                    );
+                    $nodeHasChange = true;
+                } elseif ($value === 'DESC' || $value === 'desc') {
+                    $node->args[0]->value->items[$key]->value = $this->nodeFactory->createClassConstFetch(
+                        'Doctrine\Common\Collections\Order',
+                        'Descending'
+                    );
+                    $nodeHasChange = true;
+                }
+            } elseif ($item->value instanceof Node\Expr\ClassConstFetch) {
+                if (! in_array('Criteria', $item->value->class->getParts())) {
+                    // TODO find a way to identify
+                    continue;
+                }
+
+                if ($item->value->name->toString() === 'ASC') {
+                    $node->args[0]->value->items[$key]->value = $this->nodeFactory->createClassConstFetch(
+                        'Doctrine\Common\Collections\Order',
+                        'Ascending'
+                    );
+                    $nodeHasChange = true;
+                } elseif ($item->value->name->toString() === 'DESC') {
+                    $node->args[0]->value->items[$key]->value = $this->nodeFactory->createClassConstFetch(
+                        'Doctrine\Common\Collections\Order',
+                        'Descending'
+                    );
+                    $nodeHasChange = true;
+                }
+            }
+
         }
 
-        $upper = strtoupper($nodeAttribute->args[0]->value->items[0]->value->value);
-        $nodeAttribute->args[0]->value->items[0]->value = $this->nodeFactory->createClassConstFetch(
-            'Doctrine\Common\Collections\Criteria',
-            $upper
-        );
+        if ($nodeHasChange) {
+            return $node;
+        }
 
-        return $node;
+        return null;
     }
 }
diff --git a/tests/Set/DoctrineCOLLECTION22Set/Fixture/asc_desc_in_attributes_not_replaced.php.inc b/tests/Set/DoctrineCOLLECTION22Set/Fixture/asc_desc_in_attributes_not_replaced.php.inc
@@ -0,0 +1,12 @@
+<?php
+
+class DummyEntity {
+
+    #[ORM\OneToMany(targetEntity: AnotherEntity::class, mappedBy: 'dummy')]
+    #[ORM\OrderBy(["id" => \Doctrine\Common\Collections\Criteria::ASC])]
+    public \Doctrine\Common\Collections\Collection $properties;
+
+    #[ORM\OneToMany(targetEntity: YetAnotherEntity::class, mappedBy: 'dummy')]
+    #[ORM\OrderBy(["id" => \Doctrine\Common\Collections\Criteria::DESC])]
+    public \Doctrine\Common\Collections\Collection $otherProperties;
+}