[5.6] Chained queue name and connection settings. #21568

Artistan · 2017-10-06T19:10:20Z

Allow managing chain queue an connections that will persist through the chain and can be changed at any time in the chain to persist from that point. Does not break or affect current functionality.

#21528

…he chain and can be changed at any time in the chain to persist from that point. Does not break or affect current functionality.

Artistan · 2017-10-06T19:22:19Z

this could be rolled into 5.5 also, since it does not break the current functionality of the chained jobs.

paulofreitas · 2017-10-07T00:31:07Z

@Artistan Rolling to 5.5 branch would be a BC break. Every method signature change of a contract or inheritable class/trait are a potential BC break.

Look, for example, that the Illuminate\Bus\Queueable trait is used in every queue job created. If for whatever reason people overload the chain() method in the job class this would break existing applications. That said, this is fine for 5.6. 😃

taylorotwell · 2017-10-09T16:56:19Z

src/Illuminate/Bus/Queueable.php

                $next->chained = $this->chained;
+                // use the chain setting if this job is not specifically set.
+                $next->onConnection($next->connection ?: $this->chain_connection);


You said this doesn't change existing behavior but this appears to do so. Why are you using the chain connection here?

The default chain_connection is null. So is the next job connection.
If you set the chain connection, then the next job should use it as its connection if is it not specifically set on that job. I wrote test cases for these things also.

taylorotwell · 2017-10-09T16:56:30Z

Your formatting does not match the rest of the framework.

Artistan · 2017-10-10T11:52:12Z

@taylorotwell - can you explain about the formatting please. I can refactor to be more consistent.

GrahamCampbell · 2017-10-11T18:58:49Z

src/Illuminate/Bus/Queueable.php

@@ -74,14 +88,45 @@ public function delay($delay)
    /**
     * Set the jobs that should run if this job is successful.
     *
-     * @param  array  $chain
+     * @param      $chain
+     * @param null $queue


This doc is not correct I'm afraid. This says that you can only pass null through.

@GrahamCampbell - can you approve this change review please.

GrahamCampbell · 2017-10-11T18:59:18Z

src/Illuminate/Bus/Queueable.php

+     *
+     * @var string|null
+     */
+    public $chain_connection = null;


Please use camelCase.

Artistan · 2017-10-11T20:28:12Z

Updated formatting to be more consistent with the framework per @GrahamCampbell suggestions.

jmarcher · 2017-10-13T13:27:09Z

src/Illuminate/Bus/Queueable.php

