Fix missing hint query option during find #2351
Conversation
| @@ -424,7 +424,7 @@ private function runQuery() | |||
|
|
|||
| switch ($this->query['type']) { | |||
| case self::TYPE_FIND: | |||
| $queryOptions = $this->getQueryOptions('select', 'sort', 'skip', 'limit', 'readPreference'); | |||
| $queryOptions = $this->getQueryOptions('select', 'sort', 'skip', 'limit', 'readPreference', 'hint'); | |||
There was a problem hiding this comment.
For now this fixes the bug, but I'm wondering if we should be more lenient with the options we accept. find take a lot more options than these few, and we may do well to allow people to utilise them. @jmikola any suggestions on this?
There was a problem hiding this comment.
I can work out a list of options from the Builder and Collection classes and write a bunch of tests for those in a separate PR.
There was a problem hiding this comment.
Let's hold off on what Jeremy says - not sure we need to test for every single one of them, but I'm rather thinking to just pass everything that we know isn't from ODM on to the driver.
There was a problem hiding this comment.
From what I can see, only maxTimeMS and snapshot are still missing. With snapshot being deprecated in the Find class, so I'm not sure if that should be added.
There was a problem hiding this comment.
@TimoBakx: I'm not sure what documentation you're referring to, but there are many more options supported by the extension and library:
- https://www.php.net/manual/en/mongodb-driver-query.construct.php
- https://docs.mongodb.com/php-library/current/reference/method/MongoDBCollection-find/
I think it makes sense for the builder to provide some API to add arbitrary options that may not be explicitly handled in ODM but are known to be supported in the extension/library. However, that would likely require some internal refactoring to the Query class, since many of these fields appear to be stuffed directly into the internal $query property. Handling this might require segregating options into $query['options'] so we no longer need to rely on getQueryOptions to extract specific keys.
There was a problem hiding this comment.
@jmikola I looked at all the options that have their own method in the query Builder class (and are added to the $this->query array there and checked which of those were used in the Find class code and were also missing in the $this->getQueryOptions call (which means that the instructions to the Builder would be ignored).
Moving things to separate options would indeed be better, but I'm not sure how much of a BC break that would be. It might be easier to add the missing values first. 🙂
|
I've created #2352 for the phpstan issue |
…ute to Collection->find Changed test to move hint from options to query array
…ilder and the resulting query
|
Thanks @TimoBakx! |
Summary
This PR fixes the missing
hintquery option during afindquery.I added two tests: one to make sure the hint gets correctly propagated from the query array to the find method, and one to make sure the hint is correctly carried from the query builder to the query.