Batch API: Overriding the queue class and name is not documented

OK, thanks for this issue! I have read through the code and agree with what you are saying here, for Drupal 7.

But the situation is pretty much the same in Drupal 8. In Drupal 8, here is what we need to document, on batch_set():

a) $batch_definition can contain an element 'queue', which is an array containing 'name' and 'class' elements (both must be present).

b) 'name' is a unique identifier for the queue.

c) 'class' is the name of a class that implements \Drupal\Core\Queue\QueueInterface, and it must have a constructor with two arguments: $name and a \Drupal\Core\Database\Connection object. Typically, the class will either be 'Drupal\Core\Queue\Batch' or 'Drupal\Core\Queue\BatchMemory'. The class name must include the namespace but not start with a backslash.

For Drupal 7, the only thing that changes is (c):

'class' is the name of a class that implements DrupalQueueInterface; typically, this would be 'BatchQueue' or 'BatchMemoryQueue'. The constructor just takes $name, and the class name is not namespaced, so the class must be loaded or auto-loadable (the class file needs to be in the files section of the module.info file).

Awesome. Tagging for Amsterdam.

I think that since jhodgdon laid it out so neatly, one sprinter can implement the function documentation and another can review.
But I'd be happy to 2nd-review it on site or hear your ideas, if necessary. Ping me in that case.

Also feel free to upload D7 patch in addition :) (named -D7.patch as per https://www.drupal.org/patch/submit, don't change version/status until committed)

Attaching patch. Changes are based on issue description and comments.

Patch applies without problems and does precisely what it is required to do - changes the comments as stated in the summary. Marking as RTBC

Thanks! This patch needs a bit of work:

a) We do not have *....* in documentation anywhere else:

+ *   - queue: Overrides class used for queue. An array with following *required*
+ *     items

Just leave out the * * please. Also instead of "items" how about "elements"?

b) list formatting:

+ *     items
+ *     - name: Unique identifier for the queue
 *     - class: name of a class that implements \Drupal\Core\Queue\QueueInterface.

- Line before a list should end in :
- Each list item (after the : if there is one) should start in a capital letter and end in .
See https://www.drupal.org/node/1354#lists

c)

+ *       be 'Drupal\Core\Queue\Batch' or 'Drupal\Core\Queue\BatchMemory'.

Do not enclose class names in ' and the namespace should start with \ in documentation.

d)

+ *     @code
+ *     'queue' => array(
+ *       'name' => 'my_custom_batch_queue',
+ *       'class' => 'MyCustomBatchMemoryQueue',
+ *     )

This example is probably not necessary. But if you want to include an example, please precede it by a line saying something like "For example:" and also since the instructions say to include the namespace, how about putting one in the example? In d8 all classes have namespaces.

Submitting a patch .

Maybe I shouldn't get in here too, but I saw a superfluous single quote and the first line does not end in ':' as jhodgdon requested.

Some of the wording of patch #4 (at least partially) restored.

Adding a colon (:).

+ *   - queue: Array containing two elements:
+ *     - name: (required) Unique identifier for the queue.
+ *     - class: (required) A class that implements \Drupal\Core\Queue\QueueInterface,
+ *       and it must have a constructor with two arguments: $name and a
+ *       \Drupal\Core\Database\Connection object. Typically, the class will either be
+ *       \Drupal\Core\Queue\Batch' or \Drupal\Core\Queue\BatchMemory. The class name
+ *       must include the namespace but not start with a backslash.

This patch is getting pretty close!

There is still an extra ' on the 6th line.

Also... I think the part about the class is a bit confusing. Maybe in the first line it should say "The name of a class that implements \Drupal\Core\Queue\QueueInterface" and the part about the namespace and backslash should be right after that? Because as it is, when I got to that last sentence, I was pretty confused about what it meant. So how about: "The name of a class that implements \Drupal\Core\Queue\QueueInterface, including the full namespace but not starting with a backslash." Then continue with the part about the constructor and typical classes.

Also... In batch_set() above this patch, it says that all of these elements are optional unless indicated. So it would be useful here to say what the default queue is, if you don't override it... So I guess in the first line of this patch, it would be good to say something like:

- queue: An override of the default queue. An array with two elements:

Re-rolling the patch with changes.

OK, this looks good!

A couple more things to fix:

a) What is the default queue? That would be helpful to say.

b) Formatting -- these lines need rewrapping:

+ *       not starting with a backslash. It must
+ *       have a constructor with two arguments: $name and a

Each line should be as close to 80 characters as possible without going over.

Re-rolling with changes .

The line wrapping is closer but not really ideal... there are two lines over 80 characters, and the line ending in "namespace" and the next one could both have another word or two on them.

Also, we still don't have any indication of what the default queue is.

Wow, having more discussions with Palashvijay4 in IRC. This is definitely not a Novice issue... I think we've figured out what to document and hopefully there will be a new patch shortly! :)

