Transition to using reflection-based, direct field access only

[mpdude]

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:

• Selectable should compare field values, not call getters for initialized collections orm#11160
• DCOM-275: Collections\Expr\ClosureExpressionVisitor should support private properties as well as SQL visitors do. #170
• DDC-2838: Leaky abstraction when applying Criteria to hydrated/non-hydrated PersistentCollection orm#3591
• Maybe Incompatibilities between lazy and initialized collections 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.

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 is also what @Ocramius already suggested in this comment.

For reference, here is a list of discussions around which style of accessors, getters, issers, public access etc. should be used or not used – in the future, the answer would be "only direct state (raw property value) matters".

• ClosureExpressionVisitor can not handle methods without the "get" prefix #276
• Allow accessing fields with a method of the same name #263
• can access the field of a object through same name method #149
• Add check in ClosureExpressionVisitor to use literal public method if available #135
• ClosureExpressionVisitor not using public method matching field name #134
• Add same field name method accessor support to ClosureExpressionVisitor#getObjectFieldValue. #95
• Add ability to use custom method name in ClosureExpressionVisitor #62

Migration path

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).

Caveat

As explained in more detail in #487 (references here after for more better visibility), accessing fields through reflection may bypass initialization of proxy objects in the ORM/ODM. See #487 for more details.

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. 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.

TODO

• Docs update
• PHP 8.4 raw property access (circumvent property hooks)
• After merge and/or release, maybe revisit and update documentation remarks made in Add more detailed caveats for using the Collection filtering API orm#11891

[stof]

I find it weird that so few tests are updated to use the non-deprecated usage, while also having no deprecations reported in the PHPUnit output. I think we need to improve the CI setup regarding deprecations.

The only tests that should trigger our own deprecations should be test covering legacy behaviors (basically tests that would be removed in the next major version when removing legacy code, without regressing code coverage)

[mpdude]

This PR is now based on #473, so it includes deprecation checks in PHPUnit. That was merged, so I rebased.

Please advise about the coding standard issue.

[greg0ire]

Please advise about the coding standard issue.

I think in both cases, you are facing bugs, right? If yes, let's report them (if not reported by somebody else already) and ignore them with a comment.

[mpdude]

PHPCSStandards/PHP_CodeSniffer#734 - maybe no need to report anything?

[mpdude]

@stof Could you give me another round of review here?

Thx!

[greg0ire]

FYI, I'm working on making this easier: doctrine/deprecations#94

[mpdude]

Feedback and deprecations addressed

[greg0ire]

Since this introduces deprecations, you should target 2.4.x

[greg0ire]

This is a breaking change, you should use func_get_args() IMO

https://3v4l.org/Z4ZnM

[mpdude]

Well spotted! fixed

The merge-base changed after approval.

[beberlei]

Great work cleaning this up

[mpdude]

Now rebased against the up-to-date 2.4.x branch

[greg0ire]

Thanks @mpdude !

[W0rma]

I just noticed that this deprecation will always be triggered if f.e. new Criteria(accessRawFieldValues: true) is used.

That's because of the nature of func_num_args() as explained in https://www.php.net/manual/en/function.func-num-args.php:

As of PHP 8.0.0, the func_*() family of functions is intended to be mostly transparent with regard to named arguments, by treating the arguments as if they were all passed positionally, and missing arguments are replaced with their defaults. This function ignores the collection of unknown named variadic arguments. Unknown named arguments which are collected can only be accessed through the variadic parameter.

See also https://3v4l.org/5X4Xu#v8.4.14

Users can work around that by either using new Criteria(firstResult: 0, accessRawFieldValues: true) or Criteria::create(true).

Just FYI if other users might get confused about that as well.