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.

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

Fix Scroll:current() annotation #1749

Merged
merged 1 commit into from
Jan 20, 2020

Conversation

thePanz
Copy link
Collaborator

@thePanz thePanz commented Jan 16, 2020

When analyzing a code using the iterator in a foreach($scroll as $resultSet) with phpstan, the following error is reported:

Argument of an invalid type Elastica\ResultSet|null supplied for foreach, only iterables are supported.

There is quite a debate about the return type of an Iterator::current(), as it should always return the item in the collection.
Invoking the current() when "out of bounds" of the collection is expected not to happen, as the Iterator::valid() should be called first.

@thePanz thePanz requested a review from p365labs January 16, 2020 14:44
@thePanz thePanz force-pushed the fix-scroll-iterator-current branch from 98b0782 to 5a30457 Compare January 17, 2020 10:30
Copy link
Owner

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Probably worth a changelog entry

Calling Scroll->current() on an invalid Iterator should be avoided: the Iterator documentation
states that code should first call "valid()" before accessing the "current()" value. Not
doing so can lead to unexpected results.

Scroll will throw an InvalidException when trying to call "current()" on an invalid state.
@thePanz thePanz force-pushed the fix-scroll-iterator-current branch from 5a30457 to 6236a07 Compare January 20, 2020 10:03
@thePanz
Copy link
Collaborator Author

thePanz commented Jan 20, 2020

@ruflin Changelog added :)

@thePanz thePanz merged commit 1dd1d02 into ruflin:master Jan 20, 2020
@thePanz thePanz deleted the fix-scroll-iterator-current branch January 20, 2020 11:10
@Tobion
Copy link
Collaborator

Tobion commented Jan 20, 2020

Ref. #1506

@thePanz
Copy link
Collaborator Author

thePanz commented Jan 20, 2020

Didn't know about that PR @Tobion . The whole library should get a cleanup on the Iterator implementations :)

@ruflin
Copy link
Owner

ruflin commented Jan 21, 2020

@thePanz I would not say no 😇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants