mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
Query::toIterable() should support mixed result queries with scalar mappings #6642
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @kbond on GitHub (Mar 1, 2021).
This was changed in #8467 (v2.8.2) to fix a memory leak. I'm not sure if this is something that should be "fixed".
@beberlei commented on GitHub (Mar 1, 2021):
@kbond did
->iterate()support this?toIterableis so new, i would be confused about this happening from new code, did you migrate totoIterablealready fromiterate?@alex-dev commented on GitHub (Mar 1, 2021):
I'm pretty sure it was supported by
::iterate().::toIterable()used to support it before #8467.@kbond commented on GitHub (Mar 1, 2021):
I had a workaround for it to work correctly with
->iterate()(anIterableResultdecorator to properly normalize the results), see https://github.com/doctrine/orm/issues/2821#issuecomment-739176588 for reference. I noticed in 2.8.0 when I migrated fromiteratetotoIterableI no longer needed that workaround.@beberlei commented on GitHub (Mar 1, 2021):
The problem is that
toIterableonly supported it because it kept the whole result in memory, which is precisely not what the methods primary use case is about.@alex-dev commented on GitHub (Mar 1, 2021):
How does that even make sense? By nature SQL results are iterative. No matter what hydration (object, array, scalar) is done, the result is first and foremost iterable. Only exception is for joined entities.
So I just don't understand how scalars (which are not even tracked by Doctrine for anything) could cause a problem.
With what has been said. I will allow myself a few assumptions.
ArrayHydratorcan just return an array, so there is no reason to have any problem with tracking or keeping results in memory. Same goes forScalarHydrator.SingleScalarHydratordoes not apply in an iterable context. So, we are left withSimpleObjectHydratorandObjectHydrator. Logicaly, I'd assume the latter to be a more general solution that the former. Skimming the code, this all seems true. So we are left withObjectHydrator.Reading more code... We see
ObjectHydratoris the only one with an actual::cleanupAfterRowIteration(). So there is no reason to prevent any other Hydrator from working with mixed result sets... Well, onlyArrayHydratorwould be useful in an iterable mixed result set, but outright denying it is a problem.ObjectHydrator... Well first and foremost,::cleanupAfterRowIteration()does not fix keeping the result set in memory by itself. UoW also keeps it in memory once hydrated and require a manual clear to work. Nothing in the concepts of anObjectHydratormeans no mixed results allowed. This limit feels like contrived logic in need of a refactor. It is not the only bug plaguing hydration: #8099, #8280, #8394 and #8442.TL:DR
ObjectHydrator.@beberlei commented on GitHub (Mar 1, 2021):
@alex-dev the hydrator is unfortunately quite confusing code sometimes and does some weird things even in the non join case, just to keep the join case supported. Of course refactoring could be an easier solution, but this is a bug fix to fix the memory leak, mixed results were accidently supported because of this bug. If you find a way to support this that would be great, I don't have time to investigate this at the time.
@beberlei commented on GitHub (Mar 1, 2021):
@alex-dev see https://github.com/doctrine/orm/issues/2821 for context
@alex-dev commented on GitHub (Mar 2, 2021):
I would not mind working on a full on refactor of the hydrator internals. I just want to be sure it will be useful in Doctrine 3.0 and not be rewritten code.
@beberlei commented on GitHub (Mar 2, 2021):
No, a full hydrator refactoring is not going to get merged. I meant maybe this is fixable kust for toIterable. @kbond seems to have had a workaround for iterate
@kbond commented on GitHub (Mar 2, 2021):
My workaround basically just normalized the results of
->iterate()- I don't think it will work for->toIterable()as an exception is thrown when the result is mixed.I assumeI see according to #8467, it did not.->iterate()had the same memory leak as->toIterable()?@alex-dev commented on GitHub (Mar 2, 2021):
If memory serves me well,
::iterate()had consistency problems with mixed result keys and always returned a struct-like array rather than returning the object for single entity result.Replacing
::toIterable()with a wrapped call to:iterate()fixing::iteratequirks would be a better approach.I will investigate deeper the issues I had with
::iterate()than PR an implementation of::toIterable().@kbond commented on GitHub (Mar 2, 2021):
This is what my workaround fixed.
@alex-dev commented on GitHub (Mar 2, 2021):
Yes, but I see potential issues in your code.
I just wanna collect all the issues I had with
::iterate()and be sure your workaround covers them all.@kbond commented on GitHub (Mar 2, 2021):
Yes, I'm sure ;)
Assuming the memory leak isn't re-introduced, hopefully that would be acceptable.
@alex-dev commented on GitHub (Mar 22, 2021):
AbstractHydrator::hydrateRowis the problem behind::iterate. To preserve backward compatibility, I can't change it, but leveraging iterate and unwrapping it should be simple.The PR shall supersedes #8467.