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

Dispatch to DispatchSync with non queueable job #182

Merged

Conversation

peterfox
Copy link
Collaborator

Changes

  • Adds a rule to covert dispatch helper and other dispatcher calls to use dispatch sync methods instead
  • Adds tests
  • Updates docs
  • Adds the rule to the Laravel 10

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.

@peterfox peterfox self-assigned this Feb 19, 2024
@GeniJaho
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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.

…b' of github.com:driftingly/rector-laravel into feature/dispatch-to-dispatch_sync-with-non-queueable-job
@peterfox
Copy link
Collaborator Author

@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
Copy link
Owner

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
Copy link
Collaborator Author

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

@driftingly
Copy link
Owner

Thanks @peterfox
Looks good to me 👍

@peterfox
Copy link
Collaborator Author

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

@GeniJaho GeniJaho merged commit bf0cd91 into main Feb 28, 2024
5 checks passed
@GeniJaho GeniJaho deleted the feature/dispatch-to-dispatch_sync-with-non-queueable-job branch February 28, 2024 21:02
@GeniJaho
Copy link
Collaborator

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

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