Support write command operations #180
Conversation
support write commands
|
I'd hold off on this pull request until the upstream issue has been dealt with. A fix there would mean no changes required here. Also, this pull request is missing tests. |
|
Steps to reproduce: mongodb/mongo-php-library#384 |
| @@ -405,7 +405,12 @@ public function command(array $data, $options = [], &$hash = null) | |||
| { | |||
| try { | |||
| $cursor = new \MongoCommandCursor($this->connection, $this->name, $data); | |||
| $cursor->setReadPreference($this->getReadPreference()); | |||
|
|
|||
| if (!empty($data['new']) || !empty($data['upsert'])) { | |||
There was a problem hiding this comment.
Note that there are many more commands that write than findAndModify, which it looks like this is attempting to detect.
Also, command detection would be better implemented by checking the first key in the command document (i.e. $data argument), as that must indicate the command name. That would allow most write commands to be detected, excluding aggregate and mapReduce where you would need to inspect additional options to determine if they are directing output to a collection.
There was a problem hiding this comment.
Note that there are many more commands that write than findAndModify, which it looks like this is attempting to detect.
Yes, because of this I look values of keys "new" and "upsert". So it works for all commands.
There was a problem hiding this comment.
This is a bad decision, but it works.
There was a problem hiding this comment.
My point was that commands should not be detected by the presence of options. As-is, this approach had a few complications:
- It picks up any command that might include a
neworupsertkey at the top level. Even if no other command uses these options, it assumes no future command will. - The
newandupsertoptions forfindAndModityare both boolean options. Usingempty()here means that the condition is only satisfied if either option istrue; however, they could both befalse. - Some variations of
findAndModifydo not even use these options (e.g. find and delete).
I don't meant to discourage you with the above criticism. Rather, I want to stress the importance of using a robust solution in library code (be it Doctrine MongoDB, the adapter, or the new extension's PHP library).
There was a problem hiding this comment.
I don't meant to discourage you with the above criticism. Rather, I want to stress the importance of using a robust solution in library code (be it Doctrine MongoDB, the adapter, or the new extension's PHP library).
I now, its fast fix, no more.
So, maybe allow to set manually?
if ($options['readPreference']) {
$cursor->setReadPreference($options['readPreference']);
} else {
$cursor->setReadPreference($this->getReadPreference());
}
There was a problem hiding this comment.
#181 and doctrine/mongodb#297 should address the issue. The adapter was missing some behavior that forced a primary server for most commands, which was undocumented behavior in the legacy mongo extension. On a separate note, Doctrine MongoDB needed a patch to be able to apply a read preference to findAndModify and mapReduce commands executed through its query builder.
|
As per mongodb/mongo-php-library#384 (comment), this is an upstream issue that will be fixed in doctrine/mongodb. |
Details: mongodb/mongo-php-library#384