pass dispatchAfterCommit into custom worker constructor wihtout breaking changes #521
Conversation
- align constructors for custom workers and RabbitMQQueue - add extra option 'custom_worker_type' => 'with_dispatch_after_commit', to allow change type of constructor
oprudkyi
changed the title
| @@ -95,6 +96,7 @@ protected function createConnection(array $config): AbstractConnection | |||
| */ | |||
| protected function createQueue( | |||
| string $worker, | |||
| string $customWorkerType, | |||
There was a problem hiding this comment.
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.
Hi @M-Porter
I some kind of confused
- as for me this is a minor change (only internal api has changed, protected method) and this can be released as minor version
- the library by self has recently released breaking changes as minor version , by example https://github.com/vyuldashev/laravel-queue-rabbitmq/releases/tag/v13.1.0
There was a problem hiding this comment.
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 composer update for a patch version would have broken so to mitigate the problem created by laravel that was the only solution.
Yes, it's protected, but nothing keeps you from writing your own connector which others do and this would break parent::createQueue() calls.
| switch ($customWorkerType) { | ||
| case 'with_dispatch_after_commit': | ||
| return new $worker($connection, $queue, $dispatchAfterCommit, $options); | ||
| default: | ||
| return new $worker($connection, $queue, $options); | ||
| } |
There was a problem hiding this comment.
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.
well, the library right now have a major bug which blocks real use of it (because call incompatible with Laravel )
I would prefer it fixed in some compatible way (right now we rely on custom fork)
There was a problem hiding this comment.
nope, nope, nope
This does not only feels like a hack... it is a hack.
There was a problem hiding this comment.
the library right now have a major bug which blocks real use of it
Nothing is preventing you from creating you own $worker class compatible with the underlying Queue contracts and in the constructor doing what you need to. You do not have to extend VladimirYuldashev\LaravelQueueRabbitMQ\Queue\RabbitMQQueue to use this library.
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.
| @@ -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'), | |||
There was a problem hiding this comment.
This config item does not follow other connection properties available to all drivers.
Rather put this in the options property [ options => [ 'custom_option' => 'value' ]]
|
Agreed with adm-bome. |
this is version of #513 without breaking changes (things handled via extra option)
@khepin please check