Merge pull request #389 from greg0ire/order-enum

Introduce Order enum

UPGRADE.md

@@ -6,6 +6,22 @@ awareness about deprecated code.
 - Use of our low-overhead runtime deprecation API, details:
   https://github.com/doctrine/deprecations/
 
+# Upgrade to 2.2
+
+## Deprecated string representation of sort order
+
+Criteria orderings direction is now represented by the
+`Doctrine\Common\Collection\Order` enum.
+
+As a consequence:
+
+- `Criteria::ASC` and `Criteria::DESC` are deprecated in favor of
+  `Order::Ascending` and `Order::Descending`, respectively.
+- `Criteria::getOrderings()` is deprecated in favor of `Criteria::orderings()`,
+  which returns `array<string, Order>`.
+- `Criteria::orderBy()` accepts `array<string, string|Order>`, but passing
+  anything other than `array<string, Order>` is deprecated.
+
 # Upgrade to 2.0
 
 ## BC breaking changes

docs/en/expressions.rst

@@ -76,7 +76,7 @@ orderBy
 Sets the ordering of the result of this Criteria.
 
 .. code-block:: php
-    $criteria->orderBy(['name' => Criteria::ASC]);
+    $criteria->orderBy(['name' => Order::Ascending]);
 
 setFirstResult
 --------------

src/ArrayCollection.php

@@ -466,12 +466,12 @@ public function matching(Criteria $criteria)
             $filtered = array_filter($filtered, $filter);
         }
 
