Introduce a batch builder class to make the batch API easier to use

Love it

Re-rolled patch.

Given that we should be moving towards fully OOP, would it be a good idea to move the functionality from batch_*() into the Batch class and make the functions deprecated?

I've added the rest of the setters from the list specified in the batch_set() documentation.

Then I moved the functionality from batch_set(), batch_process() and batch_get() into the class, leaving the existing functions as deprecated with redirects to the class's implementation.

I've got most of the member functions to allow for piping the operations together; e.g.

Batch::create('Title')->addOperation('callback')->enqueue()->process();

The exceptions to this are:

• process() - returns a redirect response or null
• toArray() - return an array representation
• getBatches() - returns an array of batch sets

I made an interdiff, but that was larger than the patch :)

+1 on OOP-ifying batch API. The approach looks good. I like the fluent interface.

Just some coding standards stuff:

$ cd core/
$ ../vendor/bin/phpcs -p -s lib/Drupal/Core/Batch/
E...


FILE: ...aulmitchum/projects/drupal8/core/lib/Drupal/Core/Batch/Batch.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
   3 | ERROR | [x] Namespaced classes, interfaces and traits should not
     |       |     begin with a file doc comment
     |       |     (Drupal.Commenting.FileComment.NamespaceNoFileDoc)
 201 | ERROR | [x] Expected "bool" but found "boolean" for parameter
     |       |     type
     |       |     (Drupal.Commenting.FunctionComment.IncorrectParamVarName)
 407 | ERROR | [x] Expected 1 space before "=>"; 0 found
     |       |     (Drupal.WhiteSpace.OperatorSpacing.NoSpaceBefore)
 407 | ERROR | [x] Expected 1 space after "=>"; 0 found
     |       |     (Drupal.WhiteSpace.OperatorSpacing.NoSpaceAfter)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 273ms; Memory: 5.5Mb

Fixed coding standards errors.

+++ b/core/lib/Drupal/Core/Batch/Batch.php
@@ -0,0 +1,458 @@
+    $this->setTitle(is_null($title) ? t('Processing') : $title);
+    $this->setInitMessage(t('Initializing.'));
+    $this->setProgressMessage(t('Completed @current of @total.'));
+    $this->setErrorMessage(t('An error has occurred.'));
...
+        'theme' => \Drupal::theme()->getActiveTheme()->getName(),
...
+      \Drupal::moduleHandler()->alter('batch', self::$batch);
...
+        $request = \Drupal::request();
...
+        \Drupal::service('batch.storage')->create(self::$batch);

Hey lookit all those services which should be injected. :-) And we wonder: Where will the injection come from? If we inject them into this object, will the batch store the original text or the translated text? Will all batches be processed with a translation service present, or when there's an active theme?

This is the classic too-many-responsibilities problem.

There should be two classes:

1) A data object which is stateful and can be created from a batch array or request/session object. It's responsible for holding the batch info, including untranslated strings from the batch array. It's probably an iterator.

2) A batch processor which is stateless, and can have services injected and can do all the translations for display. It triggers the batch tasks and then reports to the user. It could even be a service.

This way the batch definition will be stored on a more 1-to-1 basis in the session leading to comprehensibility. The definition will be easier to test for behaviors because all it does is create itself based on an array.

The processor will be reduced in complexity because it has fewer responsibilities, could be a service because its stateless, and will be easier to test.

1. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,458 @@
   +      _batch_populate_queue(self::$batch, $index);
   ...
   +        _batch_populate_queue(self::$batch, $key);
   

   Boo. :-)

2. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,458 @@
   +        require_once DRUPAL_ROOT . '/core/includes/batch.inc';
   +        _batch_process();
   

   VERY boo. :-)

The underscores at the beginning of those function names should be a clue that they're not part of the API, so move them to the processor class.

Leaving as 'needs review' because my opinion probably isn't the be-all end-all. :-)

I've created a new processor class and moved the processing and injections into there. I've left wrappers in \Drupal\Core\Batch\Batch to keep chaining.

The bad $queue = _batch_queue($batch_set); and nasty require_once DRUPAL_ROOT . '/core/includes/batch.inc'; _batch_process(); are still there because they are called by Form API and the install process directly, so Form API & install process will need to be rewritten before the functions can be moved into a class as protected/private methods.

I've removed process() from /core/lib/Drupal/Core/Batch/Batch.php and added an example of how to use the class, as suggested be dawehner in #13.

Also, the output has been changed so the tests pass.

A couple of changes to the example.

Moved the functionality of _batch_queue() into BatchQueueController::getQueueForBatch() but left a wrapper.

I like this patch. A lot. I have found a number of code style and relatively minor issues, though...