In short:
- There's another element called 'progressive', which defaults to TRUE. TRUE means "expected to take more than one run" and FALSE means "must finish in one run so this batch can stay in memory".
- The queue class defaults to BatchMemory if progressive is FALSE, and Batch if progressive is TRUE.

Anyway that is a rough idea... hopefully we'll have a new patch!

@Palashvijay4O - if you have a question about this issue, please ask it here!

Looks pretty good! A few things to fix:

a) We should talk of a "Boolean" and "TRUE" and "FALSE" in documentation. Not "boolean", "true", "True", etc. (capitalization).

b)

+ *   - queue: An override of the default queue (with name and class fields
+ *     optional). An array containing two elements:
+ *     - name: (required) Unique identifier for the queue.
+ *     - class: (required) The name of a class that implements

The first line says name/class are optional, but in the list it says they are required. They cannot both be correct! Maybe neither is? According to the text below, it looks like class is optional because it has defaults... and if you look at
https://api.drupal.org/api/drupal/core!includes!form.inc/function/_batch...
you can see that both of these are optional. So they should not be marked (required).

c)

 *       with two arguments: $name and a \Drupal\Core\Database\Connection
+ *       object. Typically, the class will either be \Drupal\Core\Queue\Batch or
+ *       \Drupal\Core\Queue\BatchMemory.
+ *     Defaults to Batch if progressive is false (default), or to BatchMemory is
+ *     true.

Those last two lines should be wrapped into the previous bullet item. And the part about BatchMemory doesn't quite make sense, should say "... or to BatchMemory if progressive is TRUE" I think?

d) In (c) we see that progressive has a default. So in the list above we should document that default.

Incorporated changes as per Comment #24, please review.

Thanks for the new patch! A few comments:

1. +++ b/core/includes/form.inc
   @@ -681,6 +681,21 @@ function template_preprocess_form_element_label(&$variables) {
   + *   - progressive: (default) A Boolean that indicates whether or not the batch
   + *     needs to run progressively. TRUE indicates that the batch will run in
   + *     more than one run. FALSE indicates that the batch will finish in a single
   + *     run.
   + *   - queue: An override of the default queue (with name and class fields
   

   (default) is not quite right. That should be removed, and instead where it says "FALSE indicates..." make it say "FALSE(default) indicates..."

2. +++ b/core/includes/form.inc
   @@ -681,6 +681,21 @@ function template_preprocess_form_element_label(&$variables) {
   + *     optional). An array containing two elements:
   + *     - name: Unique identifier for the queue.
   + *     - class: The name of a class that implements
   + *       \Drupal\Core\Queue\QueueInterface, including the full namespace but not
   + *       starting with a backslash. It must have a constructor with two
   + *       arguments: $name and a \Drupal\Core\Database\Connection object.
   + *       Typically, the class will either be \Drupal\Core\Queue\Batch or
   + *       \Drupal\Core\Queue\BatchMemory.
   + *     Defaults to Batch if progressive is FALSE (default), or to BatchMemory if
   + *     progressive is TRUE.
   

   The part about "Defaults to" should be wrapped with the rest of the part about the class, not separated. It's about the defaults for the class piece. And I would leave out the (default) in this area, since that is indicated up above with progressive's documentation.

Please review, updated as per last suggestion.

Thanks! Mostly there, a few little issues:

1. +++ b/core/includes/form.inc
   @@ -681,6 +681,19 @@ function template_preprocess_form_element_label(&$variables) {
   + *     one run. FALSE(default) indicates that the batch will finish in a single run.
   

   Line passes 80 characters.

2. +++ b/core/includes/form.inc
   @@ -681,6 +681,19 @@ function template_preprocess_form_element_label(&$variables) {
   + *       \Drupal\Core\Queue\BatchMemory. Defaults to Batch if progressive is
   + *       FALSE, or to BatchMemory if progressive is TRUE.
     */
   

   Wondering if this is backwards? Yes, it is -- here is what _batch_populate_queue() does:

        'class' => $batch ['progressive'] ? 'Drupal\Core\Queue\Batch' : 'Drupal\Core\Queue\BatchMemory',
    

+++ b/core/includes/form.inc
@@ -681,6 +681,20 @@ function template_preprocess_form_element_label(&$variables) {
+ *     one run. FALSE(default) indicates that the batch will finish in a single

Total nitpick (sorry!): probably need a space before (default) here.

Rest looks perfect!

Great, thanks! [Docs only, unfrozen in beta]

The patch in #31 was correct, not the patch in #33, which has a formatting problem.

@prajaankat: This issue was fine with the patch in #31. I am confused as to why you assigned it to yourself and uploaded new patches. Please find a different issue to work on that actually needs a new patch, thanks!

I committed the patch in #31 to 8.0.x (Docs patches are not frozen in Beta).