Move the Queue system to Dependency Injection

We should instead move the Queue system to use Dependency Injection and drupal_container() so that it can be lazy-instantiated on-demand. This reduces the load on the bootstrap as it will only load the Queue API when it's needed.

To accomplish this, we would do the following:

1. Turn queue() into a QueueFactory class, which handles the creation and holding of queues
2. When creating the drupal_container() $container, we register a "queue" service to the QueueFactory class
3. To retrieve/create a queue, we reference the dependency injection container, as it now holds our Queue factory instead of hitting queue() directly

In code, it would look something like this:

  drupal_container()->register('queue', 'Drupal\Core\Queue\QueueFactory');
  // ...
  $myqueue = drupal_container()->get('queue')->queue('myqueue', $reliable);

In the above example, 'queue' is registered as the QueueFactory, which handles the creation/handling of all the queues, and 'myqueue' is a Queue that we're retrieving from the factory. Since we're sticking the factory itself in the Drupal dependency injection container, the factory isn't created until we call ->get('queue').

The result is a reduced bootstrap and further decoupling since we don't depend on the queue() function.

Here you go.

So now we have a FactoryFactory that returns various Factory classes? I am sorry but this is just hilarious given how Dries didn't write Drupal in Java so many years ago. You are basically saying he made a mistake and now we are rewriting Drupal in almost-Java. But, oh please, go ahead.

Actually, queue() implements the same factory pattern as what is proposed here. The only difference is that it's now referenced through the Container so that we don't need to load it during the Drupal bootstrap.

+1 for #2.

Oh of course go ahead with #2. I am just hoping that by release time we have something less verbose. d()->queue($queuename) for example.

@chx, what are your thoughts on... ->get('queue')['myqueue']... ArrayAccess would allow us to do that sort of thing.

@Rob Loach I thought that was only for php 5.4

Just d()->queue('queuename') and be done? __get is your friend.

Edit: yes, array dereferencing is only 5.4

Anything that is spawned from the DI container will not need to call drupal_container() anyway, since the container can and should be setup to inject such dependent systems into the object on creation, which is exactly the point. So the lengthy drupal_container() incantation is a transitional syntax only. Don't Fear the Arrow. :-)

I'm good with this patch, too. (Although I'd love to see it eventually converted to a component.)

Meh my suggestion doesnt work, so what about di()->queue['queuename']? One __get one arrayaccess.

#1552744: Bootstrap for the Dependency Injection Container and make sure SimpleTest abides to it

Same as the previous patch, with the definition moved in to the container itself with #1552744: Bootstrap for the Dependency Injection Container and make sure SimpleTest abides to it.

While I am absolutely blind how can

-      $queue = queue($queue_name);
+      $queue = drupal_container()->get('queue')->queue($queue_name);

be the same... pattern? whatever. To my eyes, which are totally not pattern based and stuck in the past, this is horrible DX. However we have committed the same for the language system already so I am totally wrong. So, I am no longer up to maintain this. Patch reflects this.

Would a helper function like this help? This is all just syntax, and will likely change once the Kernel is in...

function d($service, $func = NULL, $arg = NULL) {
  $service = drupal_container()->get($service);
  if ($func) {
    if ($arg) {
      return $service->$func($arg);
    }
    else {
      return $service->$func();
    }
  }
  return $service;
}

Then $queue = queue($queue_name); would turn into...

  $queue = d('queue', 'queue', $queue_name);

We could also extend QueueFactory to support __get() and turn it into something like this:

  $queue = d('queue', $queue_name);

Again, this is simply syntax. It's better to actually fix the architecture and un-couple things. The current task is to get rid of all static and global variables so that we have proper scope of our Drupal instances.

I dunno. It's not only the length , it's the complexity. it's absolutely trivial to document , teach, IDE integrate queue() -- it returns a QueueInterface implementing class. To the contrary, try to do either with your new d() or drupal_container->get(). I thought we learned our lesson with form API about how strings describing behavior is tricky to document. If you have a good solution, I am all ears.

Where I want to end up is this:

class Foo {
  protected $queue;
  protected $db;

  public function __construct(Queue $queue, DatabaseConnection $db) {
    $this->queue = $queue;
    $this->db = $db;
  }

  public function doSomething($queue_name) {
    $result = $this->db->query("SELECT Lots of Crap FROM {atable}");
    foreach ($result as $record) {
      $this->queue->createItem($record);
    }
  }
}

No hard-coded dependencies. No long chains of incomprehension. No magic. No undocumentable strings. Nothing that prevents unit tests, reuse, or anything else that's cool. And Foo does not even know that it is being managed by a DIC; all it knows is that it has a queue and a database connection and it is moving data from one to the other.

We're a long way from there. Centralizing all of our various hard-coded front-end functions into a DIC object is a step toward getting there. It is not the final step. What it allows us to do is then refactor those things IN the DIC to be independent of it.

For the above to work, it means Foo, the Queue, and the DB connection all need to be managed through the DIC so that the DIC can put each one in the right place at the right time. This patch puts the Queue in the DIC. Next step would be to put the DB connections in the DIC, and pull from there when setting up a DB-backed Queue. That means we replace db_query() in Queue with $this->db->query(). Now we can unit test Queue with a DB that has just the queue table, or even a fake DB that has no DB behind it at all.

Then we instantiate Foo from the DIC. And the DIC handles injecting all of those things we need. And now the Ewoks dance.

This patch is step one to getting there. There are many more steps to come. Yes, it's already May, but it's only May. Let's not dig our heels in out of fear that the transitional state is no better than the current state; let's move forward toward that final state as quickly as possible, lest we end up stuck in a transitional state out of fear.

[Edit: Fix typo in code sample.]

Well we should consider what we think is reasonable for core to enforce and what's going to be in place to make it reasonable for developers. We're trying to as a community reduce our STUPIDity and that's I think true of everyone in this thread. And its not like we have hard and fast control over the code, if it ends up being onerous, contrib will just write shortcuts. @see ctools

To that end, while I think its noble to pursue removing globals and not being STUPID, the fact is that whether you use the drupal_container() or queue() that relies on the container global, they are both equally dependent on a global system. We're not actually buying anything by imposing some extra typing. More importantly, the only reason we would impose this sort of "pain" on our developers would be to ensure that they use a less painful method but we don't have enough of that in place to really justify it at this point.

So while it might seem worse, I actually think its fine and probably better that for the time being we stick with the queue() wrapper and just update it to use the container for resolution. When we can safetly look at core and the contrib use cases and say "we have in place the structure to inject a queue object into 90% of our use cases" then I think we open an issue to remove queue() and we can easily all get behind it.

I can live with #23.

I like #23, or I'm even good with going back to: $queue = DrupalQueue::get('blah');

Not really sure why we changed that to begin with.

That sounds like needs work.

This, again, belongs to Dries. This is not different from the previous God-object discussions but with deadlines looming the decision needs to be made and it needs to made now. Here are the options.

1. We keep the hook system. In this case, we need to pass in the God-object (it's called dependency injection container now, it was called context last fall and neclimdul tells me it was env years ago Edit: RobLoach tells me it will be the Symfony Kernel soon -- the actual object hardly matters on this level) to every. single. Drupal function and then a lot of those functions become little ugly wrappers with code like new NiceClass($container()->get('queue')->queue($queue_name)) instantiating nice classes like in #22. If you do not pass that variable around but instead call a singleton then you have implemented a global variable. Hardly what we want...
2. Or we make it an absolutely priority to get rid of hooks and make everything happy little Symfony-based #1509164: Use Symfony EventDispatcher for hook system and more.

chx: drupal_container() is the alternative to passing a god object everywhere. But for most things, we want to avoid using even that and pass objects into the object; using a DIC is exactly what reduces or avoids needing to mess around with NiceClass($container()->get('queue')->queue($queue_name)) yourself, because it does it for you. That's what a DIC is: A system that does that stuff for you so you don't have to. :-)

@msonnabaum at #25:

 $queue = DrupalQueue::get('blah'); 

This, and queue() for that matter, are essentially Singletons, and we want to avoid using Singletons.

#28 but how can it do stuff if we are dealing with functions. And yes, we want to avoid Singletons!

 $queue = DrupalQueue::get('blah'); 

This was a Singleton, and we want to avoid using Singletons. Having the drupal_container() architecture in place fixes this problem. It may be an interim solution while the Kernel goes in, but it is a solution that works. The patch is there, and it's green.

I'm not saying don't use DI, I'm just saying stick the drupal_container() mess in a wrapper so we don't make DX worse.

Move queue() to use drupal_container() ? I suppose we could do that.

Sounds to me like everyone more or less agrees with #23, so here's #15 updated to that. As long as we stick to procedural wrappers for now, I don't think this needs feedback from Dries until we have patches for the next level of DI integration. @chx: correct me if you disagree, and with this approach, do you still want to remove yourself from base system maintenance?

Should the factory have a local store or rely on the container for the instance caching? Otherwise looks straight forward and good.

