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

[5.3] Improve the "with default" option in the has one relation. #16382

Merged
merged 1 commit into from
Nov 15, 2016

Conversation

sileence
Copy link
Contributor

There are 3 commits here:

  1. First add test for the closure option.

  2. Add option to pass an array of attributes for the default model, i.e.:

$this->hasOne(Profile::class)->withDefault(['avatar' => 'default.jpg']);
  1. Make sure the closure is associated to the parent/related model. I'm allowing the possibility that the closure could return null.

@GrahamCampbell GrahamCampbell changed the title Improve the "with default" option in the has one relation. [5.3] Improve the "with default" option in the has one relation. Nov 12, 2016
return call_user_func($this->withDefault);
$result = call_user_func($this->withDefault);
} elseif (is_array($this->withDefault)) {
$result = $this->related->newInstance($this->withDefault);
Copy link
Member

Choose a reason for hiding this comment

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

You would probably want to forceFill these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sileence sileence force-pushed the hasOneWithDefault/improveClosure branch from e56b33a to 99d7169 Compare November 12, 2016 18:44
if (is_callable($this->withDefault)) {
return call_user_func($this->withDefault);
$result = call_user_func($this->withDefault);
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why would the callback want to return null? That's a little weird.
  2. Wouldn't it be better to pass a new linked-up model to the callback?

Maybe something like this:

protected function getDefaultFor(Model $model)
{
    if (! $this->withDefault) {
        return;
    }

    $instance = $this->related->newInstance()->setAttribute(
        $this->getPlainForeignKey(), $model->getAttribute($this->localKey)
    );

    if (is_callable($this->withDefault)) {
        return call_user_func($this->withDefault, $instance);
    }

    if (is_array($this->withDefault)) {
        $instance->forceFill($this->withDefault);
    }

    return $instance;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I don't know, I just wanted to leave that possibility.
  2. I really like how you refactored the code 👍

Copy link
Member

Choose a reason for hiding this comment

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

So are you going to refactor the code to Joseph's suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taylorotwell done. If nothing is returned from the closure, the default instance will be returned automatically.

Thank you @JosephSilber! The code looks better now.

@sileence sileence force-pushed the hasOneWithDefault/improveClosure branch 2 times, most recently from 6149832 to e98e75c Compare November 14, 2016 14:51
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.

None yet

3 participants