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

Allow specifying generic options for Query find operations #2017

Merged
merged 2 commits into from
Apr 26, 2019

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Apr 24, 2019

Q A
Type feature
BC Break no
Fixed issues #1808

Summary

Previously, find operations were unique in that they did not inherit generic options passed to the Query::__construct() via Builder::getQuery(). This makes find consistent with other operations and also adds tests for allowing query array options to override these generic options.

This also enabled users to pass driver-specific options to Collection::find() that may not be supported by the Builder API.

This method is only referenced from Query::execute() and used internally.
@jmikola jmikola added this to the 2.0.0-Beta2 milestone Apr 24, 2019
@jmikola jmikola requested a review from alcaeus April 24, 2019 14:23
@jmikola
Copy link
Member Author

jmikola commented Apr 24, 2019

@alcaeus: Let me know if this PR requires a changelog entry, or if that will be automatically generated when the next 2.0 tag is made. I don't believe we need to add anything to UPGRADE-2.0.md.

* Decorate the cursor with caching, hydration, and priming behavior.
*
* Note: while this method could strictly take a MongoDB\Driver\Cursor, we
* accept Traversable for testing purposes since Cursor cannot be mocked.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: The CursorInterface being introduced in 1.6.0 (PHPC-1123) would help here, but it's likely you'll get ODM 2.0 released before that's available. I think we should consider swapping any Cursor typehints to CursorInterface for ODM 3.0, though (if you'd like to make a ticket now).

https://jira.mongodb.org/browse/PHPC-1281

{
$collection = $this->getMockCollection();
$nearest = new ReadPreference('nearest');
$secondaryPreferred = new ReadPreference('secondaryPreferred');
Copy link
Member Author

Choose a reason for hiding this comment

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

It would have been nice to refactor both of these tests to use data providers, but ReadPreference still cannot be serialized (pending PHPC-1281 for 1.6.0) and doing so might cause problems if the test suite is run with process isolation.

Copy link
Member

Choose a reason for hiding this comment

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

As a workaround, you could simply pass the readPreference string for now and change it once we've updated the requirements to require 1.6.0. I'll leave it up to you whether to change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's a very sensible idea. I was too hung up on passing the raw arguments to consider it :P

@jmikola jmikola removed this from the 2.0.0-Beta2 milestone Apr 24, 2019
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

Looks good - no need to refactor the tests to use data providers unless you think it's absolutely worth it.

@jmikola
Copy link
Member Author

jmikola commented Apr 26, 2019

no need to refactor the tests to use data providers unless you think it's absolutely worth it.

No need since we're only testing count and find here, but it's something we could revisit in the future if more tests are created to cover option inheritance across all operation types.

Fixes doctrine#1808

Previously, find operations were unique in that they did not inherit generic options passed to the Query::__construct() via Builder::getQuery(). This makes find consistent with other operations and also adds tests for allowing query array options to override these generic options.

This also enabled users to pass driver-specific options to Collection::find() that may not be supported by the Builder API.
@jmikola
Copy link
Member Author

jmikola commented Apr 26, 2019

Revised to fix CS failures in https://travis-ci.org/doctrine/mongodb-odm/jobs/524008616. Will merge after the build passes.

@jmikola jmikola merged commit 042fc73 into doctrine:master Apr 26, 2019
@jmikola jmikola deleted the gh1808 branch April 26, 2019 16:11
@jmikola jmikola self-assigned this Apr 26, 2019
@alcaeus alcaeus added this to the 2.0.0-Beta2 milestone Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants