Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
4.05 KB

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.

ruth_delattre’s picture

Assigned: Unassigned » ruth_delattre
ruth_delattre’s picture

Assigned: ruth_delattre » Unassigned
Lars Toomre’s picture

Adding tags

ruth_delattre’s picture

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

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

ruth_delattre’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

ruth_delattre’s picture

Status: Needs work » Needs review
FileSize
1.98 KB

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

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.

ruth_delattre’s picture

Assigned: ruth_delattre » 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
FileSize
15.99 KB

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
FileSize
16.77 KB

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

\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

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

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

Berdir’s picture

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

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

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

Status: Fixed » Closed (fixed)

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