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 3 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
15 changes: 15 additions & 0 deletions src/Illuminate/Contracts/Queue/Queue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,19 @@ public function getConnectionName();
* @return $this
*/
public function setConnectionName($name);

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

/**
* Get the queue prefix.
*
* @return string
*/
public function getQueuePrefix();
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing interfaces is a breaking change. I believe this should target 5.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ntzm since the method is in the abstract parent class, all the driver implementations will inherit it, so everything will work fine. do people define implementations that don't extend the abstract class?

on principle I agree with you. some projects seem more lax when adding new methods to the interface. can an owner comment on this?

Is the simple solution to remove it from the contract for 5.4? as I stated no one is required to use the prefix, so we'd be fine without it.

Copy link
Member

Choose a reason for hiding this comment

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

Is the simple solution to remove it from the contract for 5.4? as I stated no one is required to use the prefix, so we'd be fine without it.

Yes it should not be part of the contract in 5.4, and only do so in 5.5. That how it being done before and should remain so.

}
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 isset($this->app['config']['queue.prefix']) ? $this->app['config']['queue.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.

Can't we do return $this->app['config']['queue.prefix'] ?? null here? IIRC Laravel now requires PHP >= 7, doesn't it? Or is it only on master?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only on master. 5.5 will be PHP 7+, this is targeting 5.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yah, i had originally written ?? and then caught myself when I realize I was on 5.4

Copy link
Member

Choose a reason for hiding this comment

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

return $this->app['config']->get('queue.prefix')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it appears $this->app['config'] returns an array, not an object, resulting in a 'call to member function get() on array' error.

Copy link

Choose a reason for hiding this comment

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

$this->app['config'] must return object. Object of class Illuminate\Config\Repository

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 don't believe I have any control over that. I'm using it identically to other places in the file.

Copy link

@evsign evsign Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

$this->app['config']->get(''queue.prefix') and $this->app['config']['queue.prefix'] is have identical behavior. And there no need in isset check, because if this key ('queue.prefix') is not exists then default value (null) will be returned.
Part of the config repository code:

class Repository implements ArrayAccess, ConfigContract
{
.....
    /**
     * Get the specified configuration value.
     *
     * @param  string  $key
     * @param  mixed   $default
     * @return mixed
     */
    public function get($key, $default = null)
    {
        return Arr::get($this->items, $key, $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.

I see what's going on. In an actual application it returns the config repository, but in the test it is returning a simple array. I've made @taylorotwell 's suggested change and updated the tests to use a config Repository rather than just the array.

}

/**
* 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
3 changes: 3 additions & 0 deletions tests/Queue/QueueManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function testDefaultConnectionCanBeResolved()
$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 @@ -50,6 +51,7 @@ public function testOtherConnectionCanBeResolved()
$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 @@ -72,6 +74,7 @@ public function testNullConnectionCanBeResolved()
$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