Selectable should compare field values, not call getters for initialized collections

[mpdude]

This revisits #3591 which I think is not fixed correctly.

When using the Selectable API to filter elements from a collection, the result depends on the initialization state of the collection.

When the collection is uninitialized, the filtering happens in the database at the SQL level. The given field name is translated to a database column name and a database column-based lookup (WHERE) is performed.

When the collection is initialized, we go through hoops to derive potential getter/isser names and/or access $object->$field as a last resort, but completely fail on private properties without getter method access.

To the user, it should not make a difference whether a collection is initialized. We should use plain, direct field access (through reflection?) since that is what most closely resembles the database-based lookup. I think this is what Marco refers to in doctrine/collections#149 (comment) as "rely only on state for lookups".

I am not aware of any other places where the ORM would be concerned with deriving accessor names, which makes this an additional inconsistency.

Obviously, preferring code (getter based) access is not an option for uninitialized collections.

Note that even for plain "state" lookups, there is a remaining chance for issues when DBAL types come into play that do database-to-PHP value conversion. A SQL level column-value comparison might not come to the same result as a PHP-level comparison for certain DBAL type implementations. Anyways.

Suggestions needed

• How can we fix this, given that most of the Selectable API lives in doctrine/collections and uses static method calls? Is this an ORM issue or a collections issue?
• Are we risking BC breaks, so this fix needs to come as an opt-in switch with a deprecation?
• If so, is it too late in 2.x now?

Related:

• DCOM-275: Collections\Expr\ClosureExpressionVisitor should support private properties as well as SQL visitors do. collections#170

[greg0ire]

Are we risking BC breaks, so this fix needs to come as an opt-in switch with a deprecation?

That would be the case if any of the getter/isser has side effects. I don't know if we have documentation where we forbid that.

[greg0ire]

If so, is it too late in 2.x now?

You can do a breaking change only on 3.x, and you have until Feb 1st to do that. Even after Feb 1st, it will still be possible (and encouraged) to add deprecation layers to 2.x.

[greg0ire]

How can we fix this, given that most of the Selectable API lives in doctrine/collections and uses static method calls? Is this an ORM issue or a collections issue?

If feels like a doctrine/collections issue, and might affect the ODM as well, I don't know. Maybe try to reproduce the issue with the collections API only?
If what you are worried is the upgrade path, I'd say a static property on ClosureExpressionVisitor telling whether to check getters and issers might help (but it would be global to the application, and libraries probably shouldn't interfere with it, just the end user). And then we remove it as well as the getter lookup in collections 3?

[mpdude]

Let's ignore for the time being that I have no idea what the deprecation strategy/BC might look like and how to fix this as a cross-library issue...

In order to deal with private fields, you need to know the class in which the field is declared. For the ORM, field names are currently unique, so for a given field name, we can figure that out... as long as we have access to the ClassMetadata or the ClassMetadataFactory.

We probably can get hold of that in PersistentCollections.

But, what about entity classes that have just been created, where users assigned $this->collection = new ArrayCollection() and they then use the Selectable API? How would that ArrayCollection be able to obtain class metadata? Through static calls to locate a ManagerRegistry, to find the right EntityManager for the class containing the field? In addition, ArrayCollection comes from doctrine/collections, where we'd not want to have code that is concerned with such details of the ORM.