PHPORM-139 Improve Model::createOrFirst() tests and error checking
#2759
Conversation
There was a problem hiding this comment.
Two outstanding points re: the findOneAndUpdate() call:
- Consider an explicit
(object)cast when executing the query. Re: PHPORM-139 ImplementModel::createOrFirst()usingfindOneAndUpdateoperation #2742 (comment) - Use
returnDocumentinstead ofnew. Re: PHPORM-139 ImplementModel::createOrFirst()usingfindOneAndUpdateoperation #2742 (comment)
| ['$setOnInsert' => (object) $values], | ||
| [ | ||
| 'upsert' => true, | ||
| 'returnDocument' => FindOneAndUpdate::RETURN_DOCUMENT_AFTER, |
There was a problem hiding this comment.
new option was internal. #2742 (comment)
| @@ -207,8 +214,12 @@ public function createOrFirst(array $attributes = [], array $values = []): Model | |||
| try { | |||
| $document = $collection->findOneAndUpdate( | |||
| $attributes, | |||
| ['$setOnInsert' => $values], | |||
| ['upsert' => true, 'new' => true, 'typeMap' => ['root' => 'array', 'document' => 'array']], | |||
| ['$setOnInsert' => (object) $values], | |||
There was a problem hiding this comment.
I added the (object) cast, but since we need to support server version 4.4, I need to keep an non-empty object (using filter attributes). #2742 (comment)
There was a problem hiding this comment.
The object cast certainly doesn't hurt and is arguably correct in the event all field names were numeric (if that's even possible here).
I do think it's worth adding a comment to explain that you're intentionally keeping extra fields in $values (instead of filtering down to keys in the original param) to ensure compatibility with pre-5.0 server versions.
Co-authored-by: Jeremy Mikola <[email protected]>
Applies @jmikola's review on #2742
Fix PHPORM-139