Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove compatibility layer for CursorId deprecation #1425

Closed
wants to merge 3 commits into from
Closed

Remove compatibility layer for CursorId deprecation #1425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00fa5e4
Add temporary stubs for cursor classes in 2.x
alcaeus Sep 19, 2024
bb4bb56
Remove CursorInterface::getId compatibility layer
alcaeus Sep 19, 2024
b06bffe
Update psalm baseline
alcaeus Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 12 additions & 24 deletions psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,7 @@
<code><![CDATA[$document['name']]]></code>
<code><![CDATA[$index->name]]></code>
</MixedArgument>
<MixedArrayAccess>
<code><![CDATA[$document['name']]]></code>
</MixedArrayAccess>
<MixedAssignment>
<code><![CDATA[$document]]></code>
<code><![CDATA[$index]]></code>
<code><![CDATA[$index]]></code>
<code><![CDATA[$index]]></code>
Expand All @@ -23,6 +19,9 @@
<PossiblyFalseArgument>
<code><![CDATA[$uri]]></code>
</PossiblyFalseArgument>
<PossiblyInvalidArrayAccess>
<code><![CDATA[$document['name']]]></code>
</PossiblyInvalidArrayAccess>
</file>
<file src="examples/persistable.php">
<LessSpecificReturnStatement>
Expand Down Expand Up @@ -184,9 +183,6 @@
<DeprecatedConstant>
<code><![CDATA[self::CURSOR_NOT_FOUND]]></code>
</DeprecatedConstant>
<TooManyArguments>
<code><![CDATA[getId]]></code>
</TooManyArguments>
</file>
<file src="src/Client.php">
<MixedArgument>
Expand Down Expand Up @@ -339,6 +335,15 @@
</MixedArgument>
</file>
<file src="src/Model/ChangeStreamIterator.php">
<InvalidReturnStatement>
<code><![CDATA[$cursor]]></code>
</InvalidReturnStatement>
<InvalidReturnType>
<code><![CDATA[CursorInterface<TValue>]]></code>
</InvalidReturnType>
<LessSpecificImplementedReturnType>
<code><![CDATA[CursorInterface<TValue>]]></code>
</LessSpecificImplementedReturnType>
<MixedArgument>
<code><![CDATA[$reply->cursor->nextBatch]]></code>
</MixedArgument>
Expand All @@ -354,11 +359,6 @@
<code><![CDATA[$this->current()]]></code>
</PossiblyNullArgument>
</file>
<file src="src/Model/CodecCursor.php">
<TooManyArguments>
<code><![CDATA[getId]]></code>
</TooManyArguments>
</file>
<file src="src/Model/CollectionInfoCommandIterator.php">
<MixedArrayAssignment>
<code><![CDATA[$info['idIndex']['ns']]]></code>
Expand Down Expand Up @@ -713,18 +713,6 @@
<code><![CDATA[is_object($result) ? ($result->value ?? null) : null]]></code>
</MixedReturnStatement>
</file>
<file src="src/Operation/FindOne.php">
<MixedAssignment>
<code><![CDATA[$document]]></code>
</MixedAssignment>
<MixedInferredReturnType>
<code><![CDATA[array|object|null]]></code>
</MixedInferredReturnType>
<MixedReturnStatement>
<code><![CDATA[$document === false ? null : $document]]></code>
<code><![CDATA[$document === false ? null : $document]]></code>
</MixedReturnStatement>
</file>
<file src="src/Operation/FindOneAndDelete.php">
<MixedAssignment>
<code><![CDATA[$options['fields']]]></code>
Expand Down
2 changes: 2 additions & 0 deletions psalm.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
<file name="stubs/BSON/Document.stub.php"/>
<file name="stubs/BSON/Iterator.stub.php"/>
<file name="stubs/BSON/PackedArray.stub.php"/>
<file name="stubs/Driver/Cursor.stub.php"/>
<file name="stubs/Driver/CursorInterface.stub.php"/>
</stubs>

<issueHandlers>
Expand Down
37 changes: 6 additions & 31 deletions src/ChangeStream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
use MongoDB\BSON\Document;
use MongoDB\BSON\Int64;
use MongoDB\Codec\DocumentCodec;
use MongoDB\Driver\CursorId;
use MongoDB\Driver\Exception\ConnectionException;
use MongoDB\Driver\Exception\RuntimeException;
use MongoDB\Driver\Exception\ServerException;
Expand All @@ -33,10 +32,6 @@
use function assert;
use function call_user_func;
use function in_array;
use function sprintf;
use function trigger_error;

