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] add allOnQueue and allOnConnection for job chaining #21765

Merged
merged 3 commits into from
Nov 2, 2017

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Oct 20, 2017

Having:

JobA::withChain([
    (new JobB())->onQueue('second')->onConnection('connection2'),
    (new JobC()),
])->dispatch()
   ->allOnQueue('first')
   ->allOnConnection('connection1');
  • JobA will run on first queue and on connection1.
  • JobB on second queue and on connection2.
  • JobC on first queue and on connection1.

The new allOnQueue and allOnConnection methods will instruct the dispatcher to use the given queue/connection if no queue/connection was set explicitly on each job.

The old behaviour remains the same thus this PR is not a breaking change.

@@ -96,6 +138,10 @@ public function dispatchNextJobInChain()
if (! empty($this->chained)) {
dispatch(tap(unserialize(array_shift($this->chained)), function ($next) {
$next->chained = $this->chained;
$next->chainConnection = $this->chainConnection;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not support having the chain connection change with the next job having the queue/connection already set...

$next->chainConnection = $next->chainConnection??$this->chainConnection;

Copy link
Member Author

@themsaid themsaid Oct 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we don't want to do that, the chain connection is declared once and used as a default connection for when no connection is specified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I am not sure why that was not communicated on my pull request. Keeps it simpler, Thanks.

@Artistan
Copy link
Contributor

#21568 (comment)
FYI, I was told this would need to be a 5.6 feature...

@themsaid
Copy link
Member Author

@Artistan because in your PR you've changed the method signature.

@taylorotwell
Copy link
Member

So you call these methods after dispatch?

@themsaid
Copy link
Member Author

@taylorotwell yes, the methods exist in PendingDispatch

@taylorotwell
Copy link
Member

Ah OK I see.

@taylorotwell taylorotwell merged commit 15d3f80 into laravel:5.5 Nov 2, 2017
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