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

Changed type hint from Cursor to Iterator in __construct #221

Closed

Conversation

TomHAnderson
Copy link
Member

Following instructions @ Doctrine\ODM\MongoDB\Cursor

@deprecated This class is deprecated and will be removed in 2.0. You should typehint against the {@see Iterator} interface instead.

@TomHAnderson
Copy link
Member Author

Will fix: #190

@coveralls
Copy link

Coverage Status

Coverage remained the same at 71.772% when pulling 098fe27 on TomHAnderson:hotfix/paginator_construct into 7f39c21 on doctrine:master.

@@ -19,7 +20,7 @@ class DoctrinePaginator implements AdapterInterface
/**
* Constructor
*/
public function __construct(Cursor $cursor)
public function __construct(Iterator $cursor)
Copy link
Member

Choose a reason for hiding this comment

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

This makes no sense on its own, the class is still depending it'll get a Cursor instance

Copy link
Member Author

Choose a reason for hiding this comment

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

The CommandCursor does not extend Cursor so it cannot be used in the DoctrinePaginator, which is the point of this change: #190

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but for one in count() method there's a call to $this->cursor->getBaseCursor() - from \Iterator's perspective there's no such method

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright. I'll close this PR and mark the issue as will not address.

@TomHAnderson
Copy link
Member Author

Will not address this issue: #190

@@ -19,7 +20,7 @@ class DoctrinePaginator implements AdapterInterface
/**
* Constructor
*/
public function __construct(Cursor $cursor)
public function __construct(Iterator $cursor)
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to widen the scope of acceptance and can potentially break code, as on line 34 if this file you rely on an undefined Iterator method called getBaseCursor.
When widen acceptance scope, it's recommended to add a test that covers the baseline and make sure it supports the baseline properly. In this case, a direct simple implementation of Iterator

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.

None yet

4 participants