PHPORM-127 Fix priority of embeds vs attributes - #2584
Conversation
da4339c to
81a0fc6
Compare
| $instance = new $related; | ||
|
|
||
| if ($otherKey === $relation) { | ||
| throw new \LogicException(sprintf('In %s::%s(), the key cannot be identical to the relation name "%s". The default key is "%s".', static::class, $relation, $relation, $instance->getForeignKey().'s')); |
There was a problem hiding this comment.
The solution was so easy, just don't name the foreign key the same as the relation; that is the default behavior if you don't pass any foreign keys to $this->belongsToMany(related , /* Don't pass anything else */) , and that is also the documented way to do it.
I propose to throw an exception in this case to avoid any error.
|
|
||
| public function otherGroups() | ||
| { | ||
| return $this->belongsToMany(Group::class, 'groups', 'users', 'otherGroups', '_id', '_id', 'groups'); |
There was a problem hiding this comment.
I'm going to add a test for the exception.
171d8c2 to
68afe15
Compare
GromNaN
changed the title
| } | ||
| } | ||
| ``` | ||
| **Warning:** naming the foreign key same as the relation name will prevent the relation for being called on dynamic property, i.e. in the example above if you replaced `group_ids` with `groups` calling `$user->groups` will return the column instead of the relation. |
There was a problem hiding this comment.
I think "e.g." would be more appropriate because you're providing one possible example.
See: https://www.merriam-webster.com/grammar/ie-vs-eg-abbreviation-meaning-usage-difference
|
|
||
| **REMEMBER**: These relations return Eloquent collections, they don't return query builder objects! | ||
|
|
||
| **Breaking changes** starting from v4.0 you need to define the return type of EmbedsOne and EmbedsMany relation for it to work |
There was a problem hiding this comment.
I see this PR targets 5.x. Should this documentation change be backported?
Also, is this heading better written as:
**Breaking Change:** starting from...
You're only noting a single breaking change, and it appears to be a label (vs. part of the sentence) so a colon could help differentiate. This applies below as well.
| */ | ||
| private function hasEmbeddedRelation(string $key): bool | ||
| { | ||
| if (! method_exists($this, $method = Str::camel($key))) { |
There was a problem hiding this comment.
I've never bothered to do this myself, but assumed CS for PHPLIB prohibited such inline assignments in the interest of readability. Is this common within the Laravel package?
Happy to defer to you.
| $instance = new $related; | ||
|
|
||
| if ($relatedPivotKey === $relation) { | ||
| throw new \LogicException(sprintf('In %s::%s(), the key cannot be identical to the relation name "%s". The default key is "%s".', static::class, $relation, $relation, $instance->getForeignKey().'s')); |
There was a problem hiding this comment.
You can optionally use argument numbering to remove the duplicate $relation argument.
| throw new \LogicException(sprintf('In %s::%s(), the key cannot be identical to the relation name "%s". The default key is "%s".', static::class, $relation, $relation, $instance->getForeignKey().'s')); | |
| throw new \LogicException(sprintf('In %1$s::%2$s(), the key cannot be identical to the relation name "%2$s". The default key is "%3$s".', static::class, $relation, $instance->getForeignKey().'s')); |
| if ( | ||
| method_exists($this, $key) | ||
| && ! method_exists(self::class, $key) | ||
| && ! $this->hasAttributeGetMutator($key) |
There was a problem hiding this comment.
Not sure what this method does, but it seems the fix here is replace this with checking that the method returns either EmbedsOne or EmbedsMany (per your EmbedsRelations trait).
| public function users(): BelongsToMany | ||
| { | ||
| return $this->belongsToMany(User::class, 'users', 'groups', 'users', '_id', '_id', 'users'); | ||
| return $this->belongsToMany(User::class, 'users', 'groupIds', 'userIds', '_id', '_id', 'users'); |
There was a problem hiding this comment.
Is renaming to "userIds" the meaningful change here, so that it doesn't conflict with the relationship name? Is the renaming of "groupIds" just for consistency?
jmikola
left a comment
jmikola
left a comment
There was a problem hiding this comment.
Some optional suggestions and questions. If everything is easily resolved/answered, feel free to merge.
3 participants
Fix #2577 (continuing)
Fix PHPORM-127
Moving the code from the
Modelclass to theEmbedsRelationstrait.