-        $orderings = $criteria->getOrderings();
+        $orderings = $criteria->orderings();
 
         if ($orderings) {
             $next = null;
             foreach (array_reverse($orderings) as $field => $ordering) {
-                $next = ClosureExpressionVisitor::sortByField($field, $ordering === Criteria::DESC ? -1 : 1, $next);
+                $next = ClosureExpressionVisitor::sortByField($field, $ordering === Order::Descending ? -1 : 1, $next);
             }
 
             uasort($filtered, $next);

src/Criteria.php

@@ -19,12 +19,15 @@
  */
 class Criteria
 {
-    final public const ASC  = 'ASC';
+    /** @deprecated use Order::Ascending instead */
+    final public const ASC = 'ASC';
+
+    /** @deprecated use Order::Descending instead */
     final public const DESC = 'DESC';
 
     private static ExpressionBuilder|null $expressionBuilder = null;
 
-    /** @var array<string, string> */
+    /** @var array<string, Order> */
     private array $orderings = [];
 
     private int|null $firstResult = null;
@@ -57,7 +60,7 @@ public static function expr()
     /**
      * Construct a new Criteria.
      *
-     * @param array<string, string>|null $orderings
+     * @param array<string, string|Order>|null $orderings
      */
     public function __construct(
         private Expression|null $expression = null,
@@ -151,29 +154,71 @@ public function getWhereExpression()
     /**
      * Gets the current orderings of this Criteria.
      *
+     * @deprecated use orderings() instead
+     *
      * @return array<string, string>
      */
     public function getOrderings()
+    {
+        Deprecation::trigger(
+            'doctrine/collections',
+            'https://github.com/doctrine/collections/pull/389',
+            'Calling %s() is deprecated. Use %s::orderings() instead.',
+            __METHOD__,
+            self::class,
+        );
+
+        return array_map(
+            static fn (Order $ordering): string => $ordering->value,
+            $this->orderings,
+        );
+    }
+
+    /**
+     * Gets the current orderings of this Criteria.
+     *
+     * @return array<string, Order>
+     */
+    public function orderings(): array
     {
         return $this->orderings;
     }
 
     /**
      * Sets the ordering of the result of this Criteria.
      *
-     * Keys are field and values are the order, being either ASC or DESC.
+     * Keys are field and values are the order, being a valid Order enum case.
      *
-     * @see Criteria::ASC
-     * @see Criteria::DESC
+     * @see Order::Ascending
+     * @see Order::Descending
      *
-     * @param array<string, string> $orderings
+     * @param array<string, string|Order> $orderings
      *
      * @return $this
      */
     public function orderBy(array $orderings)
     {
         $this->orderings = array_map(
-            static fn (string $ordering): string => strtoupper($ordering) === self::ASC ? self::ASC : self::DESC,
+            static function (string|Order $ordering): Order {
+                if ($ordering instanceof Order) {
+                    return $ordering;
+                }
+
+                static $triggered = false;
+
+                if (! $triggered) {
+                    Deprecation::trigger(
+                        'doctrine/collections',
+                        'https://github.com/doctrine/collections/pull/389',
+                        'Passing non-Order enum values to %s() is deprecated. Pass Order enum values instead.',
+                        __METHOD__,
+                    );
+                }
+
+                $triggered = true;
+
+                return strtoupper($ordering) === Order::Ascending->value ? Order::Ascending : Order::Descending;
+            },
             $orderings,
         );
 

src/Order.php

@@ -0,0 +1,11 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Common\Collections;
+
+enum Order: string
+{
+    case Ascending  = 'ASC';
+    case Descending = 'DESC';
+}

tests/BaseArrayCollectionTest.php

@@ -6,6 +6,7 @@
 
 use Doctrine\Common\Collections\Collection;
 use Doctrine\Common\Collections\Criteria;
+use Doctrine\Common\Collections\Order;
 use Doctrine\Common\Collections\Selectable;
 use PHPUnit\Framework\TestCase;
 use stdClass;
@@ -334,6 +335,15 @@ public function testMatchingWithSortingPreserveKeys(): void
                 ->matching(new Criteria(null, ['sortField' => Criteria::ASC]))
                 ->toArray(),
         );
+        self::assertSame(
+            [
+                'object2' => $object2,
+                'object1' => $object1,
+            ],
+            $collection
+                ->matching(new Criteria(null, ['sortField' => Order::Ascending]))
+                ->toArray(),
+        );
     }
 
     /**
@@ -443,7 +453,7 @@ public function testMultiColumnSortAppliesAllSorts(): void
         self::assertSame(
             $expected,
             $collection
-                ->matching(new Criteria(null, ['foo' => Criteria::DESC, 'bar' => Criteria::DESC]))
+                ->matching(new Criteria(null, ['foo' => Order::Descending, 'bar' => Order::Descending]))
                 ->toArray(),
         );
     }

tests/CollectionTest.php

@@ -9,6 +9,7 @@
 use Doctrine\Common\Collections\Criteria;
 use Doctrine\Common\Collections\Expr\Expression;
 use Doctrine\Common\Collections\Expr\Value;
+use Doctrine\Common\Collections\Order;
 use RuntimeException;
 use stdClass;
 
@@ -72,7 +73,7 @@ public function testMatchingOrdering(): void
     {
         $this->fillMatchingFixture();
 
-        $col = $this->collection->matching(new Criteria(null, ['foo' => 'DESC']));
+        $col = $this->collection->matching(new Criteria(null, ['foo' => Order::Descending]));
 
         self::assertInstanceOf(Collection::class, $col);
         self::assertNotSame($col, $this->collection);

tests/CriteriaTest.php

@@ -8,6 +8,7 @@
 use Doctrine\Common\Collections\Expr\Comparison;
 use Doctrine\Common\Collections\Expr\CompositeExpression;
 use Doctrine\Common\Collections\ExpressionBuilder;
+use Doctrine\Common\Collections\Order;
 use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
 use PHPUnit\Framework\TestCase;
 
@@ -25,10 +26,10 @@ public function testCreate(): void
     public function testConstructor(): void
     {
         $expr     = new Comparison('field', '=', 'value');
-        $criteria = new Criteria($expr, ['foo' => 'ASC'], 10, 20);
+        $criteria = new Criteria($expr, ['foo' => Order::Ascending], 10, 20);
 
         self::assertSame($expr, $criteria->getWhereExpression());
-        self::assertSame(['foo' => 'ASC'], $criteria->getOrderings());
+        self::assertSame(['foo' => Order::Ascending->value], $criteria->getOrderings());
         self::assertSame(10, $criteria->getFirstResult());
         self::assertSame(20, $criteria->getMaxResults());
     }
@@ -38,10 +39,11 @@ public function testDeprecatedNullOffset(): void
         $expr = new Comparison('field', '=', 'value');
 
         $this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/311');
-        $criteria = new Criteria($expr, ['foo' => 'ASC'], null, 20);
+        $criteria = new Criteria($expr, ['foo' => Order::Ascending], null, 20);
 
         self::assertSame($expr, $criteria->getWhereExpression());
         self::assertSame(['foo' => 'ASC'], $criteria->getOrderings());
+        self::assertSame(['foo' => Order::Ascending], $criteria->orderings());
         self::assertNull($criteria->getFirstResult());
         self::assertSame(20, $criteria->getMaxResults());
     }
@@ -122,13 +124,39 @@ public function testOrWhereWithoutWhere(): void
     public function testOrderings(): void
     {
         $criteria = Criteria::create()
-            ->orderBy(['foo' => 'ASC']);
+            ->orderBy(['foo' => Order::Ascending]);
 
-        self::assertEquals(['foo' => 'ASC'], $criteria->getOrderings());
+        self::assertEquals(['foo' => Order::Ascending], $criteria->orderings());
     }
 
     public function testExpr(): void
     {
         self::assertInstanceOf(ExpressionBuilder::class, Criteria::expr());
     }
+
+    public function testPassingNonOrderEnumToOrderByIsDeprecated(): void
+    {
+        $this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
+        $criteria = Criteria::create()->orderBy(['foo' => 'ASC']);
+    }
+
+    public function testConstructingCriteriaWithNonOrderEnumIsDeprecated(): void
+    {
+        $this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
+        $criteria = new Criteria(null, ['foo' => 'ASC']);
+    }
+
+    public function testUsingOrderEnumIsTheRightWay(): void
+    {
+        $this->expectNoDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
+        Criteria::create()->orderBy(['foo' => Order::Ascending]);
+        new Criteria(null, ['foo' => Order::Ascending]);
+    }
+
+    public function testCallingGetOrderingsIsDeprecated(): void
+    {
+        $criteria = Criteria::create()->orderBy(['foo' => Order::Ascending]);
+        $this->expectDeprecationWithIdentifier('https://github.com/doctrine/collections/pull/389');
+        $criteria->getOrderings();
+    }
 }