This is a sub-issue of #1775842: [meta] Convert all variables to state and/or config systems. This issue will convert the following queue variables to the config/state sub-systems:

  • queue_class_ . $name
  • queue_default_class
  • queue_default_reliable_class
#20 queue-dic-1814496-20.patch14.71 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,663 pass(es). View
#20 queue-dic-1814496-20-interdiff.txt1.14 KBBerdir
#19 queue-dic-1814496-19.patch14.61 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,748 pass(es). View
#19 queue-dic-1814496-19-interdiff.txt7.69 KBBerdir
#16 queue-dic-1814496-16.patch16.77 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,020 pass(es). View
#14 queue-dic-1814496-13.patch15.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 49,963 pass(es), 3 fail(s), and 1 exception(s). View
#10 convert_queue-vars-1814496-10.patch1.98 KBdeviance
FAILED: [[SimpleTest]]: [MySQL] 45,958 pass(es), 39 fail(s), and 16 exception(s). View
#7 convert-queue-vars-1814496-7.patch2.04 KBdeviance
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View
#1 1814496-1-convert-queue-vars.patch4.05 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] 42,281 pass(es), 19 fail(s), and 19 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more


Lars Toomre’s picture

Status: Active » Needs review
4.05 KB
FAILED: [[SimpleTest]]: [MySQL] 42,281 pass(es), 19 fail(s), and 19 exception(s). View

Here is an initial pass at a patch to convert these variables to the config system and the system.queue.yml file. Let's see what the bot thinks.

One open question I had is whether we need to test that the class actually implements QueueInterface. Hence, I added an @todo just before the assignment.

Status: Needs review » Needs work

The last submitted patch, 1814496-1-convert-queue-vars.patch, failed testing.

Lars Toomre’s picture

Adding tags

deviance’s picture

Assigned: Unassigned » deviance
Issue tags: -Configuration system, -State system
deviance’s picture

2.04 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install. View

1) variable names to: queue.class_.$name, queue.default_class and queue.default_reliable_class
2) variables_get to state()->get in and in the update function in system.install
3) updated variable names in function system_update_8031 and changed 31 to 33

deviance’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, convert-queue-vars-1814496-7.patch, failed testing.

deviance’s picture

Status: Needs work » Needs review
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] 45,958 pass(es), 39 fail(s), and 16 exception(s). View

1) variable names back to: class_.$name, class.default and class.default_reliable
2) state()->get to config(''system.queue)->get in and in the update function in system.install
3) function system_update_8033 and changed 33 to 34
4) changed {variable} to 'variable' in db_query statement in function system_update_8034

Berdir’s picture