1. +++ b/core/includes/form.inc
   @@ -783,94 +739,21 @@ function batch_set($batch_definition) {
   + * @param string $redirect_callback
   

   IMHO this should be callable, not string.

2. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +  protected $operations = [];
   +  protected $title;
   +  protected $init_message;
   +  protected $progress_message;
   +  protected $error_message;
   +  protected $finished;
   +  protected $file;
   +  protected $css = [];
   +  protected $url_options = [];
   +  protected $progressive;
   +  protected $queue;
   

   All of these need a description and @var type annotations.

3. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +   * @param string $title
   +   *   The title.
   

   Missing (optional).

4. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +    $this->setTitle(is_null($title) ? 'Processing' : $title);
   

   Can be shortened slightly: $this->setTitle($title ?: 'Processing'). It grinds my gears that the Elvis operator is so infrequently used in core...

5. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +   * @param string $title
   +   *   The title.
   

   Missing (optional) description.

6. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +  public static function create($title = NULL) {
   +    return new static($title);
   +  }
   +
   

   There doesn't seem to be much of a point to this method -- it's virtually identical to calling new Batch($title). What value is added by having this static factory method?

7. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +   * @param string $callback
   +   *   The callback.
   

   Should be callable, not string.

8. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +  public function setFinishCallback($callback) {
   

   $callback should be type-hinted as callable.

9. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,301 @@
   +   * @param string $message
   +   *   The text to display. Defaults to t('Initializing.').
   

   It doesn't set any default except in __construct() :) And the default string is not translated, so this needs to be adjusted.

10. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   * @param string $message
    +   *   The text to display.  Available placeholders are '@current',
    +   *   '@remaining', '@total', '@percentage', '@estimate' and '@elapsed'.
    +   *   Defaults to t('Completed @current of @total.').
    

    As with setInitMessage(), this method does not set a default (and the default set by the constructor is not translated). So the comment needs fixing.

11. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   * @param string $message
    +   *   The text to display. Defaults to t('An error has occurred.').
    

    Same problem as with setInitMessage and setProgressMessage.

12. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +  public function setFile($filename) {
    +    $this->file = $filename;
    +    return $this;
    +  }
    

    IMHO this should validate the file and throw an exception if it doesn't exist. Garbage in, exception out! :)

13. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   * Sets the location of css files.
    

    s/css/CSS.

14. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   * @param array $filenames
    

    Should be string[].

15. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   *   The css files to be used.
    

    s/css/CSS.

16. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +  public function setCss(array $filenames) {
    +    $this->css = $filenames;
    +    return $this;
    +  }
    

    Is it still possibly to add single CSS files to a page? Don't we need to use libraries now?

17. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +  public function setUrlOptions(array $options = []) {
    

    Having a default value for this method obviates the need for the method at all :)

18. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   *   TRUE indicates that the batch will run in more than one run. FALSE
    +   *   (default) indicates that the batch will finish in a single run.
    

    $this->progressive has no default value, so the comment is not accurate.

19. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +  public function setQueue($name, $class) {
    

    I think this should call class_exists() on $class, and throw an exception if the class is unavailable.

20. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +   * @param string $callback
    +   *   The name of the callback function.
    

    Should be callable.

21. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +  public function addOperation($callback, $arguments = []) {
    

    Both parameters should be type hinted.

22. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +  /**
    +   * Adds a CSS file to be used on the process page.
    +   *
    +   * @param string $filename
    +   *   The CSS file to use.
    +   *
    +   * @return $this
    +   */
    +  public function addCss($filename) {
    +    $this->css[] = $filename;
    +    return $this;
    +  }
    

    Why bother having both this and setCss()? Seems redundant.

23. +++ b/core/lib/Drupal/Core/Batch/Batch.php
    @@ -0,0 +1,301 @@
    +      else {
    +        switch ($key) {
    +          case 'operations':
    +          case 'css':
    +          case 'url_options':
    +            $array[$key] = [];
    +        }
    +      }
    

    Er...wouldn't it just be better to give these class members default values of []?

24. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +  private static $batches = [];
    +  private static $queues = [];
    

    Core generally does not use private, so both of these should be protected. They also need doc comments and @var type annotations.

25. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +  public static function enqueue(Batch $batch) {
    +
    +    // Initialize the batch if needed.
    

    Nit: there's an extra line of whitespace here.

26. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +    }
    +
    +  }
    

    Nit: extra line of whitespace.

27. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +   * @param string $redirect_callback
    +   *   (optional) Specify a function to be called to redirect to the progressive
    +   *   processing page.
    

    Should be callable.

28. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +  public static function process($redirect = NULL, Url $url = NULL, $redirect_callback = NULL) {
    

    $redirect_callback should be type hinted.

29. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +    if (isset(self::$batches)) {
    +
    +      // Add process information.
    

    Nit: extra line of whitespace.

30. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +        require_once DRUPAL_ROOT . '/core/includes/batch.inc';
    

    Use \Drupal::root() instead of DRUPAL_ROOT.

31. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
    @@ -0,0 +1,269 @@
    +        _batch_process();
    

    Shouldn't this be replaced with a method call on the batch object?

The functionality of _batch_process() has been moved to BatchQueueController::processQueue(). The required support functions have also been moved into BatchQueueController.

@phenaproxima

In responce to points in #17

1. As it's the name of a function to be passed (rather than the function itself) this should be a string.
6. Having the create() method allows for code like Batch::create('title')->set... and is a bit neater than (new Batch('title'))->set...
7. As with 1, this is the name of a function/method. Also, I'm not sure how a callable will be managed when being serialized / deserialized. This could be added after this has been committed through a feature request if desired.
8. As 7.
9. As you can't create an object without running __construct() at some point, I don't see a problem with this - unless someone knows different.
   It was decided in #10 not to have translated strings in Batch objects and delay this until they are processed in BatchQueueManager::process(), so they do eventually get translated.
10. As with 9, but the comment will be fixed.
11. As with 9 and 10.
12. The goal of this patch is to move the batch creation into OOP. This is an enhancement and for a feature request issue.
16. This would be a feature request / bug. Although removing this would break backwards compatibility.
17. This allows the cancellation of the URL options with a shorter code snippet. Not sure of a use case but there if needed ;)
19. This is a feature request / bug for a different issue.
20. As 7 and 8.
21. As 7, 8 and 20
22. setCss() is used to add a few files at a time while addCss() is a single file. setCss() can be used to add most of the files with addCss() to add optional additional files. setCss() can also be used to clear the list of files. Also, if one CSS file is to be added, saves having to create an array leading to cleaner code :)
23. These are required in future other parts of the code. The may be set to null through operations on the Batch object. This ensures that the keys are present.
27. As with the other callbacks (7, 8, 20, etc), this is the name of a function. Although serialization would not be as big of a problem in this case.
28. See 28.
30. Removed in patch #18
31. See 30.

As it's the name of a function to be passed (rather than the function itself) this should be a string.

Callable is a weird meta-type -- it can be a string, an array representing a class or object method, or a closure. If it's a string, it's assumed to be the name of a function. So type hinting it as callable should still behave exactly as expected.

As far as serialization goes, it should be pretty safe...but there is no reason why Batch could not implement Serializable if it needs more control over this.

All this being said, there is nothing really to prevent calling code from passing in callables anyway -- I'm just wanting it to be declared explicitly in the doc comments.

@phenaproxima still, it's out of the scope of this patch. I'm trying to replicate existing functionality first, then create new issues to change all the things later. I'm adding this to the "List of Things to Do" along with a few other things I've discovered.

Addressed the following points for #17 - 2, 3, 4, 5, 13, 14, 15, 18, 24, 25, 26 and 29.

Some of the points in #24 are to do with the batch processing part (most of batch.inc). I'm leaving that to be tackled at another time otherwise it would be a very large patch. It's already a long way from "a small DX improvement" :)

The processing still uses arrays while processing and the non-camelcase properties (1) and setting of properties (4) are to make the code to go from array to object and back more compact; I'll change these though.

I don't know wether css will work in the same way (2) but the processing side would need to be re-written to account for libraries. Would removing the "css" part be an API change/break?

I think that having enqueue() as a wrapper function (5) creates nicer code.
Batch::create()->...->enqueue(); instead of BatchQueueController::enqueue(Batch::create()->...); and
$batch = Batch::create()->...; $batch->enqueue(); instead of $batch = Batch::create()->...; BatchQueueController::enqueue($batch);.
I'll improve the comments for the related functions but I'm not sure which option is "best" and, therefore, should be recommended.

Would that be the Drupal class for the container (6)?

I'll get on with the other things in the list first and return to these ones.

P.S. I current have a list of 23 further issues for when this one is committed ;)

A few tweaks from #24.

• Removed $title from the create() method and added a new function, createFromTitle(), which does take $title as a parameter.
• Added more commenting to the enqueue() functions of Batch and BatchQueueController
• Switched to use the class property of \Drupal\Core\Queue\Batch and Drupal\Core\Queue\BatchMemory and used the simple name of the classes. (Although there is some trickery involved as there are two Batch classes under different namespaces.)

The properties have been made camelcase and the setters have been updated. Also, the to/from array methods have been updated.

Requeued for testing w/ 8.4.x

I've updated the summary in response to dawehner in #32.

Also, I've added the Needs reroll tag in response to the test queued by pingwin4eg in #31.

Re-rolled.

Fixed the failing test, and the PHPCS issues.

I agree with #24.6 - I think we should move this to a service and get rid of the static access.

Thoughts?

First crack at removing statics, will continue later.

I have seen inside the rat's nest

module install uninstall test not finishing
looking forward to debugging that as much as you'd look forward to poking yourself in the eye with rusty fork

@larowlan, while I think it's good to move batch API in to being a service, it looks like you're having a bit of trouble doing that.

I would suggest going with the patch in #37 and creating a new ticket for the service implementation.

I've got a list of follow-up tasks that can be done after this issue is merged, although I have to look through it to find out what are still issues.

For reference, here's the list of extra task so far:

1. Batch not using controller properly while executing.
 - remove functionality from core/includes/batch.inc

2. Get install procedure to properly use Batch API.
 : Depends on 1.

3. core/authorize.php does not use the Batch API properly.
 : Depends on 1.

4. META ISSUE. Update core to use the new Batch API.
 : 5 - 16

5. Update module.api.php to reference the refactored Batch API.

6. Update Form API to reference the refactored Batch API.
 - include form.api.php

7. Update config module to use the refactored Batch API.

8. Update dblog module to reference the refactored Batch API.

9. Update locale module to use the refactored Batch API.

10. Update migrate_drupal_ui module to use the refactored Batch API.

11. Update node module to use the refactored Batch API.

12. Update simpletest module to use the refactored Batch API.

13. Update system module to use the refactored Batch API.

14. Update update module to use the refactored Batch API.

15. Update user module to use the refactored Batch API.

16. Update views module to use the refactored Batch API.
 - batch_set()
 - batch_process()
 - batch_get()

17. Batch::setFinishedCallback() should accept a callable as it's parameter. 2401797 #17 - 8

18. Batch::setFile() should validate the parameter to ensure the file exists. 2401797 #17 - 12

19. Change Batch API to use CSS libraries instead of single files. 2401797 #17 - 16

20. Batch::setQueue() should validate the $class parameter to ensure that the class exists. 2401797 #17 - 19

21. Batch::addOperation() should accept a callable as it's first parameter. 2401797 #17 - 21

22. BatchQueueController::process() should accept a callable as it's third ($redirect_callback) parameter. 2401797 #17 - 28

23. Implement css/js libraries.
 : Depends on 1

24. Rename Drupal\Core\Batch\Batch or Drupal\Core\Queue\Batch for better clarity.

25. Implement batch api as a service.

@John Cook

I'm happy to do the non-static in a follow-up *if* we don't mark batch_get/batch_set et al as deprecated.

There is no sense in marking them deprecated in favour of BatchQueueController::{methods} because we'll be deprecating them too.

So in summary, happy to do non static later if

* we don't deprecate existing functions
* we mark the static methods as internal/deprecated because we're going to remove them

Thoughts?

@larowlan, I agree. We should remove the deprecation from _batch_*() and have the static methods marked as internal.

@dawehner, I'll create issues for the list in #49 today.

@John Cook - are you able to re-upload the static one then?

I'll open a new issue to continue with the container approach.

I've re-uploaded the patch from #37 and will fix the issues from #51 shortly.

@larowlan, there is a new ticket, Implement batch api as a service, to continue the work of converting batch API to being a service.

I've removed the deprecation messaged from batch.inc and added them to BatchQueueController. This is in preparation for Implement batch api as a service.

I've fixed the coding standards issue from the test report in #55.

I've created an initial draft change record as requested by dawehner in #57.

thanks John

Sure would be nice to get this in soon. Anyone available to review?

1. +++ b/core/includes/batch.inc
   @@ -207,162 +203,14 @@ function _batch_progress_page() {
     *   An array containing a completion value (in percent) and a status message.
     */
    function _batch_process() {
   ...
    function &_batch_current_set() {
   
   @@ -377,18 +225,7 @@ function &_batch_current_set() {
    function _batch_next_set() {
   
   @@ -398,108 +235,7 @@ function _batch_next_set() {
    function _batch_finished() {
   

   Functions with _names are supposed to be internal, so we should either remove them or deprecate them if we're keeping them and they've got a replacement method.

   The patch removes some other _functions outright, which might be the right thing to do, but we should be consistent. I'm not sure which way to go with this.

2. +++ b/core/includes/form.inc
   @@ -710,60 +712,15 @@ function template_preprocess_form_element_label(&$variables) {
   + * @deprecated as of Drupal 8.2.x, will be removed in Drupal 9.0.0. Create a
   
   @@ -786,94 +743,21 @@ function batch_set($batch_definition) {
   + * @deprecated as of Drupal 8.2.x, will be removed in Drupal 9.0.0. Use
   

   Needs branch update.

@Mile23 The _name functions are left on purpose. This is to be fixed the follow-up issue #2875151: Implement batch api as a service. See larowlan's comment #51 for the basis of this decision.

A re-roll of of patch #56 as there was a change in comment of batch_set().

Updated summary.

After talking to dawehner at DrupalCon Vienna, we agreed to not deprecated the existing functions but to mark the new Drupal\Core\Batch\BatchQueueController as internal. This is because the queue processing is to be moved into a service and may change.

I've marked the _batch_*() functions as internal as well as the class.

I've also done a little code clean-up and implemented changes from comment #62 (specifically points 1 and 2).

As this patch changes procedural code to object orientated, I've added the need for sign-off from a framework manager. BDFL sign-off may also be needed.

Can we consider type hinting on callable instead of string for the callbacks?

Hi @kim.pepper, thank you for your suggestion.

This patch is to get the existing functionality into an object oriented formated code.

Having the callbacks type-hinting as callable is already outlined in #2875316: setFinishedCallback should accept a callable and #2875317: addOperation should accept a callable.

Thanks John Cook. I should have read through the issue history before posting that comment!

I didn't had time to read the patch in detail but I think we should separate the Batch API for its UI implementation so that the API could be used in Drush or everywhere. Then the form implementation could be only a use case of the Batch API. Not sure that the patch is doing this decoupling.

Hi @claudiu.cristea

The Batch API is designed to provide UI feedback to the user and prevent timeouts when processing long running tasks. The UI, of course, isn't available when using drush, drupal console, or cron runs and timeouts don't really apply. In these cases the Queue API should be used instead.

I'm starting to think about refactoring Batch API to use the Queue API instead of implementing it's own version, but this is beyond the scope of this issue.

As there a few people asking for enhancements outside the intended scope of this issue, I've updated the summary to explicitly state what is to be covered at the top.

I noticed that the comment for methods in the new class were copied with the @deprication tags. I've replaced these with @internal tags instead.

Setting back to “needs review” because of false test fails.

1. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   +   * If the callbacks are in the .module file or are static methods of a class,
   +   * then this does not need to be set.
   +   *
   +   * @var
   +   */
   +  protected $file;
   +
   

   I guess it should be type of string

2. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   +   *
   +   * @return $this
   +   */
   +  public function addOperation($callback, $arguments = []) {
   +    $this->operations[] = [$callback, $arguments];
   +    return $this;
   +  }
   

   Missed array type hint for $argument

3. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
   @@ -0,0 +1,604 @@
   +   *   The queue object or null if the queue cannot be created.
   +   */
   +  public static function getQueueForBatch($batch) {
   +    if (isset($batch['queue'])) {
   +      return self::getQueue($batch['queue']['name'], $batch['queue']['class']);
   

   Missed array type hint for $batch

4. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
   @@ -0,0 +1,604 @@
   +   * set in the next request.
   +   *
   +   * @return array
   +   *   An array containing a completion value (in percent) and a status message.
   +   *
   

   Return type should be updated regarding self::finishedProcessing() return types.

5. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
   @@ -0,0 +1,604 @@
   +      else {
   +        // Processing will continue with the current batch set.
   +        $remaining        = $old_set['count'];
   +        $total            = $old_set['total'];
   +        $progress_message = $old_set['progress_message'];
   

   $old_set defined in the while loop, so, potentially,
   variable $old_set might have not been defined

6. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
   @@ -0,0 +1,604 @@
   +      // Total progress is the number of operations that have fully run plus the
   +      // completion level of the current operation.
   +      $current    = $total - $remaining + $finished;
   +      $percentage = Percentage::format($total, $current);
   +      $elapsed    = isset($current_set['elapsed']) ? $current_set['elapsed'] : 0;
   

   The same with $finished variable.

7. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
   @@ -0,0 +1,604 @@
   +      }
   +
   +      return [$percentage, $message, $label];
   +    }
   +    else {
   

   The same with $label variable.

Addressed all of the points raised by @voleger in #82.

Nits, these are mostly opinion, so feel free to ignore, that's why I'm not setting this back to Needs Work.

1. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   + * Stores a domain object for a batch process.
   

   I was going to mention that this should be a value object instead of a domain object.

   But after reading a couple of stack overflow articles about it, I'm not sure anymore.

   However, when I use ag to search for domain object vs value object we get very different results.

   ag "domain object" -Qi --stats
   13 matches
   8 files contained matches

   ag "value object" -Qi --stats
   43 matches
   37 files contained matches
   

   I think that's sufficient data to make this value oject?

2. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   + * $batch = Batch::create('Batch Title')
   ...
   +  public static function create() {
   

   I don't see any option to set the title in the ::create method, but it is documented like it is possible.

3. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   +  public static function createFromTitle($title) {
   +    return static::create()->setTitle($title);
   +  }
   

   I think this should be createWithTitle instead of createFromTitle to be more specific.

4. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   +      if (is_array($batch_definition['css'])) {
   +        $new_batch->setCss($batch_definition['css']);
   +      }
   +      else {
   +        $new_batch->addCss($batch_definition['css']);
   +      }
   

   I don't understand why the second call uses addCss instead of ->setCss([$batch_definition['css']);.

   I think even better would be to use addCss for both, that way we can add default css later and it won't be overwritten.

5. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   +  public function setQueue($name, $class) {
   

   I think this can be rewritten to remove the else.

   if ($name === NULL && $class === NULL) {
     $this->queue = NULL;
     return $this;
   }
   
   $this->queue = ['name' => $name, 'class' => $class];
   return $this;
   
   

6. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,441 @@
   +    if (is_null($name) && is_null($class)) {
   

   I think we should rewrite this as:
   if ($name === NULL && $class === NULL) {

   When I check for is_null vs == NULL with ag, we see that == NULL is used a lot more.
   44 matches
359 matches

7. +++ b/core/lib/Drupal/Core/Batch/BatchQueueController.php
   @@ -0,0 +1,607 @@
   +class BatchQueueController {
   

   I'm not sure we should call this a Controller, then again I don't think Manager or Service makes it better.

Thank you to @voleger and @borisson_ for reviewing. And thank to @Jo Fitzgerald for the patch.

To address borisson_'s comments.

1. Dictionary.com defines domain as

3. a realm or range of personal knowledge, responsibility, etc.

It is the realm of responsibility that I want to get across with the comment. Also, the class does not only store values, it also stores process (as operations), so 'value object' isn't extensive enough.

2. I've remove the title from the example code.

3. The only use of createWith...() I can find is createWithSampleValues(), all other create...() functions are createFrom...(). Because of this I want to leave it as createFromTitle() for code consistency - unless there are other reasons to change it.

4. It was the comments that I was working from to create the patch. The comments were changed in #2600012: 'css' key in batch_set is no longer used. Apparently, the CSS additions didn't work anyway. #2378095: Convert all remaining attached individual CSS/JS assets to attached asset libraries changed the Batch API to use libraries. So now the CSS setters need to be replaced with library setters.

5. I've rewritten the setQueue() to use the quick return and ...

6. to use === NULL.

7. It's not a controller in the MVC sense, but it does control stuff. It's definitely not a service either. Maybe manager would work? I'm looking for further input before changing this.

@borisson_ pointed out some changes to be made in point 4 of #84.

After a bit of discovery, I found that CSS is no longer used and libraries have been implemented by use in _batch_page(). This needs to be reflected in the patch for this issue.

TODO:
Remove the CSS based functionality from the Batch class and replace it with the equivalent 'library' code. createFromArray() should use addLibrary() for all cases. addLibrary() should be able to accept a single library (string) or multiple libraries (array).

I've changed the Batch class to accept libraries instead of CSS files.

From @borisson_'s suggesting in comment 84 point 4, createFromArray() now uses addLibraries() for all cases.

addLibraries() can now accept strings and arrays of strings to make it more adaptable.

After talking to @alexpott, it has been decided to only include the Batch object in this issue.

There would need to be a process to convert a Batch object into an array for use by the processing functions.

The processing of the batches is to be moved to other issues.

A new patch that is just the Batch class.

Changed to 'needs review'.

1. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +class Batch implements BatchInterface {
   

   Let's call this a BatchBuilder. Since this is a mutable object and we really don't want the Batch objects to be mutable once they are being processed.

2. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +    $this->setTitle('Processing');
   +    $this->setInitMessage('Initializing.');
   +    $this->setProgressMessage('Completed @current of @total.');
   +    $this->setErrorMessage('An error has occurred.');
   

   Why not just default values?

3. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function create() {
   +    return new static();
   +  }
   

   Not necessary. Or maybe create could take some of the common defaults.

4. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function createFromTitle($title) {
   +    return static::create()->setTitle($title);
   +  }
   

   Not necessary.

5. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function createFromArray(array $batch_definition) {
   +    $new_batch = static::create();
   +
   +    if (isset($batch_definition['operations'])) {
   +      foreach ($batch_definition['operations'] as list($callback, $arguments)) {
   +        $new_batch->addOperation($callback, $arguments);
   +      }
   +    }
   +
   +    if (isset($batch_definition['title'])) {
   +      $new_batch->setTitle($batch_definition['title']);
   +    }
   +
   +    if (isset($batch_definition['init_message'])) {
   +      $new_batch->setInitMessage($batch_definition['init_message']);
   +    }
   +
   +    if (isset($batch_definition['progress_message'])) {
   +      $new_batch->setProgressMessage($batch_definition['progress_message']);
   +    }
   +
   +    if (isset($batch_definition['error_message'])) {
   +      $new_batch->setErrorMessage($batch_definition['error_message']);
   +    }
   +
   +    if (isset($batch_definition['finished'])) {
   +      $new_batch->setFinishCallback($batch_definition['finished']);
   +    }
   +
   +    if (isset($batch_definition['file'])) {
   +      $new_batch->setFile($batch_definition['file']);
   +    }
   +
   +    if (isset($batch_definition['library'])) {
   +      $new_batch->addLibraries($batch_definition['library']);
   +    }
   +
   +    if (isset($batch_definition['url_options'])) {
   +      $new_batch->setUrlOptions($batch_definition['url_options']);
   +    }
   +
   +    if (isset($batch_definition['progressive'])) {
   +      $new_batch->setProgressive($batch_definition['progressive']);
   +    }
   +
   +    if (
   +      isset($batch_definition['queue']['name']) &&
   +      isset($batch_definition['queue']['class'])
   +    ) {
   +      $new_batch->setQueue(
   +        $batch_definition['queue']['name'],
   +        $batch_definition['queue']['class']
   +      );
   +    }
   +
   +    return $new_batch;
   +  }
   

   This isn't need now.

6. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setFinishCallback($callback) {
   +    $this->finished = $callback;
   +    return $this;
   +  }
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function addOperation($callback, array $arguments = []) {
   +    $this->operations[] = [$callback, $arguments];
   +    return $this;
   +  }
   

   Let's typehint with Callable

7. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +    if (!is_array($libraries)) {
   +      $libraries = [$libraries];
   +    }
   

   This is not necessary can just cast to an array. So

   $this->libraries = array_merge($this->libraries, (array) $libraries);
   </li>
   
   <li>
   <code>
   +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +    $this->libraries = $this->libraries + $libraries;
   

   Are we sure about + here? don't we mean array_merge()?

8. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function enqueue() {
   +    batch_set($this->toArray());
   +    return $this;
   +  }
   

   enqueue is a weird name. How about addToQueue()? Or just queue()?

9. +++ b/core/lib/Drupal/Core/Batch/Batch.php
   @@ -0,0 +1,334 @@
   +      'operations' => $this->operations ?: [],
   

   Is this required? I.e should we error if there are no operations.

To address the points from @alexpott in comment 91:

1. I've renamed the class and interface to BatchBuilder and BatchBuilderInterface.

2. The default values are now set in the property definitions.

3, 4, 5. The __construct(), create(), createFromTitle(), and createFromArray() methods have been removed.

6. setFinishCallback() and addOperation() methods both have callable type hints.

7. addLibraries() now casts to an array and uses array_merge.

8. The queuing method is now called queue().

9. The batch operations could be set like:

foreach ($ids as $id) {
  $batch_builder->addOperation('batch_example_callback', [$id]);
}

The code that gets the list of $ids might return an empty array, so I don't think that the BatchBuilder should set restriction on this (by throwing exceptions) but needs to cope when no operations are set. Maybe logging when there are no operations in the batch would be enough?

There is no interdiff as the files were renamed, so the interdiff would be the old files removed and new files added.

Test still need to be added.

There is no interdiff as the files were renamed, so the interdiff would be the old files removed and new files added.

git diff -M helps with finding renames. It doesn't always work - but in this case, I think it would have.

Thanks @borisson_. It worked!

One interdiff for 89 to 93.

I've created some tests for the BatchBuilder class.

I've removed the queue() function and changed the class's comment to instruct developers to use
batch_set($batch_builder->toArray());

The setQueue() function has been changed so that it checks for the class with the passed in name, throwing an exception if it cannot be found. The comments for the method have been cleaned up.

With regards to @dawehner's comments in comment #99:

1. I was thinking about clearing settings that had been passed in already, but if you really need to do that you can pass in an empty array. I've removed the default option.

2. Swap it out for tests? But then the same question. The builder doesn't do anything that would not be available during unit tests. In any case, I've removed the interface.

1. This is beginning to look really nice and concise.

2. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,324 @@
   + *   ->setTitle('Batch Title')
   

   This should be a translatable string.

3. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,324 @@
   + *   ->setInitMessage('The initialization message (optional)');
   

   This too.

4. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,324 @@
   +  protected $title = 'Processing';
   ...
   +  protected $initMessage = 'Initializing.';
   ...
   +  protected $progressMessage = 'Completed @current of @total.';
   ...
   +  protected $errorMessage = 'An error has occurred.';
   

   Just realised this defaults should all be translatable strings so they are going to need setting in the constructor.

5. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,324 @@
   +   * @param string $title
   +   *   The title.
   ...
   +   * @param string $message
   +   *   The text to display. Defaults to 'Initializing.'.
   ...
   +   * @param string $message
   ...
   +   * @param string $message
   +   *   The text to display. Defaults to 'An error has occurred.'.
   

   These should all accept string or translatablemarkup.

6. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,324 @@
   +   *   The text to display.  Available placeholders are '@current',
   +   *   '@remaining', '@total', '@percentage', '@estimate' and '@elapsed'.
   

   Let's convert this into a proper API docs list.

7. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,324 @@
   +   *   Defaults to 'Completed @current of @total.'.
   

   Normally Defaults to... on a @param doc is about what the param defaults to. That's not the case here so it looks weird.

8. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
   @@ -0,0 +1,246 @@
   +  public function testSensibleDefaultValues() {
   

   testDefaultValues() ... the Sensible is unnecessary.

9. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
   @@ -0,0 +1,246 @@
   +    $this->assertTrue(is_array($batch), 'Batch builder create an array.');
   +    $this->assertTrue(is_array($batch['operations']), 'Operations array exists.');
   ...
   +    $this->assertTrue(is_array($batch['library']), 'Library array exists.');
   ...
   +    $this->assertTrue(is_array($batch['url_options']), 'URL options array exists.');
   

   There's $this->assertInternalType() or $this->assertArrayHasKey().

10. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
    @@ -0,0 +1,246 @@
    +    $this->assertFalse(isset($batch['queue']), 'Queue has not been set.');
    

    $this->assertArrayNotHasKey()

11. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
    @@ -0,0 +1,246 @@
    +    $this->assertEquals('New Title', $batch['title'], 'Title has been set.');
    

    Generally we skip the 'Title has been set.' stuff in PHPUnit cause the assert and test method name is enough.

12. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
    @@ -0,0 +1,246 @@
    +    $this->assertEquals(1, count($batch['library']), 'Library added as string.');
    ...
    +    $this->assertEquals(3, count($batch['library']), 'Libraries added in an array.');
    ...
    +    $this->assertEquals(1, count($batch['library']), 'Libraries replaced by an array.');
    

    Let's assert what is in the array here.

To make messages translatable I think that better to add constructor which sets defaults and allows to accept overrides for this defaults

Example of usage also needs update

+++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
@@ -0,0 +1,324 @@
+ *   ->setTitle('Batch Title')
+ *   ->setFinishCallback('batch_example_finished_callback')
+ *   ->setInitMessage('The initialization message (optional)');
...
+  protected $title = 'Processing';
...
+  protected $initMessage = 'Initializing.';
...
+  protected $progressMessage = 'Completed @current of @total.';
...
+  protected $errorMessage = 'An error has occurred.';
...
+      'title' => $this->title ?: '',
+      'init_message' => $this->initMessage ?: '',
+      'progress_message' => $this->progressMessage ?: '',
+      'error_message' => $this->errorMessage ?: '',

messages should be translatable

I've changed the title and messages to be string or TranslatableMarkup, updating the example code and setters as necessary. A constructor now sets the default values for the translatable properties.

The comments have been shuffled so that the 'default' values are not in the @param docs.

The tests have been updated to check for TranslatableMarkup values, the arrays are fully checked rather than parts of them, and better asserts have been used. The messages have been removed from the asserts.

I think this addresses everything from @alexpott and @andypost's comments.

I've added a build() method to the batch builder.

At the moment this wraps the toArray() method, but in the future it could return an immutable object as the representation of a batch.

1. This is looking really good - feels nearly there.

2. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   + * Stores a domain object for a batch process.
   

   This is not quite right now. I think it is okay if this just says Builds an array for a batch process. - we can update it as the code changes if this eventually builds immutable objects.

3. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +   * If the callbacks are in the .module file or are static methods of a class,
   +   * then this does not need to be set.
   

   or can be autoloaded, for example, static methods on a class, then this does not need to be set.

4. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +   * If true, multiple calls are used. Otherwise an attempt is made to process
   +   * the batch in on page call.
   

   in on page call should be in a single run. We shouldn't assume a "page".

5. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +   *   TRUE (default) indicates that the batch will run in more than one run.
   +   *   FALSE indicates that the batch will finish in a single run.
   

   This should be something like:
   (optional) 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. Defaults to TRUE.

   Also we should file an issue (or check for one to fix the docs in batch_set() which incorrectly say the default is FALSE.

6. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +   * Sets or removes an override for the default queue.
   

   How would this remove an override?

7. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +   *   The fully qualified name of a class that implements
   +   *   \Drupal\Core\Queue\QueueInterface.
   

   We can ensure that the class implements this interface in the method using http://php.net/manual/en/function.class-implements.php

8. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +    if (!class_exists($class)) {
   +      throw new \InvalidArgumentException('Class ' . $class . ' does not exist.');
   +    }
   

   This should be tested.

9. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
   @@ -0,0 +1,357 @@
   +  /**
   +   * Adds libraries to be used on the process page.
   +   *
   +   * @param string|string[] $libraries
   +   *   The libraries to add.
   +   *
   +   * @return $this
   +   */
   +  public function addLibraries($libraries) {
   +    $this->libraries = array_unique(
   +      array_merge($this->libraries, (array) $libraries)
   +    );
   +    return $this;
   +  }
   

   Just pondering is this actually necessary - are there example in core where this would be useful? Off hand I couldn't find a single thing in core that would use this. I can't find anything that'd use setLibraries() but that seems useful.

10. +++ b/core/lib/Drupal/Core/Batch/BatchBuilder.php
    @@ -0,0 +1,357 @@
    +  /**
    +   * Builds a representation of a batch for batch_set().
    +   *
    +   * @return mixed
    +   *   A representation of the batch.
    +   */
    +  public function build() {
    +    return $this->toArray();
    +  }
    

    This feels premature. Let's wait till we do the next change.

11. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
    @@ -0,0 +1,262 @@
    +/**
    + * @file
    + * Contains \Drupal\Tests\system\Unit\Batch\BatchBuilderTest.
    + */
    

    No longer needed.

12. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
    @@ -0,0 +1,262 @@
    +      ->setErrorMessage(new TranslatableMarkup('Ooops. An error has occurred :('))
    ...
    +    $this->assertEquals(new TranslatableMarkup('Ooops. An error has occurred :('), $batch['error_message']);
    

    Ooops is should be spelled Oops :)

13. +++ b/core/modules/system/tests/src/Unit/Batch/BatchBuilderTest.php
    @@ -0,0 +1,262 @@
    +      ->addLibraries(['library2', 'library3'])
    

    Let's also add 'library1' to this so we test the array_unique()

From alexpott's comments in #105

2, 3, 4, 5. The comments have been changed.

6. I've removed the text that says that you can remove a queue class.

7. A check has been put in to ensure that the class passed in implements the correct interface. There's also a test for this.

8. A test for a missing class has been added.

9. From a new instance, both addLibraries() and setLibraries() do the same thing. The difference between the two is:

• setLibraries() allows removing libraries,
• addLibraries() can accept a string or an array.

Maybe removing setLibraries() would be better?
I couldn't find a use case in core either.

10. I was thinking about future issues to change implementations to use toArray() and then changing them again to use build(); adding build() at this stage would remove the need for the second set of issues. I've removed build() for now.

11. Comment has been removed.

12. Spellings fixed.

13. Added 'library1' to the test.

Yep lets remove addLibraries().

Also the change record needs an update - https://www.drupal.org/node/2875389

I've removed the addLibraries() method.

The change record has been updated.

And I've updated the summary of this issue.

Unfortunately #108 lost the test coverage of setLibraries().

Also as core framework manager I sign off on this addition to the API. It makes building a batch way easier and is the first step in modernising the batch code.

Oops. I got carried away with the delete key.

The test has been put back.

Condensed the test for setLibraries().

Addresses @alexpott's feedback from #105 so setting to RTBC.

Temporary testbot hiccup.

Adding review credit for people who left reviews that influenced the direction of the patch.

Committed ac703dc and pushed to 8.6.x. Thanks!

Don't you think it could be part of 8.6.0 highlights ?