Skip to content
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

[Fix] morphTo relationship #2669

Merged
merged 13 commits into from
Nov 8, 2023
Merged

Conversation

hans-thomas
Copy link
Contributor

Hi, it fixes #2657.

src/Relations/MorphTo.php Outdated Show resolved Hide resolved
@hans-thomas hans-thomas marked this pull request as ready for review November 6, 2023 14:31
@GromNaN GromNaN self-requested a review November 6, 2023 14:33
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changes should be done in HybridRelations.

There is a previous PR that that tries to fix morphTo #2640, but it does not solves the issue reported in #2657.

Also, in HybridRelations::morphTo, a "Check if it is a relation with an original model." with fallback to Eloquent is missing.

src/Relations/MorphTo.php Outdated Show resolved Hide resolved
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It fixes the issue reported in #2657.

@GromNaN GromNaN requested a review from alcaeus November 7, 2023 10:25
@alcaeus
Copy link
Member

alcaeus commented Nov 8, 2023

Thank you @hans-thomas!

@alcaeus alcaeus merged commit bfbe055 into mongodb:4.1 Nov 8, 2023
13 checks passed
@hans-thomas hans-thomas deleted the 2657-morphTo-relations branch November 23, 2023 13:12
@@ -221,7 +221,7 @@ public function morphTo($name = null, $type = null, $id = null, $ownerKey = null
$this->newQuery(),
$this,
$id,
$ownerKey,
$ownerKey ?: $this->getKeyName(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2783 suggests that this broke MorphTo relationships when a model uses a custom primary key.

#3011 includes a proposed fix.

@@ -16,7 +16,11 @@ public function addConstraints()
// For belongs to relationships, which are essentially the inverse of has one
// or has many relationships, we need to actually query on the primary key
// of the related models matching on the foreign key that's on a parent.
$this->query->where($this->ownerKey, '=', $this->parent->{$this->foreignKey});
$this->query->where(
$this->ownerKey,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the $this->getKeyName() fallback in HybridRelations::morphTo and allowing MorphTo to be constructed with a null $ownerKey would result in an exception here when null is used in place of a string field name.

The relevant fix in #3011 is to fall back to $this->getForeignKeyName() here, which respects a custom primary key name on parent.


// inverse
$photo = Photo::query()->create(['url' => 'https://graph.facebook.com/hans.thomas/picture']);
$client = Client::create(['name' => 'Hans Thomas']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hans-thomas: Was there a particular reason you used Photo::query()->create() above but Client::create() here? Best I could tell, they resolve to the same Builder::create() method.

Client::create() just ends up forwarding the call through Model::__call().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for morphTo relations broken in 4.1
4 participants