-
Notifications
You must be signed in to change notification settings - Fork 424
pass dispatchAfterCommit into custom worker constructor wihtout breaking changes #521
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ public function connect(array $config): Queue | |
|
||
$queue = $this->createQueue( | ||
Arr::get($config, 'worker', 'default'), | ||
Arr::get($config, 'custom_worker_type', 'default'), | ||
$connection, | ||
$config['queue'], | ||
Arr::get($config, 'after_commit', false), | ||
|
@@ -95,6 +96,7 @@ protected function createConnection(array $config): AbstractConnection | |
*/ | ||
protected function createQueue( | ||
string $worker, | ||
string $customWorkerType, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's your reasoning behind making this the 2nd arg? Doing this may break if others rely upon extending this function so we would have to do a major change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @M-Porter
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear: v13.1.0 was a breaking minor version by the laravel library itself. We had to no choice but to follow suite as doing Yes, it's protected, but nothing keeps you from writing your own connector which others do and this would break |
||
AbstractConnection $connection, | ||
string $queue, | ||
bool $dispatchAfterCommit, | ||
|
@@ -106,7 +108,12 @@ protected function createQueue( | |
case 'horizon': | ||
return new HorizonRabbitMQQueue($connection, $queue, $dispatchAfterCommit, $options); | ||
default: | ||
return new $worker($connection, $queue, $options); | ||
switch ($customWorkerType) { | ||
case 'with_dispatch_after_commit': | ||
return new $worker($connection, $queue, $dispatchAfterCommit, $options); | ||
default: | ||
return new $worker($connection, $queue, $options); | ||
} | ||
Comment on lines
+111
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels like a hack rather than a solution to the problem. would like to see a move towards using factory make functions that return a class so we don't have to rely on params being perfectly matched up as long as they all share an underlying contract. this is something @khepin and I have discussed but are yet to really dig into a good solution yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, the library right now have a major bug which blocks real use of it (because call incompatible with Laravel ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, nope, nope This does not only feels like a hack... it is a hack. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nothing is preventing you from creating you own I agree its not great that the constructors are no aligned but the fix is not a hack to make it work how you want it to but update to use factories to work with everyones code. |
||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config item does not follow other connection properties available to all drivers.
Rather put this in the options property [ options => [ 'custom_option' => 'value' ]]