Make paginator covariant

[VincentLanglet]

see https://phpstan.org/blog/whats-up-with-template-covariant
see https://psalm.dev/docs/annotating_code/templated_annotations/

A method which accepts Paginator<Animal> should be able to accept Paginator<Cat>.

I received the error

Template type T is declared as covariant, but occurs in invariant position in return type of method Doctrine\ORM\Tools\Pagination\Paginator::getIterator().

It's because the template of ArrayIterator is not covariant.
But the template for Traversable is covariant.

It makes sens to use Traversable instead of ArrayIterator as return type because

• IteratorAggregate::getIterator() use Traversable as return type
• it doesn't make sens to allow adding elements to this Iterator

ArrayIterator implements SeekableIterator, ArrayAccess, Serializable, Countable
the issue is the ArrayAccess.

If you preferred I can use somethink like

@psalm-return Traversable<array-key, T>&Countable
@psalm-return Traversable<array-key, T>&Serializable&Countable
@psalm-return SeekableIterator<array-key, T>&Serializable&Countable
...

Any suggestion @greg0ire ?

[greg0ire]

No suggestion yet, but can you add a piece of code under tests/Doctrine/StaticAnalysis to demonstrate the benefit and avoid regressions? It will also help understanding the PR.

[VincentLanglet]

No suggestion yet, but can you add a piece of code under tests/Doctrine/StaticAnalysis to demonstrate the benefit and avoid regressions? It will also help understanding the PR.

I added as much tests as possible ^^

[VincentLanglet]

This wouldn't be possible if Paginator wasn't covariant.

[VincentLanglet]

Passing a Paginator to getFirstAnimal wouldn't be possible if Paginator wasn't covariant

[greg0ire]

Please squash your commits, and retarget to 2.14.x: this is an improvement and there is a small BC-break.

[greg0ire]

If you preferred I can use something like…

actually, Traversable is fine, because it's exactly what we are supposed to return to fulfill the contract. Since we do not intend people to use getIterator() directly, but rather indirectly through foreach, it makes sense to give as little detail as possible on the type. That way, we can change the implementation and something else if we want to.

[VincentLanglet]

Please squash your commits, and retarget to 2.14.x: this is an improvement and there is a small BC-break.

Done

[mitelg]

hey @VincentLanglet

sorry for coming back after this MR is merged a long time ago 😬

we want to update to the recent 2.14.1 version. our problem is that we are using $paginator->getIterator()->getArrayCopy() a lot 😅

Just to be sure: the migration path would simply be exchanging this with iterator_to_array($paginator) ? the output array is the same in my first tests, just wanted to be doubled checked 😄

thanks in advance ✌️

[VincentLanglet]

$paginator->getIterator()->getArrayCopy() a lot 😅

Just to be sure: the migration path would simply be exchanging this with iterator_to_array($paginator) ?

Sure.