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

[8.x] Set relation parent key when using forceCreate on HasOne and HasMany relations #42281

Merged
merged 1 commit into from
May 6, 2022

Conversation

rodrigopedra
Copy link
Contributor

Closes #42273

Disclaimer this is the same as PR #42280 but targeted to Laravel 8.x as suggested by @driesvints .

As I will be out for some hours, I sent this new PR, so you (maintainers) can decide on which branch this PR fits better.

Reason I didn't send to 8.x on the first place is stated on this comment: #42280 (comment)

As explained in issue #42273 when calling several methods that either create or instantiate a related instance from a relation method, the relation parent's key is automatically filled, but not when calling forceCreate.

Take this model as an example:

class Lesson extends Model
{
    public function questions()
    {
        return $this->hasMany(Question::class);
    }
}

When calling all these following methods the lesson primary key is set as the new question instance foreign key automatically:

$lesson = Lesson::first();

$q1 = $lesson->questions()->create(...);
$q2 = $lesson->questions()->findOrNew(...);
$q3 = $lesson->questions()->firstOrNew(...);
$q4 = $lesson->questions()->make(...);
$q5 = $lesson->questions()->save(new Question(...));

This behavior is also present in other methods which use the ones listed above such as updateOrCreate, saveMany, and others.

But when calling ->forceCreate(...) from a relation, the foreign key is not set, which I agree with issue's #42273 OP, that is not the expected behavior.

Reason it does not work as the other is due to forceCreate not being implemented on HasOneOrMany, thus it is deferred to the underlying Builder which, of course, would not set any additional attributes.

This PR:

  • Adds a forceCreate method to the HasOneOrMany relation class, which adds the relation parent key before actually force creating a related instance
  • Adds relevant tests

Copy link

@atoff atoff left a comment

Choose a reason for hiding this comment

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

Hey Rodrigo, thanks for taking a look at my issue so soon! This looks to do exactly what I was thinking of, but not sure if it would be neater to do something like my suggested change?

Just thinking that it might be nice to re-use the implementation for create? Could be totally wrong but just an idea!

@taylorotwell taylorotwell merged commit 0431475 into laravel:8.x May 6, 2022
@rodrigopedra
Copy link
Contributor Author

rodrigopedra commented May 6, 2022

Hey @atoff I added a comment after your last regard, but lastly I resolved the conversation to avoid spamming maintainers notifications.

If you want to discuss it further we can move back to the related issue (#42273).

Thanks for looking into it, and have a nice day =)

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.

4 participants