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

[8.x] Use "sync" queue in Bus::dispatchNow() #32559

Merged
merged 3 commits into from
Apr 28, 2020
Merged

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Apr 27, 2020

This PR fixes two issues. First, it fixes an issue where job middleware are not run when doing something like ProcessPodcast::dispatchNow(). This can be fixed by dispatching this job through the sync queue instead of invoking it directly from the command bus.

The second problem it fixes is currently any jobs dispatched using dispatchNow do not have have access to the $this->job property, even though they use InteractsWithQueue. The $this->job variable will just be null. This is awkward because you may have a job that is sometimes dispatched using the normal dispatch method and sometimes using dispatchNow and the $this->job variable is only going to be available when dispatched using dispatch. So, you would have to put null checks within your job to work around this. This PR solves this by setting a SyncJob on the job if one is not already set.

Added a new dispatchDirectly method to execute a job directly within the command bus without using the sync queue or job middleware (basically the old dispatchNow behavior).

Alternative:

One alternative to this is to leave dispatchNow as it is and add a new dispatchSync method (or some similar name) that dispatches the job using the sync queue.

@Muffinman
Copy link
Contributor

I've run into this myself with a project recently. Looks like a good change! 👍

@GrahamCampbell GrahamCampbell changed the title Use "sync" queue in Bus::dispatchNow() [8.x] Use "sync" queue in Bus::dispatchNow() Apr 27, 2020
@driesvints driesvints linked an issue Apr 28, 2020 that may be closed by this pull request
@driesvints
Copy link
Member

I think updating the behavior here for dispatchNow definitely makes sense. At the moment, I can't see any way it'd break an app.

@winkbrace
Copy link

I know it's just semantics, but maybe a little more difference in method naming? So instead of dispatchDirectly you could emphasize the difference in behaviour more with for example dispatchRaw or dispatchOnly?

@taylorotwell taylorotwell merged commit abcead0 into master Apr 28, 2020
@taylorotwell
Copy link
Member Author

Decided to take a less breaking approach using a new dispatchSync method.

@rodrigopedra
Copy link
Contributor

Hi @taylorotwell , great addition solves a problem I have in a app right now.

One question: as the dispacthNow will attach a SyncJob instance to the command, will the failed method be called when an exception is thrown inside the command?

That would simplify/unify a lot of jobs error handling strategies I current have between sync and queued jobs.

Thanks in advance.

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.

[6.0] Job middleware not respected with dispatchNow()
5 participants