+++ b/core/includes/common.incundefined
@@ -7211,12 +7211,13 @@ function drupal_get_filetransfer_info() {
 function queue($name, $reliable = FALSE) {
   static $queues;
   if (!isset($queues[$name])) {
-    $class = variable_get('queue_class_' . $name, NULL);
+    $class = config('system.queue')->get('class.' . $name);
     if ($class && $reliable && in_array('Drupal\Core\Queue\ReliableQueueInterface', class_implements($class))) {
-      $class = variable_get('queue_default_reliable_class', 'Drupal\Core\Queue\System');
+      $class = config('system.queue')->get('class.default_reliable');
     elseif (!$class) {
-      $class = variable_get('queue_default_class', 'Drupal\Core\Queue\System');
+      $class = config('system.queue')->get('class.default');
     $queues[$name] = new $class($name);

We need to convert queue() to DIC with a factory class, just like other systems. I'm not sure how this will affect this, it's quite possible that it will actually just do a global $conf, see KeyValue factories.

I think thesw variables are usually used in a similar way as the cache backend configuration, meaning it's configured directly in settings.php. Unlike cache, it might be possible for them to be in actually stored variables, but I'm not sure if this is something that we need to support.

+++ b/core/modules/system/system.installundefined
@@ -2214,6 +2214,28 @@ function system_update_8033() {
+  // First, convert the two default class values.
+  update_variables_to_config('system.queue', array(
+    'queue_default_class' => 'class.default',
+    'queue_default_reliable_class' => 'class.default_reliable'
+  ));
+  // Then, convert any remaining variables in form of 'queue_class_'.$name.
+  $resource = db_query("SELECT name FROM 'variable' WHERE name LIKE 'queue_class%'");
+  while ($data = db_fetch_object($resource)) {
+    $name = str_replace('class.','',$data->name);
+    update_variables_to_config('system.queue', array(
+      $data->name => 'class.'.$name,
+    ));

We should create a single mapping array and then call the helper function once.

Not sure if this is actually necessary.

Status: Needs review » Needs work

The last submitted patch, convert_queue-vars-1814496-10.patch, failed testing.

deviance’s picture

Assigned: deviance » Unassigned

From what I understand (still a novice), converting queue-variables seems to be a little more complicated than writing an update routine. Thus, I am releasing the issue :-(

Berdir’s picture

Title: Convert Queue variables to config/state system » Make queue a container service
Status: Needs work » Needs review
15.99 KB
FAILED: [[SimpleTest]]: [MySQL] 49,963 pass(es), 3 fail(s), and 1 exception(s). View

Ok, here is an initial implementation.

Based on the abstracted factory pattern used by keyvalue and my cache in DIC issue.

I'm not exactly sure when we should use this and when the plugin system, possibly this should use the plugin system but the advantage of this pattern is that it allows a very clean definition in the container where everything is injected.

Status: Needs review » Needs work

The last submitted patch, queue-dic-1814496-13.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
16.77 KB
PASSED: [[SimpleTest]]: [MySQL] 50,020 pass(es). View

Fixed the tests and the accidental change in the key value factory, this should pass the tests.

Wow, that was easier an expected :)

aspilicious’s picture

+++ b/core/includes/common.incundefined
@@ -6719,7 +6719,7 @@ function drupal_get_filetransfer_info() {
+ *   (optional) TRUE if the ordering of items and guaranteeing every item executes at

Exceeds 80 chars

+++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.phpundefined
@@ -0,0 +1,140 @@
+   * Implements Drupal\Core\Queue\QueueInterface::createItem().


+++ b/core/lib/Drupal/Core/Queue/QueueDatabaseFactory.phpundefined
@@ -0,0 +1,47 @@
+namespace Drupal\Core\Queue;
+use Drupal\Core\Database\Connection;

Add a newline between those two lines.

+++ b/core/lib/Drupal/Core/Queue/QueueDatabaseFactory.phpundefined
@@ -0,0 +1,47 @@
+   * @param \Drupal\Core\Database\Connection $connection
+   *   The connection to run against.
+   * @return \Drupal\Core\QueueStore\DatabaseStorage

Add a newline between @param and @return

Small nitpicks, looking great

Berdir’s picture

Thanks for the review.

We were discussing about plugins in IRC, and when plugins should be used and when not. I wasn't sure myself where the distinction is what should be pluggable and for what the abstracted factory pattern from the keyvalue system is enough. So I came up with the following:

Unlike something like the views plugins, blocks, where configuration can be changed all the time, possibly by normal users, this feels more like system level configuration to me. To change this, keyvalue, cache backend (possibly mail as well?), additional configuration is usually required, something like Memcache or MongoDB or a queue application needs to be installed on a server first And it is environment specific, similar to the distinction between $settings and CMI (#1833516: Add a new top-level global for settings.php - move things out of $conf). There also won't be dozens of plugin definitions that need to be discovered and presented to the user.

Does that make sense? :)

Will re-roll this after the settings issue is in and also investigate if we can inject the Settings class, not sure yet if that will be possible.

Berdir’s picture

7.69 KB
14.61 KB
PASSED: [[SimpleTest]]: [MySQL] 49,748 pass(es). View


- Fixed the mentioned coding style issues.
- Improved documentation in the settings class and added a getSingleton() method
- Added it to the container so that we can inject it into the QueueFactory.

=> No more globals :)

Berdir’s picture

1.14 KB
14.71 KB
PASSED: [[SimpleTest]]: [MySQL] 49,663 pass(es). View

Per @chx's review in IRC, made Settings final (the alternative would to use static:: and not self:: but that's rather pointless, this class can not be extended so lets mark it as such) and changed $singleton to $instance.

chx’s picture

Status: Needs review » Reviewed & tested by the community

This patch is really yummy and cleans up queue nicely.

andypost’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Queue/Batch.php
@@ -18,7 +18,7 @@
-class Batch extends System {
+class Batch extends DatabaseQueue {
    * Overrides Drupal\Core\Queue\System::claimItem().

Please fix doc-block comments according current classes

Overrides should be short className::method() following current

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

That was not changed or introduced in this patch. I just had to rename the class that Batch extends from. Coding standards should only be applied to code that is actually changed in a patch.

Also, the place you are referring to has not yet been agreed upon, the issue is still at needs work and being discussed.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great! Committed/pushed to 8.x.

Berdir’s picture should be updated/extended...

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.