PHPORM-8 Remove automatic _id conversion, apply Cast to queries #23
Conversation
GromNaN
force-pushed
the
phporm-8-remove-automatic-conversion
branch
from
20968bb
to
6946c7a
Compare
| */ | ||
| public function chunkById($count, callable $callback, $column = '_id', $alias = null) | ||
| { | ||
| return parent::chunkById($count, $callback, $column, $alias); |
There was a problem hiding this comment.
The parent method is already using defaultKeyName.
| @@ -950,19 +926,6 @@ protected function compileWheres(): array | |||
| } | |||
| } | |||
|
|
|||
| // Convert id's. | |||
There was a problem hiding this comment.
In the Query\Builder, we don't know the model. We cannot guess the type of the _id field. It's better not doing any transformation.
| @@ -19,28 +27,38 @@ class ObjectId implements CastsAttributes | |||
| */ | |||
| public function get($model, string $key, $value, array $attributes) | |||
| { | |||
| if (! $value instanceof BSONObjectId) { | |||
| return $value; | |||
| if ($value instanceof BSONObjectId && $model->getKeyName() === $key && $model->getKeyType() === 'string') { | |||
There was a problem hiding this comment.
The ObjectId cast should not be used with anything else than ObjectId key type. Otherwise there is a mismatch in the type when using Model->getKey() to query the database.
This impacts queries using the query builder and relations.
There was a problem hiding this comment.
This must be revisited to support SQL<->MongoDB relations.
|
|
||
| // Attach the new parent id to the related model. | ||
| $query->push($this->foreignPivotKey, $this->parent->getKey(), true); | ||
| } | ||
|
|
||
| // Attach the new ids to the parent model. | ||
| $this->parent->push($this->getRelatedKey(), (array) $id, true); | ||
| $this->parent->push($this->getRelatedKey(), is_array($id) ? $id : [$id], true); |
There was a problem hiding this comment.
The (array) $id cast works until it is applied to an object.
(array) 1 => [1]
(array) (new ObjectId()) => ['oid' => ... ]| @@ -257,7 +259,7 @@ protected function buildDictionary(Collection $results) | |||
|
|
|||
| foreach ($results as $result) { | |||
| foreach ($result->$foreign as $item) { | |||
| $dictionary[$item][] = $result; | |||
| $dictionary[(string) $item][] = $result; | |||
There was a problem hiding this comment.
Casts the ObjectId to a valid array-key.
The type of $dictionary can't be changed to SplObjectStorage it's used to search on values in Laravel's BelongsToMany::match.
Fortunately, the key is stringified before being searched.
| @@ -245,8 +251,7 @@ protected function getForeignKeyValue($id) | |||
| $id = $id->getKey(); | |||
| } | |||
|
|
|||
| // Convert the id to MongoId if necessary. | |||
| return $this->toBase()->convertKey($id); | |||
| return $id; | |||
| */ | ||
| public function getIdAttribute($value = null) | ||
| { | ||
| // If we don't have a value for 'id', we will use the MongoDB '_id' value. |
There was a problem hiding this comment.
This is a feature that needs to be kept: Allow to use a custom 'id' field as id.
Developers should work with
ObjectIdfor the_idfield.But if the
Model::$keyTypeisstring, they can use a cast to convert database values (ObjectId) tostringwhen reading, andstringtoObjectIdwhen setting the value for the database (as document value or for a query).Changes are TDD: I updated to code to make the tests work as much as possible.
Only the model key is casted because this where there is an issue. But could opt for a more generic approach an apply the cast to all the values sent to Eloquent builder (ie: linked to a model).