[BC break!] Fix DocumentRepository to fit ObjectRepository interface

[lemoinem]

Hello,

https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Persistence/ObjectRepository.php#L60 explicitly document the findBy() method as returning an array of objects.

This interface spec is broken in spec and behaviour by https://github.com/doctrine/mongodb-odm/blob/master/lib/Doctrine/ODM/MongoDB/DocumentRepository.php#L164

This PR aims to fix this. It is quite a major BC break for the MongoDB ODM (as the various updated tests show), but I believe the interface shouldn't have been broken in the first place. We were able to break the interface only because PHP doesn't provide a way to check the type of the return value of a method and because Doctrine Common does not provide implementation tests for its interfaces (I don't intend this remark as a critic, but to describe a fact here 😉). It prevents code base engineered to interact with the Doctrine\Common\Persistence interfaces to crash miserably without clean workarounds.

In order to minimize BC break. I provided the protected method getDocumentPersister() in the DocumentRepository so custom repositories could easily return a Cursor (For example, to load associations through custom repository methods).

If you have any ideas that could fix this without BC Break, I would be happy to ear about it and try as hard as possible to reduce BC breaks and update my PR accordingly.

[jmikola]

We've definitely known about this for a while, but it's been brushed aside as a dirty little secret.

ObjectRepository should technically allow array|Traversable to be returned. Converting MongoDB cursors to arrays up front can drastically affect memory and performance.

@Ocramius, @jwage: Thoughts?

[Ocramius]

@jmikola we can't change that interface in 2.x - I'd rather fix the behavior here in a minor bump. Users should not have relied on the cursor anyway given the interface...

[jmikola]

@jwage: Thoughts?

[jwage]

👍 for this necessary BC break.

[jmikola]

You should add false as the second iterator_to_array() argument here, in case the document has a non-scalar _id value. This will also ensure the user gets back a numerically indexed (i.e. real) array, instead of an associative one.

[jmikola]

@lemoinem: Please make the above changes (feel free to squash commits), and I'll create the changelog entry for this before merging. Thanks!

[lemoinem]

@jmikola Thanks for the feedback. I took your comments into account and double checked my commit, I don't think I forget anything. Here is the new one!

[lemoinem]

Here, I rebased on current master and update the phpDoc (to change @return array to @return array The entities., to be consistent with findAll). I think I'm done with playing around with the branch.

[jmikola]

Manually merged in 64ed807.

I left @return array as-is, as "The entities" seemed redundant.

[tgabi333]

I have two questions in my mind:
1, there is a Cursor::toArray method, wouldn't be better to use that against iterator_to_array, keeping the consistency of result sets?
2, we used to call Cursor::snapshot method in context of findAll like query-s, it could help keeping up eventual consistency

[lemoinem]

1. Cursor::toArray() actually uses iterator_to_array internally. So it shouldn't change anything.
2. Cursor::snapshot(), while highly relevant, seems both impossible to use in the general case (php.net => Currently, snapshot mode may not be used with sorting or explicit hints) and not the right place to use it (I think it would be better off in the Cursor::toArray() method, in which case my 1. comment would be made invalid).

[tgabi333]

@lemoinem , thank for your answer. As i see, calling toArray() would be a good example of DRY principle.

[lemoinem]

@tgabi333 Indeed. To be frank, I didn't even consider using toArray() in my original PR. New PR (#769) addressing this issue.

There is still whether calling Cursor::snapshot() would be a good idea too. The documentation is not explicit as to what the behavior would be when it may not be called... It looks like it's undefined behavior, so I would prefer not to call it when the doc says we shouldn't.

Perhaps including a test to ensure we are under the right conditions? Consistency is actually rather important IMHO.

@jmikola and others, Any thoughts?

[jmikola]

@tgabi333: Thanks for bringing this up.

@lemoinem: snapshot() should be left to the query builder and something that users opt in to use. I think simply switching over to toArray() is sufficient. I'll follow up in #769.

[lemoinem]

@jmikola Yes, I agree, but in the case of findAll or findBy, the user doesn't have access to the QueryBuilder or the Cursor, although they will (and should) expect consistency...

[jmikola]

snapshot() is mainly useful for ensuring you don't iterate over the same result document multiple times. A common example would be if you're iterating through a result set and updating documents as you go through. If one of the updates requires the document to be moved on disk, its index entry might be updated and we could see it a second time later in the iteration.

If we exhaust the result set immediately by converting it to an array, it's less likely a snapshot would be necessary. Granted, it's still possible if another connection changes and moves data around while the driver fetches all of the batches.

[lemoinem]

Fair point! Thanks for the explanation.
@tgabi333 I hope the discussion cleared it up for you too. Thanks for the feedback!

[tgabi333]

Thank you guys.