[5.1] Exists property is set to false on soft deleted eloquent model

[HolgerW1]

If you soft delete a model the property exists is set to false which may lead to problems.

Try the following code as a test:

$model->delete();
$model->forceDelete();

Surprisingly the model won't be hard deleted because the first delete call will set the exists property to false thus the second call of forceDelete won't perform anything and the model remains in the DB.

Soft deleting a model should prevent setting the exists property to false because otherwise the model object is in an inconsistent state and any logic relying on this property to check if a model was deleted will fail.

[GrahamCampbell]

no trailing whitespace please

[HolgerW1]

Wow, I really should call it a day - even the automatic code formatting failed on me thus the trailing spaces.
Happy weekend everybody!

[GrahamCampbell]

Could you rebase to resolve the merge conflcits please.

[HolgerW1]

Could you rebase to resolve the merge conflcits please.

Done.

[HolgerW1]

@GrahamCampbell I found some other bugs related to the model deletion and the exists property:

• the return value documentation of some deletion methods is wrong
• the return value of some deletion methods is inconsistent (forceDelete should have the same return value types as delete)
• the deleted event is fired even if a deletion fails
• deleting a model that doesn't exist anymore in the DB doesn't fail and fires the deleted event (see above)
• calling modification methods (update ...) on a model object for which a deletion failed will operate on the model as if it doesn't exist yet / anymore

Should I add fixes for those to this pull request?

[lukasgeiter]

The return type of forceDelete has already been discussed. #9624

[HolgerW1]

The return type of forceDelete has already been discussed. #9624

Thanks for the cross reference! Even though I would discuss this from a different point of view.

The forceDelete method (just as the delete method) may fail for whatever reason and without throwing an exception. As a developer you have to know whether this implied 'strong' operation has failed or not, you need the model to be deleted and you need to know if the deletion was successful or not and if you should take any additional action.
You could of course make a separate call after each forceDelete to check if the model still exists but this would be really bad design. Especially since the forceDelete does / should always know if the operation was successful or not.

That being said you may still return void (i.e. not call return) in forceDelete if that's a choice by design but the meaning of this should be very clear and documented. I.e. a omitted return would mean 'I don't know what happened, check for yourself' while a bool value would indicate very clearly the result of the operation on which the developer can rely. 'true' means the deletion operation was successful while 'false' means the operation did nothing due to some kind of error which should be handled by the caller.

Btw. there's no such thing as a 'real' void return in PHP. Not calling return is the same as returning NULL (http://php.net/manual/en/functions.returning-values.php)

[GrahamCampbell]

Btw. there's no such thing as a 'real' void return in PHP. Not calling return is the same as returning NULL

Yes, but not the same as void. Returning a string is allowed if the return annotation says void for example. void just means, don't use the return value, for whatever reason.

[HolgerW1]

void just means, don't use the return value, for whatever reason.

True, that's at least the definition of phpdoc.

void, this type is commonly only used when defining the return type of a method or function. The basic definition is that the element indicated with this type does not contain a value and the user should not rely on any retrieved value.

I don't wanna get offtopic but I guess the point is that you shouldn't return anything if you define the return value as void. Defining a return value as undefined and telling the users to not use it but on the other hand actually returning meaningful and well defined data is unexpected and inconsistent. Why return something if it can't and shouldn't be used anyways? That's crying for wrong usages, misunderstandings and unwanted side effects.

Which brings me back to the mentioned methods delete and forceDelete. The delete method of the Eloquent Model class should currently explicitly return null instead of omitting the return statement (which would semantically be a @return void) even though PHP internally returns null.

Question regarding this: what's the current definition and interpretation of the possible return values of the delete method? true for success, false for failure and null if the method didn't do anything?

forceDelete on the other hand defines the return value as void but actually returns the result of the delete call (which isn't void). But if you look at the forceDelete implementation of the SoftDeletes trait then you're gonna see that this implementation doesn't use a return statement which would comply to the phpdoc definition.

My suggestion regarding this particular point for a clear and consistent definition and implementation (note that I haven't added those to the pull request yet):
both methods delete and forceDelete defined in the Modelclass and the SoftDeletes trait should have the exact same return type definition and should both return true on success and false on failure (tbd if null remains for NOPs). Which would give the caller the opportunity to actually know that sth. went wrong during the deletion process.

Please note that the other previously listed bugs (3, 4 and 5) not necessarily have anything to do with this. Those remain no matter how the definition of the return values look like.

Sorry for the TLDR block.

[taylorotwell]

Should this be considered a breaking change?

[GrahamCampbell]

Yeh, this probably needs to go to 5.2.

[HolgerW1]

The pull request in its current form is not a breaking change but rather a bugfix. I wouldn't recommend to postpone it.

Changing the return value behavior of forceDelete to match the one of delete shouldn't be a breaking change either because it was meant to be void and would become bool or null just like the one of delete. I.e. sth. is added that couldn't be and shouldn't have been used before. forceDelete of the Model class currently returns the result of delete anyways, only soft-deleted models would be affected.

Returning false upon deletion failure and not firing deleted however could be a breaking change if developers did sth. strange like expect the delete to return true on failure or expect this event to fire no matter of the result of the deletion. Which seems odd to me but there might be a slight possibility.

From my point of view the current deletion behavior (maybe except the deleted event firing) should be fixed since 5.1 is meant to be a LTS version which might be used for several years. But that's of course a decision the software architect has to make ;-)

How about I open a new separate pull request later for the change of the return value thing so you're actually able to see what would change in code?

[GrahamCampbell]

Ping @HolgerW1.

[rentalhost]

@HolgerW1 Just to understand... Why you closed this PR?

[GrahamCampbell]

Why you closed this PR?

Because we're not accepting it on 5.1. We're waiting on a PR to 5.2.

[rentalhost]

@GrahamCampbell It isn't considered like a bugfix? Users on 5.1 will be ever affected by this bug on LTS? I don't think that it is a breaking-change, but an expected behaviour when you call forceDelete() (you should literally force delete the row, if it don't delete, it fails).

Imagine that you have a system with Administrators and Moderators. If some Moderator delete something, they will do a soft delete. Administrators can check if it will be hard deleted or restored (returning to Moderators) or just keep as soft delete.

To hard delete in this case, you should use a workaround (nothing good) by restoring first and then forceDeleting.

Ok, it's just a example. In this case I should consider create another system like "tag as invalid" to Administrator check if before delete. But is possible you expect that by terms.

[HolgerW1]

Just to understand... Why you closed this PR?

As Graham said, they consider it a breaking change and it seems there was no chance for it to get it in for 5.1. Furthermore even though this PR would fix this specific issue the whole delete and forceDelete functions could need some major refactoring. I.e. it makes more sense to not just fix this specific issue but rather adapt the whole deletion functionality.

We're waiting on a PR to 5.2.

Sorry, but I currently don't have the time to create a PR for the master branch. Plus I think it would make more sense if someone would review the current deletion implementations. Especially the inconsistent return behavior and the setting of the exists property.

[taylorotwell]

This is fixed in 5.5.