mongodb / mongo-php-driver

The Official MongoDB PHP driver
https://pecl.php.net/package/mongodb
Apache License 2.0
889 stars 204 forks source link

Allow to extend `MongoDB\Driver\Cursor` #754

Closed abellion closed 6 years ago

abellion commented 6 years ago

Description

I would like to be able to set some metadata to the retrieved documents when using find(), findOne() or aggregate().

To do so, a solution is to wrap the MongoDB\Driver\Cursor inside an IteratorIterator and set the metadata to the documents when the current() method is called.

The downside of this approach is that the returned iterator is not an instance of the MongoDB\Driver\Cursor class so it may break some type hints and it doesn't have its methods.

Proposal

If the MongoDB\Driver\Cursor were an instance of Iterator, it would be easy to extend it and take advantage of its current() method while not loosing type hinting and the other methods the Cursor class provides.

jmikola commented 6 years ago

Having MongoDB\Driver\Cursor implement Iterator still would not allow it to be extended, as the the class is final. We're not keen on removing final from the core classes due to (a) issues we had with extending core classes in the legacy ext-mongo extension and (b) promoting composition over inheritance. As an internal class, we already need to provide the iterator handlers, which IteratorIterator is already capable of exposing as PHP methods accessible directly from userland.

If I understand correctly, the real need here is having the extension provide an interface implemented by both MongoDB\Driver\Cursor and available for a userland IteratorIterator sub-class to facilitate polymorphism in any application code expecting a cursor (assuming it type hints with that interface instead of MongoDB\Driver\Cursor).

Interfaces for driver classes was something we considered early on in PHPC-378 but there was never a pressing need for it. IIRC, I believe I also spoke to @alcaeus about whether future versions of Doctrine ODM might need such an interface, as its current API does have a class that composes a cursor, but we were unsure. Presently, I expect users may be using Traversable to pass cursors and iterators interchangeably, since that's the common interface; however, that doesn't help if the code also expects to be able to call methods like MongoDB\Driver\Cursor::isDead().

Note that we did end up adding interfaces for BSON classes (PHPC-640), since we were able to demonstrate a need for it (and it will become more relevant as additional BSON features are implemented). I'm happy to consider adding a MongoDB\Driver\CursorInterface if you think that will address your use case. I expect it would look like the following:

namespace MongoDB\Driver;

interface CursorInterface extends \Traversable
{
    function getId(): CursorId;
    function getServer(): Server;
    function isDead(): boolean;
    function setTypeMap(array $typeMap): void;
    function toArray(): array;
}
abellion commented 6 years ago

Thank you for this detailed answer. My first thought was about inheritance to benefit directly from the parent's methods, but if keeping the Cursor class as final is legit, never mind ; an interface would be perfect !

jmikola commented 6 years ago

I've opened PHPC-1123 to track this feature request. I reckon it's something we can add for 1.5.0.

In the meantime, I might suggest relying on Traversable if you have functions that expect either a MongoDB\Driver\Cursor or a cursor wrapped with an IteratorIterator. That will also be forward-compatible with an interface in 1.5.0, since it will end up extending Traversable. This should work fine provided those functions don't depend on calling one of the specific cursor methods (e.g. setTypeMap()). Even then, you may be able to work around that with instanceof to detect cases where the Traversable is also a MongoDB\Driver\Cursor. It's not ideal, but it may be a reasonable work-around until 1.5.0 can address this.

I'm going to close this out, but feel free to follow that issue in JIRA for updates.

abellion commented 6 years ago

Thank you !