@@ -95,7 +140,19 @@ public function dispatchNextJobInChain()
    {
        if (! empty($this->chained)) {
            dispatch(tap(unserialize(array_shift($this->chained)), function ($next) {
+                if (in_array('Illuminate\Bus\Queueable', class_uses_recursive($next))) {


I think is better to negate this condition than having an empty if body

Artistan · 2017-10-18T12:00:52Z

updated per @jmarcher suggestions.

Artistan · 2017-10-18T18:17:41Z

@GrahamCampbell - can you check your change requests

jmarcher · 2017-10-18T19:35:51Z

src/Illuminate/Bus/Queueable.php

@@ -95,7 +140,18 @@ public function dispatchNextJobInChain()
    {
        if (! empty($this->chained)) {
            dispatch(tap(unserialize(array_shift($this->chained)), function ($next) {
+                /* @var \Illuminate\Bus\Queueable $next */


This comment can be removed

jmarcher · 2017-10-18T19:38:47Z

src/Illuminate/Bus/Queueable.php

@@ -95,7 +140,18 @@ public function dispatchNextJobInChain()
    {
        if (! empty($this->chained)) {
            dispatch(tap(unserialize(array_shift($this->chained)), function ($next) {
+                /* @var \Illuminate\Bus\Queueable $next */
+                if (! in_array('Illuminate\Bus\Queueable', class_uses_recursive($next))) {


Should this check be checking if Illuminate\Contracts\Queue\ShouldQueue is implemented?

not sure. should it check both?
here is the 5.5 current ....

/** * Dispatch the next job on the chain. * * @return void */ public function dispatchNextJobInChain() { if (! empty($this->chained)) { dispatch(tap(unserialize(array_shift($this->chained)), function ($next) { $next->chained = $this->chained; })); } }

I suppose it could check on the chain method also, so it would not allow adding non-Queueable jobs to the chain.
Originally i added it because I had a test fail without much warning.

jmarcher · 2017-10-18T19:38:57Z

src/Illuminate/Bus/Queueable.php

+                if (! in_array('Illuminate\Bus\Queueable', class_uses_recursive($next))) {
+                    throw new \Exception('Trying to dispatch an object that is not Queueable');
+                }
+                // pass the chain settings on to the next job in the chain, IF this job does not have a new chain settings set...


Remove this comment

jmarcher · 2017-10-18T19:39:16Z

src/Illuminate/Bus/Queueable.php

+                // pass the chain settings on to the next job in the chain, IF this job does not have a new chain settings set...
+                $next->onChainConnection($next->chainConnection ?: $this->chainConnection);
+                $next->onChainQueue($next->chainQueue ?: $this->chainQueue);
+                // array of remaining jobs...


This comment is also useless

jmarcher · 2017-10-18T19:39:42Z

src/Illuminate/Bus/Queueable.php

                $next->chained = $this->chained;
+                // use the chain setting if this job is not specifically set.


Remove this comment it says the same as the code

jmarcher · 2017-10-18T19:40:42Z

src/Illuminate/Foundation/Bus/Dispatchable.php

     * @return \Illuminate\Foundation\Bus\PendingChain
     */
-    public static function withChain($chain)
+    public static function withChain(array $chain, string $chain_queue = null, string $chain_connection = null)


Use Camel Case for method parameters

jmarcher · 2017-10-18T19:41:20Z

src/Illuminate/Foundation/Bus/PendingChain.php

     * @return void
     */
-    public function __construct($class, $chain)
+    public function __construct($class, $chain, $chain_queue = null, $chain_connection = null)


Use camel case for method parameters

jmarcher · 2017-10-18T19:41:29Z

src/Illuminate/Foundation/Bus/PendingChain.php

+     *
+     * @var string|null
+     */
+    public $chain_queue = null;


Use camel case here

You forgot this one

Here as well initialization not needed

jmarcher · 2017-10-18T19:41:37Z

src/Illuminate/Foundation/Bus/PendingChain.php

+     *
+     * @var string|null
+     */
+    public $chain_connection = null;


Use camel case here

jmarcher · 2017-10-18T20:55:36Z

src/Illuminate/Foundation/Bus/Dispatchable.php

     * @return \Illuminate\Foundation\Bus\PendingChain
     */
-    public static function withChain($chain)
+    public static function withChain(array $chain, string $chainQueue = null, string $chainConnection = null)


Remove type hint, this won't work

jmarcher · 2017-10-18T20:55:54Z

src/Illuminate/Foundation/Bus/PendingChain.php

+     *
+     * @var string|null
+     */
+    public $chain_queue = null;


You forgot this one

jmarcher · 2017-10-18T21:00:42Z

src/Illuminate/Foundation/Bus/PendingChain.php

+     *
+     * @var string|null
+     */
+    public $chainConnection = null;


Initialization not needed

jmarcher · 2017-10-18T21:01:01Z

src/Illuminate/Foundation/Bus/PendingChain.php

+     *
+     * @var string|null
+     */
+    public $chain_queue = null;


Here as well initialization not needed

Artistan · 2017-10-19T12:06:40Z

updated again, Not sure why @GrahamCampbell review is still there.

jmarcher · 2017-10-19T12:14:20Z

I can not see any of the changes I requested

Artistan · 2017-10-19T12:15:45Z

@jmarcher -- this should be everything you mentioned.
ec8c3ab

Artistan · 2017-10-19T12:25:31Z

@jmarcher -- additional commits added from your latest reviews. :) thanks!

taylorotwell · 2017-10-20T13:19:45Z

src/Illuminate/Bus/Queueable.php

@@ -95,7 +140,14 @@ public function dispatchNextJobInChain()
    {
        if (! empty($this->chained)) {
            dispatch(tap(unserialize(array_shift($this->chained)), function ($next) {
+                if (! in_array('Illuminate\Bus\Queueable', class_uses_recursive($next))) {
+                    throw new \Exception('Trying to dispatch an object that is not Queueable');
+                }


I don't understand why this is here. It should be removed. If the methods don't exist, we will get an error anyways.

taylorotwell · 2017-10-20T13:20:12Z

There is still a lot of bad formatting in this. @themsaid maybe you can take a shot at supporting this stuff.

themsaid · 2017-10-20T13:57:06Z

@taylorotwell here's my attempt on this: #21765

Artistan · 2017-10-20T14:47:00Z

I would greatly appreciate someone could explain "bad formatting" someday. Seems to be more of a preference in naming? Thanks for all the feedback. I hope @themsaid can finish off the feature with good formatting 😁

Charles Peterson added 5 commits October 6, 2017 14:06

Allow managing chain queue an connections that will persist through t…

3b701fd

…he chain and can be changed at any time in the chain to persist from that point. Does not break or affect current functionality.

formatting updates.

6e6d738

couple more formating updates.

d60a8c7

spacing ...

128cb5e

returns and more spacing.

d319406

taylorotwell reviewed Oct 9, 2017

View reviewed changes

GrahamCampbell requested changes Oct 11, 2017

View reviewed changes

GrahamCampbell reviewed Oct 11, 2017

View reviewed changes

Charles Peterson added 2 commits October 11, 2017 15:23

camelCase public variables. :)

7fc438a

fix docblock specifications.

3205698

jmarcher suggested changes Oct 13, 2017

View reviewed changes

Charles Peterson added 2 commits October 16, 2017 15:06

remove the empty if statement

a39bf13

fix formatting

9ae522a

Charles Peterson added 2 commits October 18, 2017 14:30

updated Readme, for me.

68e2c10

oops

869ff31

jmarcher suggested changes Oct 18, 2017

View reviewed changes

camelCases and Comments per @jmarcher suggestions.

ec8c3ab

jmarcher suggested changes Oct 18, 2017

View reviewed changes

Initialization not needed

6ab0cf2

updated type hints for nullable parameters.

7c16195

taylorotwell reviewed Oct 20, 2017

View reviewed changes

taylorotwell closed this Oct 20, 2017

Artistan mentioned this pull request Oct 20, 2017

[5.5] add allOnQueue and allOnConnection for job chaining #21765

Merged

		$next->chained = $this->chained;
		// use the chain setting if this job is not specifically set.

[5.6] Chained queue name and connection settings. #21568

[5.6] Chained queue name and connection settings. #21568

Conversation

Artistan commented Oct 6, 2017

Artistan commented Oct 6, 2017

paulofreitas commented Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylorotwell commented Oct 9, 2017

Artistan commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artistan commented Oct 11, 2017

Choose a reason for hiding this comment

Artistan commented Oct 18, 2017

Artistan commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Artistan commented Oct 19, 2017

jmarcher commented Oct 19, 2017

Artistan commented Oct 19, 2017

Artistan commented Oct 19, 2017

Choose a reason for hiding this comment

taylorotwell commented Oct 20, 2017

themsaid commented Oct 20, 2017

Artistan commented Oct 20, 2017

paulofreitas commented Oct 7, 2017 •

edited

Loading