PHPLIB-954: Add return types to all methods

[alcaeus]

PHPLIB-954

This PR:

• adds testing with the 2.0 branch for PHPC,
• adds native return types for all methods, removing ReturnTypeWillChange attributes where present
• replaces all Cursor types with CursorInterface
• removes the compatibility layer introduced for the CursorId deprecation

[alcaeus]

Left some explanations. A number of the psalm errors are due to vimeo/psalm#10019, which doesn't correctly infer the return type or (object) ['foo' => 'bar'] as stdClass but instead assumes object. I'll take a look at the other psalm errors regarding codec generics, but feel free to leave feedback in the meantime.

[alcaeus]

We don't have any plans for this yet, but I propose having CursorInterface extend Iterator to avoid the need for an intersection type here. Currently, only the Cursor class implements Iterator (added in PHPC-1691, but not added to CursorInterface due to BC constraints).

[GromNaN]

CursorInterface already extends Traversable, which is better IMO. We should not require the Iterator interface for custom cursor classes.
In case people really need the Iterator API, they can wrap it in an IteratorIterator.

Suggested change

	public function aggregate(array $pipeline, array $options = []): CursorInterface&Iterator
	public function aggregate(array $pipeline, array $options = []): CursorInterface

[alcaeus]

A CursorInterface type without an intersection with Iterator won't allow you to call $cursor->next(); - that's an issue we faced in ODM. Ideally, we'd fix this in PHPC by having CursorInterface extend Iterator, but until then we're stuck with this solution.

[GromNaN]

There is always solutions to wrap a Transversable that is not an Iterator.
With a Generator (see UnrewindableIterator, IteratorIterator or IteratorAggregate::getIterator()

[jmikola]

Providing some additional context and throwing in my two cents:

• We added Iterator to Cursor in PHPC-1691 in order to comply with PHP 8.
• CursorInterface was introduced in PHPC-1123 to allow other classes to fill in for a Cursor, which cannot be extended. AFAIK, the only existing use case for this within our own driver is CodecCursor.
• CodecCursor currently implements both CursorInterface and Iterator, so that would not be negatively impacted by this change.

While conditionally wrapping a CursorInterface to get access to Iterator methods is always possible, it may be tedious. Given that CursorInterface is intended to be used for classes that compose a Cursor, it seems a small ask for the wrapping class to also implement Iterator.

I'm not sure what the use case would be for a CursorInterface implementation to rely on IteratorAggregate, as it would then have no way to influence the inner Cursor's iteration. Hypothetically, it could proxy other methods (e.g. toArray()), but I've never encountered such an implementation in the wild.

If having CursorInterface extend Iterator doesn't negatively impact PHPLIB, Doctrine ODM, and the Laravel integration, I wouldn't be too concerned about other use cases. Worst case, we're asking users to implement a implement a few extra methods (easily accomplished with a trait if needed).

[alcaeus]

Sounds good. Added PHPC-2447 to track this.

I'll note that we shouldn't implement IteratorAggregate, as that would suggest one can obtain multiple iterators from a single cursor. As a cursor can't be rewound and may only ever yield a single iterator, I don't think we should consider implementing IteratorAggregate.

[alcaeus]

CursorInterface was changed to extend Iterator, and this PR now includes changes for PHPLIB-1114, using CursorInterface where possible.

[GromNaN]

CursorInterface already extends Traversable, which is better IMO. We should not require the Iterator interface for custom cursor classes.
In case people really need the Iterator API, they can wrap it in an IteratorIterator.

Suggested change

	public function aggregate(array $pipeline, array $options = []): CursorInterface&Iterator
	public function aggregate(array $pipeline, array $options = []): CursorInterface

[jmikola]

Optional suggestions.

Decide how to handle the CI warnings, but LGTM.

[alcaeus]

Updated the PR. This currently includes changes from #1427, but I'll wait until that PR has been merged up to 2.x and rebasing this PR to preserve proper history.

Significant changes since review

[jmikola]

Nothing relevant to this PR, but I want to point out that it's a bit awkward that we can assign subscribers to Managers but not individual Servers. That forces us to use a global subscriber here.

I suppose we could add Server::getManager() to work around that 🤷‍♂️

[GromNaN]

Would it make the test more robust if you don't rely on int cast?

Suggested change

	$this->assertNotEquals(0, $changeStream->getCursorId());
	$this->assertNotEquals(new Int64(0), $changeStream->getCursorId());

[alcaeus]

It doesn't make a difference, as the custom Int64Comparator will always handle these assertions, and it leverages the comparison behaviour implemented in Int64 directly.

[alcaeus]

Note: this method previously created the fail point on the primary server, instead of on the secondary server where the changestream was created. This meant that the change stream was never resumed and the cursor ID was the same as before. We just didn't catch this mistake as the assertNotSame comparison always saw the objects as different as they didn't have the same identity. Lessons learned...

[GromNaN]

It should be fixed on the 1.x branch, right?

[alcaeus]

Extracted to #1434. Will rebase when that PR is merged.