use const E_USER_DEPRECATED;

/**
* Iterator for a change stream.
Expand Down Expand Up @@ -88,12 +83,8 @@ class ChangeStream implements Iterator
*/
private bool $hasAdvanced = false;

/**
* @see https://php.net/iterator.current
* @return array|object|null
*/
#[ReturnTypeWillChange]
public function current()
/** @see https://php.net/iterator.current */
public function current(): array|object|null
{
$value = $this->iterator->current();

Expand All @@ -106,26 +97,9 @@ public function current()
return $this->codec->decode($value);
}

/**
* @return CursorId|Int64
* @psalm-return ($asInt64 is true ? Int64 : CursorId)
*/
#[ReturnTypeWillChange]
public function getCursorId(bool $asInt64 = false)
public function getCursorId(): Int64
{
if (! $asInt64) {
@trigger_error(
sprintf(
'The method "%s" will no longer return a "%s" instance in the future. Pass "true" as argument to change to the new behavior and receive a "%s" instance instead.',
__METHOD__,
CursorId::class,
Int64::class,
),
E_USER_DEPRECATED,
);
}

return $this->iterator->getInnerIterator()->getId($asInt64);
return $this->iterator->getInnerIterator()->getId();
}

/**
Expand Down Expand Up @@ -255,7 +229,8 @@ private function onIteration(bool $incrementKey): void
* have been received in the last response. Therefore, we can unset the
* resumeCallable. This will free any reference to Watch as well as the
* only reference to any implicit session created therein. */
if ((string) $this->getCursorId(true) === '0') {
// Use a type-unsafe comparison to compare with Int64 instances
if ($this->getCursorId() == 0) {
$this->resumeCallable = null;
}

Expand Down
26 changes: 8 additions & 18 deletions src/Model/ChangeStreamIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

namespace MongoDB\Model;

use Iterator;
use IteratorIterator;
use MongoDB\BSON\Document;
use MongoDB\BSON\Serializable;
Expand Down Expand Up @@ -49,7 +48,7 @@
*
* @internal
* @template TValue of array|object
* @template-extends IteratorIterator<int, TValue, CursorInterface<int, TValue>&Iterator<int, TValue>>
* @template-extends IteratorIterator<int, TValue, CursorInterface<TValue>>
*/
final class ChangeStreamIterator extends IteratorIterator implements CommandSubscriber
{
Expand Down Expand Up @@ -77,18 +76,17 @@
}

/**
* Necessary to let psalm know that we're always expecting a cursor as inner
* iterator. This could be side-stepped due to the class not being final,
* but it's very much an invalid use-case. This method can be dropped in 2.0
* once the class is final.
* This method is necessary as psalm does not properly set the return type
* of IteratorIterator::getInnerIterator to the templated iterator
*
* @return CursorInterface<int, TValue>&Iterator<int, TValue>
* @see https://github.com/vimeo/psalm/pull/11100.
*
* @return CursorInterface<TValue>
Dismissed Show dismissed Hide dismissed
Dismissed Show dismissed Hide dismissed
*/
final public function getInnerIterator(): Iterator
public function getInnerIterator(): CursorInterface
{
$cursor = parent::getInnerIterator();
assert($cursor instanceof CursorInterface);
assert($cursor instanceof Iterator);

return $cursor;
}
Expand Down Expand Up @@ -173,18 +171,10 @@

/**
* @internal
* @psalm-param CursorInterface<int, TValue>&Iterator<int, TValue> $cursor
* @psalm-param CursorInterface<TValue> $cursor
*/
public function __construct(CursorInterface $cursor, int $firstBatchSize, array|object|null $initialResumeToken, private ?object $postBatchResumeToken = null)
{
if (! $cursor instanceof Iterator) {
throw InvalidArgumentException::invalidType(
'$cursor',
$cursor,
CursorInterface::class . '&' . Iterator::class,
);
}

if (isset($initialResumeToken) && ! is_document($initialResumeToken)) {
throw InvalidArgumentException::expectedDocumentType('$initialResumeToken', $initialResumeToken);
}
Expand Down
35 changes: 6 additions & 29 deletions src/Model/CodecCursor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,24 @@

namespace MongoDB\Model;

use Iterator;
use MongoDB\BSON\Document;
use MongoDB\BSON\Int64;
use MongoDB\Codec\DocumentCodec;
use MongoDB\Driver\Cursor;
use MongoDB\Driver\CursorId;
use MongoDB\Driver\CursorInterface;
use MongoDB\Driver\Server;
use ReturnTypeWillChange;

use function assert;
use function iterator_to_array;
use function sprintf;
use function trigger_error;

use const E_USER_DEPRECATED;
use const E_USER_WARNING;

/**
* @template TValue of object
* @template-implements CursorInterface<int, TValue>
* @template-implements Iterator<int, TValue>
* @template-implements CursorInterface<TValue>
*/
class CodecCursor implements CursorInterface, Iterator
class CodecCursor implements CursorInterface
{
private const TYPEMAP = ['root' => 'bson'];

Expand All @@ -64,33 +58,16 @@ public function current(): ?object
* @param DocumentCodec<NativeClass> $codec
* @return self<NativeClass>
*/
public static function fromCursor(Cursor $cursor, DocumentCodec $codec): self
public static function fromCursor(CursorInterface $cursor, DocumentCodec $codec): self
{
$cursor->setTypeMap(self::TYPEMAP);

return new self($cursor, $codec);
}

/**
* @return CursorId|Int64
* @psalm-return ($asInt64 is true ? Int64 : CursorId)
*/
#[ReturnTypeWillChange]
public function getId(bool $asInt64 = false)
public function getId(): Int64
{
if (! $asInt64) {
@trigger_error(
sprintf(
'The method "%s" will no longer return a "%s" instance in the future. Pass "true" as argument to change to the new behavior and receive a "%s" instance instead.',
__METHOD__,
CursorId::class,
Int64::class,
),
E_USER_DEPRECATED,
);
}

return $this->cursor->getId($asInt64);
return $this->cursor->getId();
}

public function getServer(): Server
Expand Down Expand Up @@ -138,7 +115,7 @@ public function valid(): bool
}

/** @param DocumentCodec<TValue> $codec */
private function __construct(private Cursor $cursor, private DocumentCodec $codec)
private function __construct(private CursorInterface $cursor, private DocumentCodec $codec)
{
}
}
41 changes: 41 additions & 0 deletions stubs/Driver/Cursor.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

namespace MongoDB\Driver;

/**
* @template-covariant TValue of array|object
*
* @template-implements CursorInterface<TValue>
*/
final class Cursor implements CursorInterface
{
/**
* @return TValue|null
* @psalm-ignore-nullable-return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, @psalm-ignore-nullable-return tells Psalm to never consider a null return value. But current() and key() can certainly return null if valid() is false. Why needn't we consider that case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was blatantly copied over from psalm's iterator stubs. The way I see it, this is a shortcoming of the original interface. The interface doesn't specify whether current and key should throw when on an invalid position, and most iterators return null instead. IMO, we should throw, but I imagine a lot of code isn't exactly built to handle throwing in the methods. At the same time, we do expect people to not call current and key without checking that the iterator is on a valid position, hence that annotation.

For immutable classes, the psalm-assert-if-true annotation would solve this, but since next has to reset a previously memoized result from valid I'm not sure if psalm can handle it. It would definitely be a worthwhile improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

I imagine throwing would be incredibly annoying, as manual iteration is already a complicated subject. I reckon the average PHP developer isn't familiar with the sequence of method calls to mimic a foreach loop.

Not so much the case with our write result getters relying on isAcknowledged(), as that's easier to wrap one's head around.

*/
public function current(): array|object|null
{
}

public function next(): void
{
}

/** @psalm-ignore-nullable-return */
public function key(): ?int
{
}

public function valid(): bool
{
}

public function rewind(): void
{
}

/** @return array<TValue> */
public function toArray(): array
{
}
}
32 changes: 32 additions & 0 deletions stubs/Driver/CursorInterface.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace MongoDB\Driver;

use Iterator;

/**
* @template TValue of array|object
* @template-implements Iterator<int, TValue>
*/
interface CursorInterface extends Iterator
{
/**
* @return TValue|null
* @psalm-ignore-nullable-return
*/
public function current(): array|object|null;

public function getId(): \MongoDB\BSON\Int64;
jmikola marked this conversation as resolved.
Show resolved Hide resolved

public function getServer(): Server;

public function isDead(): bool;

/** @psalm-ignore-nullable-return */
public function key(): ?int;

public function setTypeMap(array $typemap): void;

/** @return array<TValue> */
public function toArray(): array;
}
Loading
Loading