From 68afe154feaa8c14add0ed1193faf332515a4e23 Mon Sep 17 00:00:00 2001 From: shamaseen Date: Tue, 29 Aug 2023 13:22:21 +0200 Subject: [PATCH] Fix priority of embeds vs attributes --- README.md | 14 +++++++++++--- src/Eloquent/EmbedsRelations.php | 30 ++++++++++++++++++++++++++++++ src/Eloquent/HybridRelations.php | 4 ++++ src/Eloquent/Model.php | 12 +++++------- tests/ModelTest.php | 7 +++++++ tests/Models/Group.php | 2 +- tests/Models/User.php | 25 +++++++++++++++++++------ tests/RelationsTest.php | 4 ++-- 8 files changed, 79 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index d7a505bd1..6b8fb4655 100644 --- a/README.md +++ b/README.md @@ -805,6 +805,7 @@ 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 @@ -812,12 +813,15 @@ If you want to embed models, rather than referencing them, you can use the `embe **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 + ```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); } diff --git a/src/Eloquent/EmbedsRelations.php b/src/Eloquent/EmbedsRelations.php index 2847de338..3f38ac54e 100644 --- a/src/Eloquent/EmbedsRelations.php +++ b/src/Eloquent/EmbedsRelations.php @@ -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> + */ + 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))) { + 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); + } } diff --git a/src/Eloquent/HybridRelations.php b/src/Eloquent/HybridRelations.php index dc735b973..900f50c48 100644 --- a/src/Eloquent/HybridRelations.php +++ b/src/Eloquent/HybridRelations.php @@ -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')); + } + $relatedPivotKey = $relatedPivotKey ?: $instance->getForeignKey().'s'; // If no table name was provided, we can guess it by concatenating the two diff --git a/src/Eloquent/Model.php b/src/Eloquent/Model.php index 45f583501..57f6a8411 100644 --- a/src/Eloquent/Model.php +++ b/src/Eloquent/Model.php @@ -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) - ) { + 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() { diff --git a/tests/ModelTest.php b/tests/ModelTest.php index 6b3e5eb57..12eebf006 100644 --- a/tests/ModelTest.php +++ b/tests/ModelTest.php @@ -1035,4 +1035,11 @@ public function testEnumCast(): void $this->assertSame(MemberStatus::Member->value, $check->getRawOriginal('member_status')); $this->assertSame(MemberStatus::Member, $check->member_status); } + + public function testGetFooAttributeAccessor() + { + $user = new User(); + + $this->assertSame('normal attribute', $user->foo); + } } diff --git a/tests/Models/Group.php b/tests/Models/Group.php index 4fba28493..ad079e6b9 100644 --- a/tests/Models/Group.php +++ b/tests/Models/Group.php @@ -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'); } } diff --git a/tests/Models/User.php b/tests/Models/User.php index f1d373fab..77d184f76 100644 --- a/tests/Models/User.php +++ b/tests/Models/User.php @@ -4,6 +4,7 @@ namespace MongoDB\Laravel\Tests\Models; +use Carbon\Carbon; use DateTimeInterface; use Illuminate\Auth\Authenticatable; use Illuminate\Auth\Passwords\CanResetPassword; @@ -14,6 +15,8 @@ use Illuminate\Support\Str; use MongoDB\Laravel\Eloquent\HybridRelations; use MongoDB\Laravel\Eloquent\Model as Eloquent; +use MongoDB\Laravel\Relations\EmbedsMany; +use MongoDB\Laravel\Relations\EmbedsOne; /** * Class User. @@ -23,9 +26,9 @@ * @property string $email * @property string $title * @property int $age - * @property \Carbon\Carbon $birthday - * @property \Carbon\Carbon $created_at - * @property \Carbon\Carbon $updated_at + * @property Carbon $birthday + * @property Carbon $created_at + * @property Carbon $updated_at * @property string $username * @property MemberStatus member_status */ @@ -76,7 +79,7 @@ public function clients() public function groups() { - return $this->belongsToMany(Group::class, 'groups', 'users', 'groups', '_id', '_id', 'groups'); + return $this->belongsToMany(Group::class, 'groups', 'userIds', 'groupIds', '_id', '_id', 'groups'); } public function photos() @@ -84,12 +87,12 @@ public function photos() return $this->morphMany(Photo::class, 'has_image'); } - public function addresses() + public function addresses(): EmbedsMany { return $this->embedsMany(Address::class); } - public function father() + public function father(): EmbedsOne { return $this->embedsOne(self::class); } @@ -106,4 +109,14 @@ protected function username(): Attribute set: fn ($value) => Str::slug($value) ); } + + public function getFooAttribute() + { + return 'normal attribute'; + } + + public function foo() + { + return 'normal function'; + } } diff --git a/tests/RelationsTest.php b/tests/RelationsTest.php index f418bf384..c930cfb3d 100644 --- a/tests/RelationsTest.php +++ b/tests/RelationsTest.php @@ -344,8 +344,8 @@ public function testBelongsToManyCustom(): void $group = Group::find($group->_id); // Check for custom relation attributes - $this->assertArrayHasKey('users', $group->getAttributes()); - $this->assertArrayHasKey('groups', $user->getAttributes()); + $this->assertArrayHasKey('userIds', $group->getAttributes()); + $this->assertArrayHasKey('groupIds', $user->getAttributes()); // Assert they are attached $this->assertContains($group->_id, $user->groups->pluck('_id')->toArray());