[5.3] Update Model save() to return true on non-error

[davidrushton]

In 5.3, the Model save() method is updated to return false when no attributes have changed.

This breaks all of my project repository classes, which use the following code:

if ( ! $user->save() ) {
    throw new StorageException;
}

If the Eloquent class is extended or an Eloquent event listener is used, save() correctly returns false, which will halt my repository with an exception.

Saving without changing anything on the model is fine, so I don't think false is an appropriate return value from this method.

Commit 34b86eb fixed this same issue 5.2

Thanks :)

David.

[nhowell]

I, too, don't understand why this change was made in 5.3.

[nhowell]

@davidrushton I think one way you could avoid this problem in 5.3 is by first checking if the model is dirty:

if ($user->isDirty() && !$user->save()) {
    throw new StorageException;
}

Still, I think you shouldn't need to do this in the first place...

[fernandobandeira]

I tried to change the update method to return a model a while ago and got the following response #13488

No, sorry. It also returns false is there were no changes made, so the return value has meaning already.

So maybe this is intended on 5.3 at least.

[taylorotwell]

Honestly you should never check the return value of the save method. If it didn't throw an exception, it worked. There is no need at all to ever check the return value for any reason. It's been very hard for me to communicate this to the community even though checking the value is never shown in the documentation.

[taylorotwell]

Even though I've merged this please never check the return value on this method. There is never a good reason to do so.

[nhowell]

@taylorotwell That doesn't really explain why it's useful to return false when no changes were made, though. It muddies the meaning of the return value because then it could mean that either the model isn't dirty, or a model event returned false.

I think it's unintuitive to return false on a non-error. false should mean that something prevented the model from saving (e.g. a model event).

Honestly you should never check the return value of the save method.

How about using it to check if it passed the model events? Which appears to be @davidrushton's use case.

Anyway, thanks for merging! 👍

[nhowell]

Even though I've merged this please never check the return value on this method. There is never a good reason to do so.

Then, for argument's sake, why not just return void from save()?

Also, to add to my point, even the comments in the save() method say that false indicates that the save failed:

// If the "saving" event returns false we'll bail out of the save and return
// false, indicating that the save failed.

[davidrushton]

Thanks for merging @taylorotwell. I disagree that false should not be caught as an unexpected return value from the save() method. If nothing was changed, I can use isDirty() myself if I want to check for that situation.

As @nhowell said, I think the following logic is sound, given that a model save method may be overwritten and return false, or that an event listener may cancel save and return false:

Returning false from an Eloquent event listener will cancel the save / update operation.

For example, as an integrity check I do:

public function create(array $fields, array $related)
{
    $product = $this->product->newInstance($fields);

    if ( ! $product->save() ) {
        throw new StorageException;
    }

    if ( $sites = array_get($related, 'sites') ) {
        $product->sites()->sync($sites);
    }

    return $product;
}

If save returns false, and I cannot guarantee the model was saved, then I can't use Eloquent to create certain relationships. Although I don't use model events often, the StorageException catches this situation rather than an unexpected and uncaught exception.

[taylorotwell]

Just ignore the return value of this method entirely. There is no reason to check it. If the method didn't throw an exception you can assume it was saved.

[nhowell]

If the method didn't throw an exception you can assume it was saved.

But that is simply not true if you're using model events that might cancel the save...

On Saturday, September 3, 2016, Taylor Otwell notifications@github.com
wrote:

Just ignore the return value of this method entirely. There is no reason
to check it. If the method didn't throw an exception you can assume it was
saved.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15236 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMPG2HTHhnBTbd99uKWSNjltUeDTNZCks5qmYm_gaJpZM4Jz1i0
.