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] Allow Eloquent has one relations to return a default new model #16198

Merged
merged 1 commit into from Nov 11, 2016

Conversation

sileence
Copy link
Contributor

@sileence sileence commented Oct 31, 2016

So I've been dealing with this problem: I have a user and I need to get its profile, but the user does not have one, so something like this throws an exception:

$thi->profile->twitter (Trying to use a property on a non-object whatever whatever...)

So I came with this idea:

    public function profile()
    {
        return $this->hasOne(UserProfile::class)->withDefault();
    }

Now when the result is not found, Eloquent creates one new model instance for me and I can continue with my life...

This is just a proposal / proof of concept.

@themsaid
Copy link
Member

themsaid commented Oct 31, 2016

Well this is a breaking change I believe since if($this->profile) will return true while it returns false before this change.

Moreover I think it's better for these nullable relationships to check before you use:

if($this->profile){
   // Do what you want if a profile was found
}

@sileence
Copy link
Contributor Author

@themsaid Does it? It should return null as before. The only difference is that when using withDefault you'll get a new profile with the FK populated instead of null.

That way you should be able to do:

$user->profile->fill($data)

$user->profile->save();

Without being worried of profile being null.

In the view you could use:

if($user->profile->exists)

But this change should be activated if and only if you call withDefault... So no breaking change intended.

@themsaid
Copy link
Member

got it now, not breaking :)

@taylorotwell
Copy link
Member

Can you do getProfileAttribute and do your null checking there?

@sileence
Copy link
Contributor Author

sileence commented Nov 2, 2016

@taylorotwell yes, but it's not easy, to do it right you require something like this:

    public function getProfileAttribute()
    {
        if ($profile = $this->getRelationValue('profile')) {
            return $profile;
        }

        $profile = new UserProfile();
        $profile->user_id = $this->id;

        $this->setRelation('profile', $profile);

        return $profile;
    }

The problem is not only that the object is null but that if you create a new one it won't be related to the user (in this example) by default.

@sileence
Copy link
Contributor Author

sileence commented Nov 2, 2016

@taylorotwell also having profile() and getProfileAttribute() feels hacky. What method is called first and why? (I know it works but it's not clear at first sight).

@shadoWalker89
Copy link
Contributor

I know that I'm not a high profile dude, but IMHO I think this is very bad.
If something that doesn't exist, then it doesn't, and that is why null should be returned. Also, that is why the null keyword exists in all sorts of programming languages, to indicate a missing value ...

Having a default model returned is very confusing, especially to newcomers.

This if($user->profile) is a lot more cleaner and understandable then if($user->profile->exists).

To me having if($user->profile->exists) in my code is a lot more hacky then profile() and getProfileAttribute()

@sileence
Copy link
Contributor Author

sileence commented Nov 2, 2016

@shadoWalker89 I'm not changing the default behavior, if you don't use ->withDefault() then you'll still get null, if you do then you know what you are doing and what to expect.

Also there is a pattern in OOP call Null Object in which you replace null by a null object in that way you eliminate or simplify the conditionals in the code. You can find a video in Laracasts to check why and when it is useful to have a null object instead of null.

@sileence
Copy link
Contributor Author

sileence commented Nov 2, 2016

Also notice that with hasMany you get an empty collection, not null. And you need to call isEmpty() to know if the collection is empty or not.

@GrahamCampbell GrahamCampbell changed the title [5.3] [proposal] Allow Eloquent has one relations to return a default new model [5.3] [Allow Eloquent has one relations to return a default new model Nov 2, 2016
@GrahamCampbell GrahamCampbell changed the title [5.3] [Allow Eloquent has one relations to return a default new model [5.3] Allow Eloquent has one relations to return a default new model Nov 2, 2016
->setAttribute(
$this->getPlainForeignKey(), $this->getParentKey()
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it return null explicitly if $this->withDefault evaluates to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you don't return anything, the method will return null by default. So there is an implicit return null; at the end of the method.

@taylorotwell
Copy link
Member

How would this work if profile was eager loaded from an array of models?

@sileence
Copy link
Contributor Author

sileence commented Nov 9, 2016

@taylorotwell since the getResults won't be called it will be null but I can fix that so it keeps the same intended behaviour.

@sileence
Copy link
Contributor Author

@taylorotwell ok I made that last change and tested with load, with and lazy loading as well and it's working 👍

@taylorotwell taylorotwell merged commit 87ff047 into laravel:5.3 Nov 11, 2016
@taylorotwell
Copy link
Member

I did add one feature to this. You can do:

->hasOne('Foo')->withDefault(function () {
    return new SomeValue;
});

use Illuminate\Database\Eloquent\Collection;

class HasOne extends HasOneOrMany
{
/**
* Determine whether getResults should return a default new model instance or not.
Copy link
Member

Choose a reason for hiding this comment

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

This variable doesn't determine anything, rather, the description should be just "If getResults should return a default model instance."

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

6 participants