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
Files: 
CommentFileSizeAuthor
#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 ]

Comments

Status:Active» Needs review
StatusFileSize
new4.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.

Adding tags

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

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

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

changed:
1) variable names back to: class_.$name, class.default and class.default_reliable
2) state()->get to config(''system.queue)->get in common.inc 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

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

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 :-(

Title:Convert Queue variables to config/state systemMake queue a container service
Status:Needs work» Needs review
StatusFileSize
new15.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.

Status:Needs work» Needs review
StatusFileSize
new16.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 :)

+++ 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().

\Drupal\Core\...

+++ 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

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.

StatusFileSize
new7.69 KB
new14.61 KB
PASSED: [[SimpleTest]]: [MySQL] 49,748 pass(es).
[ View ]

Ok.

- 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 :)

StatusFileSize
new1.14 KB
new14.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.

Status:Needs review» Reviewed & tested by the community

This patch is really yummy and cleans up queue nicely.

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 http://drupal.org/coding-standards/docs#classes

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.

Status:Reviewed & tested by the community» Fixed

Looks great! Committed/pushed to 8.x.

http://drupal.org/node/1479680 should be updated/extended...

Status:Fixed» Closed (fixed)

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