Skip to content

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,62 @@ class RabbitMQJob extends BaseJob
}
```

### Use your own RabbitMQQueue class

To use own RabbitMQQueue set `RABBITMQ_WORKER` (or `worker` param) to your queue class.

```php
RABBITMQ_WORKER='App\\Services\\CustomRabbitMQQueue'
```
or

```php
'worker' => env('RABBITMQ_WORKER', App\Services\CustomRabbitMQQueue::class),
```

where queue class is defined like
```php
<?php

namespace App\Queue\Jobs;

use VladimirYuldashev\LaravelQueueRabbitMQ\Queue\RabbitMQQueue;

class CustomRabbitMQQueue extends RabbitMQQueue
{
public function __construct(
AbstractConnection $connection,
string $default,
array $options = []
) {
```

for compatibility queue constructor doesn't receive `$dispatchAfterCommit`, to use parent contructor use extra parameter in config
```php
'worker' => env('RABBITMQ_WORKER', App\Services\CustomRabbitMQQueue::class),
'custom_worker_type' => 'with_dispatch_after_commit',
```

then you can reuse parent constructor
```php
<?php

namespace App\Queue\Jobs;

use VladimirYuldashev\LaravelQueueRabbitMQ\Queue\RabbitMQQueue;

class CustomRabbitMQQueue extends RabbitMQQueue
{
public function __construct(
AbstractConnection $connection,
string $default,
bool $dispatchAfterCommit = false,
array $options = []
) {
```

or skip constructor entirely

## Laravel Usage

Once you completed the configuration you can use the Laravel Queue API. If you used other queue drivers you do not need to
Expand Down
9 changes: 8 additions & 1 deletion src/Queue/Connectors/RabbitMQConnector.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Copy link
Collaborator

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' ]]

$connection,
$config['queue'],
Arr::get($config, 'after_commit', false),
Expand Down Expand Up @@ -95,6 +96,7 @@ protected function createConnection(array $config): AbstractConnection
*/
protected function createQueue(
string $worker,
string $customWorkerType,
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @M-Porter
I some kind of confused

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 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.

AbstractConnection $connection,
string $queue,
bool $dispatchAfterCommit,
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 )
I would prefer it fixed in some compatible way (right now we rely on custom fork)

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
}

Expand Down