driftingly / rector-laravel

Rector upgrades rules for Laravel
http://getrector.org
MIT License
469 stars 48 forks source link

Dispatch to DispatchSync with non queueable job #182

Closed peterfox closed 4 months ago

peterfox commented 5 months ago

Changes

Why

Performs the following:

-$result = dispatch(new SomeJob());
-$anotherResult = Bus::dispatch(new SomeJob());
-$anotherResult = $this->dispatch(new SomeJob());
+$result = dispatch_sync(new SomeJob());
+$anotherResult = Bus::dispatchSync(new SomeJob());
+$anotherResult = $this->dispatchSync(new SomeJob());

In Laravel 9 and earlier if you were able to send a non-queue job to dispatch and it would handle it immediately but now it will return a PendingBatch instead of the return value of the job. Even then it doesn't make sense to use dispatch instead of dispatchSync if it's a non queueable job so this should be helpful to a few people.

GeniJaho commented 5 months ago

This rule is great 🙌 The return value could also be used in places like if () conditions, return expressions, or foreach loops. Would it make sense, or would it be safe, to rename this method in all its occurrences, not just assignments?

peterfox commented 5 months ago

@GeniJaho I looked at this but it's very difficult to work out all possible contexts of a return value being used

The main problem with dispatch returning PendingBatch is that it uses a destructor so in some cases it will be fine. But the worst is when a variable is created so I just focused on that.

GeniJaho commented 5 months ago

I was thinking, since we're only doing a rename, we could target all use cases by targeting the three nodes directly:

public function getNodeTypes(): array
{
    return [FuncCall::class, MethoCall::class, StaticCall::class];
}

This would also simplify the rule, but I'm not sure if we should touch all cases anyway. Please generate the docs once more, since your other PRs were merged.

peterfox commented 5 months ago

@GeniJaho docs have been updated 👍

In theory, there's no reason why it can't just be for any call I guess, as I can't see a reason why someone would dispatch an instance that didn't have ShouldQueue implemented.

driftingly commented 5 months ago

Thanks @peterfox and @GeniJaho If you were using dispatch in an if or foreach I don't think you're expecting a PendingBatch. I'm leaning towards renaming all instances, not just assignments.

peterfox commented 5 months ago

@GeniJaho @driftingly I've updated to refactor all calls rather than just assigned ones.

driftingly commented 4 months ago

Thanks @peterfox Looks good to me 👍

peterfox commented 4 months ago

@GeniJaho you okay with the rule in the current state?

GeniJaho commented 4 months ago

@peterfox I was ok with the rule since 1 week ago 😁