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.5] Soft deleted models should still existing #17613

Merged
merged 1 commit into from
Feb 1, 2017
Merged

[5.5] Soft deleted models should still existing #17613

merged 1 commit into from
Feb 1, 2017

Conversation

JuanDMeGon
Copy link
Contributor

Hi, guys :)

When a model is soft deleted, its "exists" property should keep true. It was not completely removed and technically still existing.

This minor change will allow deleting and immediately forceDelete.

Example:

function someExample(Model $instance)
{
   //Independently of it uses SoftDeletes we can call delete()
   $instance->delete();

    if (someConditionToForceDeleteNow) {
        $instance->forceDelete();
    }
    return $instance;
}

If the instance was force deleted or not, equally the deleted_at attributes has been set and can be returned in the response.

Without this change, the instance will never be force deleted, and if we do not call the delete() before we will never have the deteleted_at date updated/set.

@CesarLanderos
Copy link

whats wrong with checking first your condition and then deleting?, like:

function someExample(Model $instance)
{
    if (someConditionToForceDeleteNow) {
        $instance->forceDelete();
    } else {
        $instance->delete();
    }
    return $instance;
}

i know that an else statement is not pretty but sound like is what you need.

@JuanDMeGon
Copy link
Contributor Author

It is a status stuff.
When I directly force delete, the deleted_at attribute is not updated/established, and it is a good idea to return an instance that reflects the action, in this case, the deleted_at attribute indicates that the element has been deleted at that precise moment.

It is used especially for RESTful APIs, on that, you want to return a response and evidentiate the action taken, along with the status code.

Equally, a "soft deleted" element still existing anyway, and you should be able to "force delete" it immediately after if needed.

@CesarLanderos
Copy link

Yeah, but for the client it is not relevant if it still exists on the database, thats why, i think, is set to false even if it was soft deleted.

@CesarLanderos
Copy link

But if i am wrong with the porpuse of the exists attribute, well, there is nothing to argue here :p

@JuanDMeGon
Copy link
Contributor Author

It is relevant at some point.
If you do not force delete, it returns an instance with the deleted_at attribute established, but if you force delete it returns an instance with null deleted_at value, and that is inconsistent.
If you forced to delete, you should have a deleted_at value independently if the instance was previously soft deleted or not.

Equally, as you said, maybe I am misunderstanding the purpose of the exists attribute and a soft deleted instance should not be existing? (Sound weird for me haha)

@GrahamCampbell
Copy link
Member

I'm sure we recently had a PR to move it there, and now we want to move it back?

@JuanDMeGon
Copy link
Contributor Author

Sorry for the confusion @GrahamCampbell , it is exactly the same PR, but for Laravel 5.5. The PR for Laravel 5.4 was closed by Taylor because it implies a breaking change.

#17381

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.

4 participants