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.

Sign up for GitHub

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.4] option to add a queue 'prefix' to all tube names #18860

Merged
merged 5 commits into from
Apr 21, 2017
Merged

[5.4] option to add a queue 'prefix' to all tube names #18860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eff266f
option to add a queue 'prefix' to all tube names
browner12 Apr 19, 2017
116cc95
style fix
browner12 Apr 19, 2017
17327b7
add prefix to database queue driver
browner12 Apr 19, 2017
1c40e8f
revert new methods on contract
browner12 Apr 20, 2017
6640139
use `get` method on config repository to access value
browner12 Apr 20, 2017
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
2 changes: 1 addition & 1 deletion src/Illuminate/Queue/BeanstalkdQueue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public function deleteMessage($queue, $id)
*/
public function getQueue($queue)
{
return $queue ?: $this->default;
return $this->getQueuePrefix().($queue ?: $this->default);
Copy link

Choose a reason for hiding this comment

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

I think there would be better to create method in parent class, like getQueueName($queue), and call it in all driver classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I'm following you correctly, the problem with moving the getQueue method to the parent is each driver has a different implementation. take a look at the SqsQueue file, as it's the weirdest of them all.

Copy link

Choose a reason for hiding this comment

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

I meant a little different. I mean create new method like resolveQueueName($queue) and call it in getQueue.

protected function resolveQueueName($queue) : string
{
  return $this->getQueuePrefix() . ($queue ?? $this->default);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i think i'm following you now. correct me if I'm wrong. the code

$this->getQueuePrefix().($queue ?: $this->default)

appears in 3 of the queue drivers, so you're suggesting adding a method in the parent to handle this?

I agree this would work. I don't know if I see a huge benefit adding it. Might just be an additional function/layer that adds to the complexity. I'm gonna leave it as is for now, but try a PR if this gets merged.

}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/Illuminate/Queue/Console/ListenCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,11 @@ protected function getQueue($connection)
{
$connection = $connection ?: $this->laravel['config']['queue.default'];

return $this->input->getOption('queue') ?: $this->laravel['config']->get(
$queue = $this->input->getOption('queue') ?: $this->laravel['config']->get(
"queue.connections.{$connection}.queue", 'default'
);

return $this->laravel['config']->get('queue.prefix', null).$queue;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/Illuminate/Queue/Console/WorkCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,11 @@ protected function logFailedJob(JobFailed $event)
*/
protected function getQueue($connection)
{
return $this->option('queue') ?: $this->laravel['config']->get(
$queue = $this->option('queue') ?: $this->laravel['config']->get(
"queue.connections.{$connection}.queue", 'default'
);

return $this->laravel['config']->get('queue.prefix', null).$queue;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Queue/DatabaseQueue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ public function deleteReserved($queue, $id)
*/
protected function getQueue($queue)
{
return $queue ?: $this->default;
return $this->getQueuePrefix().($queue ?: $this->default);
}

/**
Expand Down
30 changes: 30 additions & 0 deletions src/Illuminate/Queue/Queue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ abstract class Queue
*/
protected $connectionName;

/**
* The queue prefix.
*
* @var string
*/
protected $queuePrefix;

/**
* Push a new job onto the queue.
*
Expand Down Expand Up @@ -188,4 +195,27 @@ public function setContainer(Container $container)
{
$this->container = $container;
}

/**
* Set the queue prefix.
*
* @param string $prefix
* @return $this
*/
public function setQueuePrefix($prefix = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

@browner12 Why are these not called setPrefix and getPrefix? After all, we're already in a Queue class here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there was a method on a parent or child class named similarly, so someone suggested it to help reduce ambiguity.

{
$this->queuePrefix = $prefix;

return $this;
}

/**
* Get the queue prefix.
*
* @return string
*/
public function getQueuePrefix()
{
return $this->queuePrefix;
}
}
13 changes: 12 additions & 1 deletion src/Illuminate/Queue/QueueManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ protected function resolve($name)

return $this->getConnector($config['driver'])
->connect($config)
->setConnectionName($name);
->setConnectionName($name)
->setQueuePrefix($this->getQueuePrefix());
}

/**
Expand Down Expand Up @@ -232,6 +233,16 @@ public function setDefaultDriver($name)
$this->app['config']['queue.default'] = $name;
}

/**
* Get the name of the queue prefix.
*
* @return string
*/
public function getQueuePrefix()
{
return $this->app['config']->get('queue.prefix');
}

/**
* Get the full name for the given connection.
*
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Queue/RedisQueue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ protected function getRandomId()
*/
protected function getQueue($queue)
{
return 'queues:'.($queue ?: $this->default);
return 'queues:'.$this->getQueuePrefix().($queue ?: $this->default);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Queue/SqsQueue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public function pop($queue = null)
*/
public function getQueue($queue)
{
$queue = $queue ?: $this->default;
$queue = $this->getQueuePrefix().($queue ?: $this->default);

return filter_var($queue, FILTER_VALIDATE_URL) === false
? rtrim($this->prefix, '/').'/'.$queue : $queue;
Expand Down
32 changes: 21 additions & 11 deletions tests/Queue/QueueManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Mockery as m;
use PHPUnit\Framework\TestCase;
use Illuminate\Config\Repository;
use Illuminate\Queue\QueueManager;

class QueueManagerTest extends TestCase
Expand All @@ -15,18 +16,21 @@ public function tearDown()

public function testDefaultConnectionCanBeResolved()
{
$config = new Repository([
'queue.default' => 'sync',
'queue.connections.sync' => ['driver' => 'sync'],
]);

$app = [
'config' => [
'queue.default' => 'sync',
'queue.connections.sync' => ['driver' => 'sync'],
],
'config' => $config,
'encrypter' => $encrypter = m::mock('Illuminate\Contracts\Encryption\Encrypter'),
];

$manager = new QueueManager($app);
$connector = m::mock('StdClass');
$queue = m::mock('StdClass');
$queue->shouldReceive('setConnectionName')->once()->with('sync')->andReturnSelf();
$queue->shouldReceive('setQueuePrefix')->once()->with('')->andReturnSelf();
$connector->shouldReceive('connect')->once()->with(['driver' => 'sync'])->andReturn($queue);
$manager->addConnector('sync', function () use ($connector) {
return $connector;
Expand All @@ -38,18 +42,21 @@ public function testDefaultConnectionCanBeResolved()

public function testOtherConnectionCanBeResolved()
{
$config = new Repository([
'queue.default' => 'sync',
'queue.connections.foo' => ['driver' => 'bar'],
]);

$app = [
'config' => [
'queue.default' => 'sync',
'queue.connections.foo' => ['driver' => 'bar'],
],
'config' => $config,
'encrypter' => $encrypter = m::mock('Illuminate\Contracts\Encryption\Encrypter'),
];

$manager = new QueueManager($app);
$connector = m::mock('StdClass');
$queue = m::mock('StdClass');
$queue->shouldReceive('setConnectionName')->once()->with('foo')->andReturnSelf();
$queue->shouldReceive('setQueuePrefix')->once()->with('')->andReturnSelf();
$connector->shouldReceive('connect')->once()->with(['driver' => 'bar'])->andReturn($queue);
$manager->addConnector('bar', function () use ($connector) {
return $connector;
Expand All @@ -61,17 +68,20 @@ public function testOtherConnectionCanBeResolved()

public function testNullConnectionCanBeResolved()
{
$config = new Repository([
'queue.default' => 'null',
]);

$app = [
'config' => [
'queue.default' => 'null',
],
'config' => $config,
'encrypter' => $encrypter = m::mock('Illuminate\Contracts\Encryption\Encrypter'),
];

$manager = new QueueManager($app);
$connector = m::mock('StdClass');
$queue = m::mock('StdClass');
$queue->shouldReceive('setConnectionName')->once()->with('null')->andReturnSelf();
$queue->shouldReceive('setQueuePrefix')->once()->with('')->andReturnSelf();
$connector->shouldReceive('connect')->once()->with(['driver' => 'null'])->andReturn($queue);
$manager->addConnector('null', function () use ($connector) {
return $connector;
Expand Down