-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHPORM-127 Fix priority of embeds vs attributes #2584
base: 5.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -805,19 +805,23 @@ class User extends Model | |
} | ||
} | ||
``` | ||
**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. | ||
|
||
### EmbedsMany Relationship | ||
|
||
If you want to embed models, rather than referencing them, you can use the `embedsMany` relation. This relation is similar to the `hasMany` relation but embeds the models inside the parent object. | ||
|
||
**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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this PR targets 5.x. Should this documentation change be backported? Also, is this heading better written as:
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. |
||
|
||
```php | ||
use MongoDB\Laravel\Eloquent\Model; | ||
use MongoDB\Laravel\Relations\EmbedsMany; | ||
|
||
class User extends Model | ||
{ | ||
public function books() | ||
public function books(): EmbedsMany | ||
{ | ||
return $this->embedsMany(Book::class); | ||
} | ||
|
@@ -886,10 +890,11 @@ Like other relations, embedsMany assumes the local key of the relationship based | |
|
||
```php | ||
use MongoDB\Laravel\Eloquent\Model; | ||
use MongoDB\Laravel\Relations\EmbedsMany; | ||
|
||
class User extends Model | ||
{ | ||
public function books() | ||
public function books(): EmbedsMany | ||
{ | ||
return $this->embedsMany(Book::class, 'local_key'); | ||
} | ||
|
@@ -902,12 +907,15 @@ Embedded relations will return a Collection of embedded items instead of a query | |
|
||
The embedsOne relation is similar to the embedsMany relation, but only embeds a single model. | ||
|
||
**Breaking changes** starting from v4.0 you need to define the return type of EmbedsOne and EmbedsMany relation for it to work | ||
|
||
```php | ||
use MongoDB\Laravel\Eloquent\Model; | ||
use MongoDB\Laravel\Relations\EmbedsOne; | ||
|
||
class Book extends Model | ||
{ | ||
public function author() | ||
public function author(): EmbedsOne | ||
{ | ||
return $this->embedsOne(Author::class); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,19 @@ | |
use Illuminate\Support\Str; | ||
use MongoDB\Laravel\Relations\EmbedsMany; | ||
use MongoDB\Laravel\Relations\EmbedsOne; | ||
use ReflectionMethod; | ||
use ReflectionNamedType; | ||
|
||
/** | ||
* Embeds relations for MongoDB models. | ||
*/ | ||
trait EmbedsRelations | ||
{ | ||
/** | ||
* @var array<class-string, array<string, bool>> | ||
*/ | ||
private static array $hasEmbeddedRelation = []; | ||
|
||
/** | ||
* Define an embedded one-to-many relationship. | ||
* | ||
|
@@ -76,4 +83,27 @@ protected function embedsOne($related, $localKey = null, $foreignKey = null, $re | |
|
||
return new EmbedsOne($query, $this, $instance, $localKey, $foreignKey, $relation); | ||
} | ||
|
||
/** | ||
* Determine if an attribute is an embedded relation. | ||
* | ||
* @param string $key | ||
* @return bool | ||
* @throws \ReflectionException | ||
*/ | ||
private function hasEmbeddedRelation(string $key): bool | ||
{ | ||
if (! method_exists($this, $method = Str::camel($key))) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return false; | ||
} | ||
|
||
if (isset(self::$hasEmbeddedRelation[static::class][$key])) { | ||
return self::$hasEmbeddedRelation[static::class][$key]; | ||
} | ||
|
||
$returnType = (new ReflectionMethod($this, $method))->getReturnType(); | ||
|
||
return self::$hasEmbeddedRelation[static::class][$key] = $returnType instanceof ReflectionNamedType | ||
&& in_array($returnType->getName(), [EmbedsOne::class, EmbedsMany::class], true); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -272,6 +272,10 @@ public function belongsToMany( | |||||
|
||||||
$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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I propose to throw an exception in this case to avoid any error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can optionally use argument numbering to remove the duplicate
Suggested change
|
||||||
} | ||||||
|
||||||
$relatedPivotKey = $relatedPivotKey ?: $instance->getForeignKey().'s'; | ||||||
|
||||||
// If no table name was provided, we can guess it by concatenating the two | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
use MongoDB\BSON\ObjectID; | ||
use MongoDB\BSON\UTCDateTime; | ||
use MongoDB\Laravel\Query\Builder as QueryBuilder; | ||
use ReflectionException; | ||
use function uniqid; | ||
|
||
abstract class Model extends BaseModel | ||
|
@@ -154,6 +155,7 @@ public function getTable() | |
|
||
/** | ||
* @inheritdoc | ||
* @throws ReflectionException | ||
*/ | ||
public function getAttribute($key) | ||
{ | ||
|
@@ -172,11 +174,7 @@ public function getAttribute($key) | |
} | ||
|
||
// This checks for embedded relation support. | ||
if ( | ||
method_exists($this, $key) | ||
&& ! method_exists(self::class, $key) | ||
&& ! $this->hasAttributeGetMutator($key) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
) { | ||
if ($this->hasEmbeddedRelation($key)) { | ||
return $this->getRelationValue($key); | ||
} | ||
|
||
|
@@ -474,7 +472,7 @@ public function getForeignKey() | |
/** | ||
* Set the parent relation. | ||
* | ||
* @param \Illuminate\Database\Eloquent\Relations\Relation $relation | ||
* @param Relation $relation | ||
*/ | ||
public function setParentRelation(Relation $relation) | ||
{ | ||
|
@@ -484,7 +482,7 @@ public function setParentRelation(Relation $relation) | |
/** | ||
* Get the parent relation. | ||
* | ||
* @return \Illuminate\Database\Eloquent\Relations\Relation | ||
* @return Relation | ||
*/ | ||
public function getParentRelation() | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,6 @@ class Group extends Eloquent | |
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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