[5.3] Potential issue with withDefault() method on HasOne relationship

[patrickcarlohickman]

• Laravel Version: 5.3.23+
• PHP Version: >=5.6.4
• Database Driver & Version: Any

Description:

The new withDefault() method on the HasOne relationship is very useful. However, the Closure functionality of the method may need to be slightly restricted.

There is existing code that depends on the result of any Eloquent relationship to either be null, a Model instance, or a Collection of Model instances. However, the current functionality of the withDefault() method opens up the potential for returning an object that is not one of those three expected values.

Now, not only do some existing applications and packages depend on this set of return types, but some of the core framework code does, as well. For example, if you supply a Closure that just returns a new stdClass instance, and then call push() to save all the relationships, the parent model will save fine, but when it attempts to save the relationship, it will generate the error Call to undefined method stdClass::push().

Steps To Reproduce:

In tests/Database/DatabaseEloquentIntegrationTest.php, add two new relationships to the EloquentTestUser class:

class EloquentTestUser extends Eloquent
{
    public function postWithDefaultModel()
    {
        return $this->hasOne('EloquentTestPost', 'user_id')->withDefault();
    }

    public function postWithDefaultNonModel()
    {
        return $this->hasOne('EloquentTestPost', 'user_id')->withDefault(function () {
            return new stdClass;
        });
    }
}

Add the following two tests:

// this test passes fine
public function testPushHasOneWithDefaultModel()
{
    $user = EloquentTestUser::create(['email' => 'taylorotwell@gmail.com']);
    $post = $user->postWithDefaultModel;

    $this->assertInstanceOf('EloquentTestUser', $user);
    $this->assertInstanceOf('EloquentTestPost', $post);
    $this->assertFalse($post->exists);

    $post->name = 'First Post';
    $user->push();

    $this->assertTrue($post->exists);
    $this->assertTrue(!empty($post->id));
}

// this test causes an error at $user->push()
public function testPushHasOneWithDefaultNonModel()
{
    $user = EloquentTestUser::create(['email' => 'taylorotwell@gmail.com']);
    $post = $user->postWithDefaultNonModel;

    $this->assertInstanceOf('EloquentTestUser', $user);
    $this->assertNotInstanceOf('Illuminate\Database\Eloquent\Model', $post);

    $post->name = 'First Post';
    $result = $user->push(); // Error

    $this->assertTrue($result);
}

Suggested Resolution

Three potential options:

1. Remove the Closure functionality altogether. I would imagine this is not desirable, as the Closure functionality does add some nice flexibility.

2. Throw an exception if the Closure returns a value that is not null or a Model instance.

3. Ignore the return value of the Closure completely. The Closure already receives a Model instance as an argument. This instance should be able to be modified inside the Closure without having to return it from the Closure.

For example, the below relationship will work just fine (problems arise with non-falsey, non-Model return values).

return $this->hasOne('EloquentTestPost', 'user_id')->withDefault(function ($model) {
    $model->name = 'Modified Post';
    // no need to return the $model
});

[themsaid]

@sileence what do you think?

[sileence]

@themsaid I like the option 3. Right now if you return null it will return the default instance anyway. I can PR this later on if you want me too.

[sileence]

BTW I'm glad you find this tiny feature useful @patrickcarlohickman

[decadence]

@sileence Is there any chance this feature will be implemented for BelongsTo?

[sileence]

@decadence: @taylorotwell @themsaid do you think adding withDefault to belongsTo could be useful? If so I can create the PR.

[decadence]

@sileence I think it will. But there is one problem: you can't set foreign key for current model because model from withDefault will not exist by that time.
Like here (from HasOne->getDefaultFor):

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

But we call save and associate anyway later.

[decadence]

Any updates on this?

[sileence]

Well, withDefault has now being implemented for belongsTo and morphOne relations.

I think it's responsibility of the developer to return a model (or null) when using this feature with a Closure. I think it's out of the scope of Laravel to add checks for this. cc @themsaid

[laurencei]

Closing as " think it's out of the scope of Laravel to add checks for this"