Should the factory have a local store or rely on the container for the instance caching?

Eventually, I think we'll want to move queue to the plugin system where the "mapper" caches instances, but we don't have the plugin system in core yet. We do not want to do drupal_container()->get("queue.$name") for reasons discussed in #1519376: [Meta] Extend Symfony's service container and use that for simple swappable systems and plugin access and discovery (can't compile open-ended service names).

+++ b/core/includes/common.inc
@@ -7657,95 +7657,10 @@ function drupal_get_filetransfer_info() {
+ * Queue items for later processing.
+ *
+ * @see Drupal\Core\Queue\QueueFactory
  */
 function queue($name, $reliable = FALSE) {

We need to figure out a documentation standard for procedural wrappers to DI services. I don't know if a simple @see makes clear that that's where the documentation is for the default implementation, but that a different class can swap out that one.

+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -24,5 +24,8 @@ class ContainerBuilder extends BaseContainerBuilder {
+    $this->register('queue', 'Drupal\\Core\\Queue\\QueueFactory');

I think we have some inconsistencies in HEAD where some places use single backslash (within single quotes) and some use double backslash (search for 'Drupal\ in your IDE to see what I mean). Single quotes allow either. It would be nice for us to pick an approach and stick to it. My preference is single backslash unless there's a compelling reason for double.

+++ b/core/lib/Drupal/Core/Queue/QueueFactory.php
@@ -0,0 +1,101 @@
+  public function queue($name, $reliable = FALSE) {

If the class is called *Factory, should the method be changed to getInstance() (the standard method name for returning an instance that is reused on successive calls)?

We need to figure out a documentation standard for procedural wrappers to DI services -- yes we need to and it needs to include some @param docs but at least type and @return type. See above for IDE integration et al. It's the biggest reason I was so against this d_i thing. However, going this way is like making a decision I asked from Dries. You began on creating the ugly functions wrapping the nice classes of the future. So is this what we want?

With the queue() factory change, I believe that was sneaked into one of the PSR-0 patches, purely because it's consistent with the pattern used for cache(), lock() etc. there wasn't any other particular reason for it.

fwiw I'm very happy with doing this for now, seems like the best possible transitional step:

 function queue($name, $reliable = FALSE) {
-  static $queues;
-  if (!isset($queues[$name])) {
-    $class = variable_get('queue_class_' . $name, NULL);
-    if ($class && $reliable && in_array('Drupal\Core\Queue\ReliableQueueInterface', class_implements($class))) {
-      $class = variable_get('queue_default_reliable_class', 'Drupal\Core\Queue\System');
-    }
-    elseif (!$class) {
-      $class = variable_get('queue_default_class', 'Drupal\Core\Queue\System');
-    }
-    $queues[$name] = new $class($name);
-  }
-  return $queues[$name];
+  return drupal_container()->get('queue')->queue($name, $reliable);

And can we make sure the docs are ok.
* Instantiate and cache a queue.
Should start with a 3th person verb.

Thank you!

fwiw webchick brought up in another issue that language is already using drupal_container() directly. However in that case, it was directly accessing a global, with no previous procedural wrapper (and we do have procedural wrappers for some globals like current_path(), just nothing like language_interface() in this case).

That patch went in only because I wasn't monitoring language issues closely...

Hm. I can't remember what I said in that other issue, but +100 for a queue() wrapper around this for DX (e.g. #39). If we can do a similar language() wrapper around the DiC stuff for language things, all the better.

I guess the only reason not to do this is effulgentsia's benchmarks #1578090: Benchmark/profile kernel showed that PHP function calls are terrible for performance, but this seems like a reasonable trade-off for increased accessibility.

PHP function calls are terrible for performance

That only starts to matter when you get into hundreds or thousands. This varies by processor speed, but a rough guideline is 1000 no-op function calls take 1ms, which can be 1% or more of the total page request time. Having a queue() wrapper, language() wrapper, etc. doesn't mean it must be used everywhere. It can be used in most places, and functions that get called hundreds of times per page request (e.g., t(), url()) can bypass using those wrappers.

I'll repeat what I said in #36 though: IMO, all subsystems that involve factories (queue, cache, etc.) will end up getting refactored as plugins once we have a plugin system in core, and I think we're getting close to having that. So unless moving queue to DI is an urgent requirement, it might not be the best use of people's time to finish this issue, only to have it refactored again later. Meanwhile, I'm sure we have plenty of other systems that don't involve factories that are excellent candidates for moving into DI directly.

Wasn't aware of this issue, #1814496: Make queue a container service went just in.