-
-
Notifications
You must be signed in to change notification settings - Fork 188
Description
When enabling the reflection-based access of fields (#472), proxy objects might not get initialized. This can lead to subtle changes in behavior.
Before #472 (or without activating this feature), access through getters or public properties triggered typical proxy initialization logic.
Motivating example
I am not sure whether a Collection as provided by Doctrine ORM for to-many-associations can contain uninitalized proxies in the first place. In my particular case, the proxies come from a situation as follows:
I have an Entity Publisher, with a to-many association on Book, with a to-many association on Author.
There is a method Publisher::getAuthorsOrderedByYearOfBirth() along the lines of:
return $this->books
->map(static fn (Book $b): Author => $b->getAuthor())
->matching(Criteria::create(true)->orderBy(['birthdate' => Order::Ascending]));(Ignore potentially duplicate authors for the sake of simplicity.)
Here, Book::getAuthor() may be the source of an uninitialized Author proxy (created by the ORM). Accessing the birthdate field through reflection on these proxies leads to null values without triggering initialization, effectively returning the result with not particular sort order applied.
Possible solution (1)
We could include proxy detection and initialization logic in ClosureExpressionVisitor::getObjectFieldValue(), which might be as simple as
if ($object instanceof \Doctrine\Persistence\Proxy) {
$object->__load();
}I assume this would fix the problem for all ORM/ODM scenarios pre native lazy objects. Rumor has it that native lazy objects will be initialized through reflection-based access as well, so no problem there.
Pro:
- Might transparently fix the problem for most of the practical situations.
Downsides:
- Very much tailored to the ORM/ODM use cases.
doctrine/persistenceis currently not a dependency of this library here. Unsure whether it needs to be or should become for that check. On the short run technically no, we don't need it for theinstanceofcheck. Conceptually, maybe yes – theoretically, the next major version of it might not include the__loadmethod anymore.
Possible solution (2)
Raise awareness and tell users to fix the problem in userland:
return $this->books
->map(static function (Book $b): Author {
$author = $b->getAuthor();
if ($author instanceof Proxy) {
$author->__load();
}
)
->matching(Criteria::create(true)->orderBy(['birthdate' => Order::Ascending]);Pro:
- Nothing to do here (apart from docs update, maybe)
Downside:
- Higher WTF ratios, bad DX, subtle/surprising bugs
- Slight increase in global code pollution through boilerplate code added.