Use CursorInterface type instead of Cursor

[soyuka]

Q	A
Type	improvement
BC Break	no
Fixed issues	#2395

Summary

I still have the same issue, I want to move from execute() to getAggregation()->getIterator() but this class is completely locked in and we have no way to unit test this:

https://github.com/api-platform/core/blob/92a81f024541054b9322e7457b75c721261e14e0/src/Doctrine/Odm/State/ItemProvider.php#L74

The assertion above is fine I can disable it, but in fact I wonder why you wouldn't do instanceof CursorInterface, what if someone implements another Cursor? We can't use the Aggregation type, nor the AggregationBuilder (as the signature of getAggregation forces the use of the final class Aggregation) nor the DocumentRepository, which forces the use of the AggregationBuilder...

Can you help providing a solution for library maintainers? Thanks!

[malarzm]

The assertion above is fine I can disable it, but in fact I wonder why you wouldn't do instanceof CursorInterface, what if someone implements another Cursor?

My guess is that at the time a Cursor was the only thing the underlying library could have returned. But that'd be best confirmed by somebody with proper driver/library knowledge like @alcaeus or @jmikola :)

[soyuka]

Actually I'd love to change the assert as well:

https://github.com/doctrine/mongodb-odm/blob/d3c7dce09d5a49989e1f216b93b70cd67d43de61/lib/Doctrine/ODM/MongoDB/Aggregation/Aggregation.php#L57C1-L59

lmk if these changes are possible, thanks.

[jmikola]

My guess is that at the time a Cursor was the only thing the underlying library could have returned.

I'm not sure how much this question pertains to the Aggregate operation itself, but I can point out that it historically returned a Traversable. That was recently changed in mongodb/mongo-php-library@5aff53f since a non-cursor response is no longer possible on MongoDB 3.6+ (required by the current driver/library version).

Also, nothing has historically implemented CursorInterface (apart from MongoDB\Driver\Cursor), but that will change once codecs are implemented (see: mongodb/mongo-php-library#1140).

With that in mind, I think it may make sense to use CursorInterface instead of Cursor in ODM, although there's likely no rush as alternative implementations won't be returned until ODM (or any other library) starts making use of codecs, which would be a separate initiative.

@alcaeus can speak more to this once he's back from holiday next week.

[jmikola]

I will note that while Cursor does implement both CursorInterface and Iterator, CursorInterface itself is only Traversable. Depending how ODM uses the $cursor object here, this change might conflict with static analysis checks.

[alcaeus]

With that in mind, I think it may make sense to use CursorInterface instead of Cursor in ODM, although there's likely no rush as alternative implementations won't be returned until ODM (or any other library) starts making use of codecs, which would be a separate initiative.

Yes, absolutely. We haven't had a reason for making this change, but this reason is coming with the addition of codecs in PHPLIB, which will result in return values that are not MongoDB\Driver\Cursor but will offer its API. Please note that for historic reasons, CursorInterface does not implement Iterator, but the Cursor class does. The correct replacement for any Cursor type would thus be CursorInterface & Iterator.

[soyuka]

I did the change, do you think that we could also get rid of the assert($cursor instanceof Cursor); ?

[alcaeus]

I did the change, do you think that we could also get rid of the assert($cursor instanceof Cursor); ?

Yes - the assertion can be removed. IIRC this was there for static analysis reasons.

[soyuka]

bumped and rebased against 2.6, lmk if it needs to target something else

[divine]

Hello,

Can this be pushed forward and reviewed please? It’s me who reported this issue with the APP in api-platform/core#5705.

MongoDB works great for our project but this depreciation notices is a bit scary to use in a stable framework.

cc @alcaeus @jmikola @malarzm

Thanks!

[malarzm]

FWIW branch 2.6.x requires php ^8.0 while this is valid starting with 8.1. But maybe we could bump requirement to 8.1, 8.0 usage in 2.5.x is rather minimal: https://packagist.org/packages/doctrine/mongodb-odm/php-stats#2.5

[alcaeus]

No objections to bumping to PHP 8.1 from my end.

[malarzm]

Bumped in #2556

[alcaeus]

The change itself looks good, but static analysis is not too happy. We might have to leave the assertions in place to quiet down PHPStan.

[soyuka]

sure, should I update this?

[alcaeus]

@soyuka I fixed issues, but since you used the 2.5.x branch in your fork I didn't want to force-push to it. I created #2579 instead.