Transition to using reflection-based, direct field access only

# Motivation

There are a few issues about differences in behaviour when using the collection filtering API (the `Selectable` interface)  against collections that are database-backed (in ORM, these are `PersistentCollections`) vs. memory-based collections using `ArrayCollection` from this package here.

For example:

* doctrine/orm#11160
* #170
* #149, especially see this [comment](#149 (comment))
* doctrine/orm#3591
* Maybe doctrine/orm#11021

Database-based matching can work on the raw field values only, as those values are persisted to the database and there is no PHP code involved when filtering at the database level.

Memory-based matching currently tries a [series of access methods on the objects](https://www.doctrine-project.org/projects/doctrine-collections/en/2.3/index.html#selectable-methods:~:text=For%20collections%20that%20contain%20objects).

The effects of this may be surprising. For example, with the ORM, it may be fine to filter entities based on the value of `private` or `protected` fields that have no getters. This works as long as a persistent collection is uninitialized. But as soon as it gets initialized, the `ArrayCollection` will require a getter method to be available.

Another (more rare) example is a getter method that does some type of type conversion, like having a `string` field with values like `'y'|'n'` internally but returning a `bool` value from the getter; or, more generally, every type mismatch between the return value of the getter and the field value. Yet another example may be getters that cause side effects 🙈.

# Proposed solution

I discussed with @beberlei at the Doctrine hackathon that the primary use case for this library here was supporting the ORM/ODM use cases. This can be seen in places as `ClosureExpressionVisitor::getObjectFieldValue()` that take a `$field` parameter.

So, although this library here has nothing to do with ORM/ODM mapping, I want to add a migration path here that moves the `doctrine/collection` behaviour closer to the implementation realities of ORM/ODM. This means to ultimately use direct (reflection-based) field access only.

This feature is opt-in and will be activated by passing `accessRawFieldValues: true` to the `Criteria` constructor. The `Criteria` object is what is typically constructed by users in preparation for calling the `Selectable` API, so it seems to be a good fit.

By opting in through this flag, memory-based comparisons and sorting will use direct field access only. Not activating the feature triggers a deprecation notice. In the next major version, direct field access will be the only (default) behaviour.

The `$accessRawFieldValues` can be removed in the next major version (or, possibly, go through another round of deprecations in case when it is still passed, before being eventually removed).

# Remaining edge case

Given an inheritance hierarchy of classes where a multiple classes feature a `private` field of the same name, the downmost field will be picked.

This may differ from Doctrine ORM behaviour when this field is not mapped at all in the ORM and another field (higher up the class hierarchy) is used as the mapped field instead.

We cannot solve this without having access to ORM/ODM mapping metadata at hand, which is not possible from within an `ArrayCollection` that is typically created as [a newable type](https://testing.googleblog.com/2008/10/to-new-or-not-to-new.html). We rather plan to discourage or even prevent this kind of setup (entity class hierarchy with different classes having fields of the same name) at some point when loading and validating metadata.

Author: mpdude

UPGRADE.md

@@ -6,6 +6,18 @@ awareness about deprecated code.
 - Use of our low-overhead runtime deprecation API, details:
   https://github.com/doctrine/deprecations/
 
+# Upgrade to 2.4
+
+## Deprecated accessing fields through other means than raw field access when using the criteria filtering API (the `Doctrine\Common\Collections\Selectable` interface)
+
+Starting with the next major version, the only way to access data when using the criteria filtering 
+API is through direct (reflection-based) access at properties directly, also bypassing property hooks.
+This is to ensure consistency with how the ORM/ODM work. See https://github.com/doctrine/collections/pull/472 for
+the full motivation.
+
+To opt-in to the new behaviour, pass `true` for the `$accessRawFieldValues` parameter when creating a `Criteria`
+object through either `Doctrine\Common\Collections\Criteria::create()` or when calling the `Doctrine\Common\Collections\Criteria` constructor. 
+
 # Upgrade to 2.2
 
 ## Deprecated string representation of sort order

docs/en/index.rst

@@ -364,11 +364,14 @@ You can read more about expressions :ref:`here <expressions>`.
 .. note::
 
     For collections that contain objects, the field name given to ``Comparison`` will
-    lead to various access methods being tried in sequence. For the exact implementation,
-    refer to the ``ClosureExpressionVisitor::getObjectFieldValue()`` method.
-
-    Roughly speaking, for a field named ``field``, the following things will be tried
-    in order:
+    lead to various access methods being tried in sequence. This behavior is deprecated
+    as of v2.4.0. Set the ``$accessRawFieldValues`` parameter in the ``Criteria`` constructor
+    to ``true`` to opt-in to the new behaviour of using direct (reflection-based) field access only.
+    This will be the only option in the next major version.
+
+    Unless you opt in, refer to the ``ClosureExpressionVisitor::getObjectFieldValue()`` method
+    for the exact order of accessors tried. Roughly speaking, for a field named ``field``,
+    the following things will be tried in order:
 
     1. ``getField()``, ``isField()`` and ``field()`` as getter methods
     2. When the object implements a ``__call`` magic method, invoke it

phpcs.xml.dist

@@ -44,4 +44,14 @@
         <exclude-pattern>src/ArrayCollection.php</exclude-pattern>
         <exclude-pattern>src/Collection.php</exclude-pattern>
     </rule>
+
+    <rule ref="PSR2.Classes.PropertyDeclaration.Multiple">
+        <!-- https://github.com/doctrine/collections/pull/472#issuecomment-3384164908 -->
+        <exclude-pattern>tests/TestObjectPropertyHook.php</exclude-pattern>
+    </rule>
+
+    <rule ref="PSR2.Classes.PropertyDeclaration.ScopeMissing">
+        <!-- https://github.com/doctrine/collections/pull/472#issuecomment-3384164908 -->
+        <exclude-pattern>tests/TestObjectPropertyHook.php</exclude-pattern>
+    </rule>
 </ruleset>

src/ArrayCollection.php

@@ -450,11 +450,13 @@ public function slice(int $offset, int|null $length = null)
     /** @phpstan-return Collection<TKey, T>&Selectable<TKey,T> */
     public function matching(Criteria $criteria)
     {
+        $accessRawFieldValues = $criteria->isRawFieldValueAccessEnabled();
+
         $expr     = $criteria->getWhereExpression();
         $filtered = $this->elements;
 
         if ($expr) {
-            $visitor  = new ClosureExpressionVisitor();
+            $visitor  = new ClosureExpressionVisitor($accessRawFieldValues);
             $filter   = $visitor->dispatch($expr);
             $filtered = array_filter($filtered, $filter);
         }
@@ -464,7 +466,7 @@ public function matching(Criteria $criteria)
         if ($orderings) {
             $next = null;
             foreach (array_reverse($orderings) as $field => $ordering) {
-                $next = ClosureExpressionVisitor::sortByField($field, $ordering === Order::Descending ? -1 : 1, $next);
+                $next = ClosureExpressionVisitor::sortByField($field, $ordering === Order::Descending ? -1 : 1, $next, $accessRawFieldValues);
             }
 
             uasort($filtered, $next);

src/Criteria.php

@@ -38,9 +38,12 @@ class Criteria
      *
      * @return static
      */
-    public static function create()
+    public static function create(bool $accessRawFieldValues = false): self
     {
-        return new static();
+        $new                       = new static();
+        $new->accessRawFieldValues = $accessRawFieldValues;
+
+        return $new;
     }
 
     /**
@@ -67,8 +70,16 @@ public function __construct(
         array|null $orderings = null,
         int|null $firstResult = null,
         int|null $maxResults = null,
+        private bool $accessRawFieldValues = false,
     ) {
-        $this->expression = $expression;
+        if (! $accessRawFieldValues) {
+            Deprecation::trigger(
+                'doctrine/collections',
+                'https://github.com/doctrine/collections/pull/472',
+                'Not enabling raw field value access for the Criteria matching API in %s is deprecated. Raw field access will be the only supported method in 3.0',
+                self::class,
+            );
+        }
 
         if ($firstResult === null && func_num_args() > 2) {
             Deprecation::trigger(
@@ -282,4 +293,10 @@ public function setMaxResults(int|null $maxResults)
 
         return $this;
     }
+
+    /** @internal */
+    public function isRawFieldValueAccessEnabled(): bool
+    {
+        return $this->accessRawFieldValues;
+    }
 }

src/Expr/ClosureExpressionVisitor.php

@@ -6,6 +6,8 @@
 
 use ArrayAccess;
 use Closure;
+use Doctrine\Deprecations\Deprecation;
+use ReflectionClass;
 use RuntimeException;
 
 use function array_all;
@@ -18,11 +20,14 @@
 use function method_exists;
 use function preg_match;
 use function preg_replace_callback;
+use function sprintf;
 use function str_contains;
 use function str_ends_with;
 use function str_starts_with;
 use function strtoupper;
 
+use const PHP_VERSION_ID;
+
 /**
  * Walks an expression graph and turns it into a PHP closure.
  *
@@ -31,6 +36,11 @@
  */
 class ClosureExpressionVisitor extends ExpressionVisitor
 {
+    public function __construct(
+        private readonly bool $accessRawFieldValues = false,
+    ) {
+    }
+
     /**
      * Accesses the field of a given object. This field has to be public
      * directly or indirectly (through an accessor get*, is*, or a magic
@@ -40,19 +50,30 @@ class ClosureExpressionVisitor extends ExpressionVisitor
      *
      * @return mixed
      */
-    public static function getObjectFieldValue(object|array $object, string $field)
+    public static function getObjectFieldValue(object|array $object, string $field, bool $accessRawFieldValues = false)
     {
         if (str_contains($field, '.')) {
             [$field, $subField] = explode('.', $field, 2);
-            $object             = self::getObjectFieldValue($object, $field);
+            $object             = self::getObjectFieldValue($object, $field, $accessRawFieldValues);
 
-            return self::getObjectFieldValue($object, $subField);
+            return self::getObjectFieldValue($object, $subField, $accessRawFieldValues);
         }
 
         if (is_array($object)) {
             return $object[$field];
         }
 
+        if ($accessRawFieldValues) {
+            return self::getNearestFieldValue($object, $field);
+        }
+
+        Deprecation::trigger(
+            'doctrine/collections',
+            'https://github.com/doctrine/collections/pull/472',
+            'Not enabling raw field value access for %s is deprecated. Raw field access will be the only supported method in 3.0',
+            __METHOD__,
+        );
+
         $accessors = ['get', 'is', ''];
 
         foreach ($accessors as $accessor) {
@@ -96,21 +117,50 @@ public static function getObjectFieldValue(object|array $object, string $field)
         return $object->$field;
     }
 
+    private static function getNearestFieldValue(object $object, string $field): mixed
+    {
+        $reflectionClass = new ReflectionClass($object);
+
+        while ($reflectionClass && ! $reflectionClass->hasProperty($field)) {
+            $reflectionClass = $reflectionClass->getParentClass();
+        }
+
+        if ($reflectionClass === false) {
+            throw new RuntimeException(sprintf('Field "%s" does not exist in class "%s"', $field, $object::class));
+        }
+
+        $property = $reflectionClass->getProperty($field);
+
+        if (PHP_VERSION_ID >= 80400) {
+            return $property->getRawValue($object);
+        }
+
+        return $property->getValue($object);
+    }
+
     /**
      * Helper for sorting arrays of objects based on multiple fields + orientations.
      *
      * @return Closure
      */
-    public static function sortByField(string $name, int $orientation = 1, Closure|null $next = null)
+    public static function sortByField(string $name, int $orientation = 1, Closure|null $next = null, bool $accessRawFieldValues = false)
     {
+        if (! $accessRawFieldValues) {
+            Deprecation::trigger(
+                'doctrine/collections',
+                'https://github.com/doctrine/collections/pull/472',
+                'Not enabling raw field value access for %s is deprecated. Raw field access will be the only supported method in 3.0',
+                __METHOD__,
+            );
+        }
+
         if (! $next) {
             $next = static fn (): int => 0;
         }
 
-        return static function ($a, $b) use ($name, $next, $orientation): int {
-            $aValue = ClosureExpressionVisitor::getObjectFieldValue($a, $name);
-
-            $bValue = ClosureExpressionVisitor::getObjectFieldValue($b, $name);
+        return static function ($a, $b) use ($name, $next, $orientation, $accessRawFieldValues): int {
+            $aValue = ClosureExpressionVisitor::getObjectFieldValue($a, $name, $accessRawFieldValues);
+            $bValue = ClosureExpressionVisitor::getObjectFieldValue($b, $name, $accessRawFieldValues);
 
             if ($aValue === $bValue) {
                 return $next($a, $b);
@@ -129,34 +179,34 @@ public function walkComparison(Comparison $comparison)
         $value = $comparison->getValue()->getValue();
 
         return match ($comparison->getOperator()) {
-            Comparison::EQ => static fn ($object): bool => self::getObjectFieldValue($object, $field) === $value,
-            Comparison::NEQ => static fn ($object): bool => self::getObjectFieldValue($object, $field) !== $value,
-            Comparison::LT => static fn ($object): bool => self::getObjectFieldValue($object, $field) < $value,
-            Comparison::LTE => static fn ($object): bool => self::getObjectFieldValue($object, $field) <= $value,
-            Comparison::GT => static fn ($object): bool => self::getObjectFieldValue($object, $field) > $value,
-            Comparison::GTE => static fn ($object): bool => self::getObjectFieldValue($object, $field) >= $value,
-            Comparison::IN => static function ($object) use ($field, $value): bool {
-                $fieldValue = ClosureExpressionVisitor::getObjectFieldValue($object, $field);
+            Comparison::EQ => fn ($object): bool => self::getObjectFieldValue($object, $field, $this->accessRawFieldValues) === $value,
+            Comparison::NEQ => fn ($object): bool => self::getObjectFieldValue($object, $field, $this->accessRawFieldValues) !== $value,
+            Comparison::LT => fn ($object): bool => self::getObjectFieldValue($object, $field, $this->accessRawFieldValues) < $value,
+            Comparison::LTE => fn ($object): bool => self::getObjectFieldValue($object, $field, $this->accessRawFieldValues) <= $value,
+            Comparison::GT => fn ($object): bool => self::getObjectFieldValue($object, $field, $this->accessRawFieldValues) > $value,
+            Comparison::GTE => fn ($object): bool => self::getObjectFieldValue($object, $field, $this->accessRawFieldValues) >= $value,
+            Comparison::IN => function ($object) use ($field, $value): bool {
+                $fieldValue = ClosureExpressionVisitor::getObjectFieldValue($object, $field, $this->accessRawFieldValues);
 
                 return in_array($fieldValue, $value, is_scalar($fieldValue));
             },
-            Comparison::NIN => static function ($object) use ($field, $value): bool {
-                $fieldValue = ClosureExpressionVisitor::getObjectFieldValue($object, $field);
+            Comparison::NIN => function ($object) use ($field, $value): bool {
+                $fieldValue = ClosureExpressionVisitor::getObjectFieldValue($object, $field, $this->accessRawFieldValues);
 
                 return ! in_array($fieldValue, $value, is_scalar($fieldValue));
             },
-            Comparison::CONTAINS => static fn ($object): bool => str_contains((string) self::getObjectFieldValue($object, $field), (string) $value),
-            Comparison::MEMBER_OF => static function ($object) use ($field, $value): bool {
-                $fieldValues = ClosureExpressionVisitor::getObjectFieldValue($object, $field);
+            Comparison::CONTAINS => fn ($object): bool => str_contains((string) self::getObjectFieldValue($object, $field, $this->accessRawFieldValues), (string) $value),
+            Comparison::MEMBER_OF => function ($object) use ($field, $value): bool {
+                $fieldValues = ClosureExpressionVisitor::getObjectFieldValue($object, $field, $this->accessRawFieldValues);
 
                 if (! is_array($fieldValues)) {
                     $fieldValues = iterator_to_array($fieldValues);
                 }
 
                 return in_array($value, $fieldValues, true);
             },
-            Comparison::STARTS_WITH => static fn ($object): bool => str_starts_with((string) self::getObjectFieldValue($object, $field), (string) $value),
-            Comparison::ENDS_WITH => static fn ($object): bool => str_ends_with((string) self::getObjectFieldValue($object, $field), (string) $value),
+            Comparison::STARTS_WITH => fn ($object): bool => str_starts_with((string) self::getObjectFieldValue($object, $field, $this->accessRawFieldValues), (string) $value),
+            Comparison::ENDS_WITH => fn ($object): bool => str_ends_with((string) self::getObjectFieldValue($object, $field, $this->accessRawFieldValues), (string) $value),
             default => throw new RuntimeException('Unknown comparison operator: ' . $comparison->getOperator()),
         };
     }

tests/ArrayCollectionTestCase.php

@@ -283,11 +283,8 @@ public function testGet(): void
 
     public function testMatchingWithSortingPreserveKeys(): void
     {
-        $object1 = new stdClass();
-        $object2 = new stdClass();
-
-        $object1->sortField = 2;
-        $object2->sortField = 1;
+        $object1 = new TestObjectPrivatePropertyOnly(2);
+        $object2 = new TestObjectPrivatePropertyOnly(1);
 
         $collection = $this->buildCollection([
             'object1' => $object1,
@@ -304,7 +301,7 @@ public function testMatchingWithSortingPreserveKeys(): void
                 'object1' => $object1,
             ],
             $collection
-                ->matching(new Criteria(null, ['sortField' => Order::Ascending]))
+                ->matching(new Criteria(null, ['fooBar' => Order::Ascending], 0, accessRawFieldValues: true))
                 ->toArray(),
         );
     }
@@ -333,7 +330,7 @@ public function testLegacyMatchingWithSortingPreserveKeys(): void
                 'object1' => $object1,
             ],
             $collection
-                ->matching(new Criteria(null, ['sortField' => Criteria::ASC]))
+                ->matching(new Criteria(null, ['sortField' => Order::Ascending]))
                 ->toArray(),
         );
     }
@@ -358,7 +355,7 @@ public function testMatchingWithSlicingPreserveKeys(
         self::assertSame(
             $slicedArray,
             $collection
-                ->matching(new Criteria(null, null, $firstResult, $maxResult))
+                ->matching(new Criteria(null, null, $firstResult, $maxResult, accessRawFieldValues: true))
                 ->toArray(),
         );
     }
@@ -480,7 +477,7 @@ public function testMultiColumnSortAppliesAllSorts(): void
         self::assertSame(
             $expected,
             $collection
-                ->matching(new Criteria(null, ['foo' => Order::Descending, 'bar' => Order::Descending]))
+                ->matching(new Criteria(null, ['foo' => Order::Descending, 'bar' => Order::Descending], accessRawFieldValues: true))
                 ->toArray(),
         );
     }