Remove compatibility layer for CursorId deprecation#1425
Remove compatibility layer for CursorId deprecation#1425alcaeus wants to merge 3 commits intomongodb:v2.xfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * @return CursorInterface<int, TValue>&Iterator<int, TValue> | ||
| * @see https://github.com/vimeo/psalm/pull/11100. | ||
| * | ||
| * @return CursorInterface<TValue> |
Check notice
Code scanning / Psalm
LessSpecificImplementedReturnType
| * @return CursorInterface<int, TValue>&Iterator<int, TValue> | ||
| * @see https://github.com/vimeo/psalm/pull/11100. | ||
| * | ||
| * @return CursorInterface<TValue> |
Check notice
Code scanning / Psalm
InvalidReturnType
|
The CI build fails as we're not testing with the 2.0 version yet. This depends on mongodb/mongo-php-driver#1672. It may be necessary to wrap this work into #1391. |
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
Some questions, but I don't think I have any actionable feedback.
| { | ||
| /** | ||
| * @return TValue|null | ||
| * @psalm-ignore-nullable-return |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 'MongoDB\Driver\Cursor::getId(): The method "MongoDB\Driver\Cursor::getId" will no longer return a "MongoDB\Driver\CursorId" instance in the future. Pass "true" as argument to change to the new behavior and receive a "MongoDB\BSON\Int64" instance instead.', | ||
| $deprecations[1][1], | ||
| ); | ||
| } |
|
Folded into #1391 |
2 participants
This pull request removes the compatibility layer introduced to handle the deprecation of
MongoDB\Driver\CursorId. To work around psalm not having updated stubs, we introduce stubs temporarily until the stubs are properly updated.Note that due to an issue in psalm, we still need a workaround for
ChangeStreamIterator::getInnerIterator. I've created vimeo/psalm#11100 to fix this issue, and the errors are suppressed in the baseline until a fix is released.