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 SerializesModels to optionally restore models excluded by global scope (SoftDeletes, for example) #16301

Merged
merged 5 commits into from
Nov 8, 2016

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Nov 7, 2016

Currently, if you want to queue jobs with deleted models (or anything else restricted by default via a global scope), you cannot use SerializesModels. This change adds a method to Illuminate/Queue/SerializesAndRestoresModelIdentifiers called restoresModelsWithoutScopes(), which returns false by default, but can be overridden by classes that use it to change this behavior.

Now you can do:

class DoSomethingWithDeletedUsers extends Job implements ShouldQueue
{
    use SerializesModels;

    public function __construct(User $user)
    {
        $user->delete();
        $this->user = $user;
    }

    public function handle()
    {
        $this->user->trashed(); // true
    }

    protected function restoresModelsWithoutScopes()
    {
        return true;
    }
}

By having restoresModelsWithoutScopes() return true, the getRestoredPropertyValue() method will call newQueryWithoutScopes() instead of newQuery().

@taylorotwell
Copy link
Member

Personally it seems like it should always be without scopes and not even be configurable. If you queued the model instance surely that means you want it to be available.

@inxilpro
Copy link
Contributor Author

inxilpro commented Nov 7, 2016

That's what I thought at first, but I can think of some cases where that's not true. For example, say you queued an email to a user to be delivered in 3 hours and then deleted the user for some reason 2 hours later—in that case you probably wouldn't want the job to run.

I could see a strong argument for defaulting restoresModelsWithoutScopes() to return true, though, because I think you're right in most cases.

@taylorotwell
Copy link
Member

Yeah, I can't decide there. I mean that sounds fairly edge case and could be mitigated by checking if the user has been deleted or not before sending the email. I'm still of the opinion it should be without scopes by default.

@inxilpro
Copy link
Contributor Author

inxilpro commented Nov 7, 2016

I'll update tonight making no scopes the default. I don't think it hurts much to make it configurable, especially because it's a pretty big pain to modify this particular behavior via inheritance/composition.

@GrahamCampbell GrahamCampbell changed the title Allow SerializesModels to optionally restore models excluded by global scope (SoftDeletes, for example) [5.3] Allow SerializesModels to optionally restore models excluded by global scope (SoftDeletes, for example) Nov 7, 2016
@GrahamCampbell
Copy link
Member

Looks like a nice idea. 👍

@inxilpro
Copy link
Contributor Author

inxilpro commented Nov 8, 2016

Done.

@taylorotwell taylorotwell merged commit 007802d into laravel:5.3 Nov 8, 2016
@taylorotwell
Copy link
Member

I tweaked this a bit. If you want scopes you can just override the getQueryForModelRestoration method. There isn't really a need for an option then.

@inxilpro
Copy link
Contributor Author

inxilpro commented Nov 8, 2016

Oh, good call. That's certainly simpler is we assume that most people won't want to change this.

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.

3 participants