The sample batch finished callback function should have the fourth parameter

Thanks for the patch @hgoto.

There are a couple of points.

The calls to _batch_test_finished_helper() have been changed to add the $elapsed parameter, but the function specification hasn't been updated to receive it (in core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc line 90).

Also callback_batch_finished() (in core/lib/Drupal/Core/Form/form.api.php) should be updated to include the $elapsed parameter and the code comment updated.

The patch in #6 look great. Both points from #4 have been addressed.

It just needs a re-roll and then I think it will be ready for RTBC.

reroll + fix

The re-rolled patch applied cleanly and look good.

A drupal_set_message() was changed to \Drupal::messenger()->addMessage() but that was just changing deprecated code.

I'm happy with this so changing to RTBC.

1. +++ b/core/includes/form.inc
   @@ -648,7 +648,8 @@ function template_preprocess_form_element_label(&$variables) {
   + *   // See _batch_finished() for more information about these paramaters.
   

   I've read the docs of _batch_finished() they don;t seem to be helpful. How about pointing to callback_batch_finished() as those are helpful.

2. +++ b/core/lib/Drupal/Core/Form/form.api.php
   @@ -109,12 +109,15 @@ function callback_batch_operation($multiple_params, &$context) {
   + * @param $elapsed
   + *   A string which represents the elapsed time for the batch process.
     */
   

   This could do with an example - eg. 1 min 30 sec. Also this should be @param string $elapsed

3. +++ b/core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
   @@ -264,30 +264,31 @@ public function _resultStack($id, $value = 0) {
   +    $pattern_elapsed = ' \((\d+ min )?\d+ sec\)';
   

   code do with a comment explaining the looseness of the regex is to cope with variable amounts of elapsed time.

We will work in ContributionWeekend2019

Here is the patch.

@dhirendra.mishra, thank you for the patch.

In future can you add an interdiff when you create a new patch - I helps with reviewing the patch. I've added an interdiff for 9 to 14.

Following on from @alexpott's comments in #12, there are still a few changes needed.

1. +++ b/core/includes/form.inc
   @@ -648,7 +648,8 @@ function template_preprocess_form_element_label(&$variables) {
   + *   // See callback_batch_finished() for more information about these paramaters.
   

   There is a spelling mistake in "parameters".

   The comment now goes beyond 80 characters - although it wouldn't if the code was pasted as a function. I'm not not sure of the standards for this maybe @alexpott can advise?

2. +++ b/core/lib/Drupal/Core/Form/form.api.php
   @@ -109,12 +109,15 @@ function callback_batch_operation($multiple_params, &$context) {
   + * @param string $elapsed
   + *   A string which represents the elapsed time for the batch process.
   

   Examples are still missing as requested by @alexpott in #12 point 2.

3. +++ b/core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
   @@ -264,30 +264,32 @@ public function _resultStack($id, $value = 0) {
   +    // eg. 1 min 30 sec
   +    $pattern_elapsed = ' \((\d+ min )?\d+ sec\)';
   

   The comment should be an explanation of what the regex does. Something like The elapsed time should be either in minutes and seconds or seconds alone.

   Could the elapsed time be in multiple minutes (like "2 mins 10 secs")? Do the parts get pluralised (adding 's')? Would the regex match in these cases?

i will work on your comment. Thanks @John Cook

Here is the interdiff and updated patch as per #15 comment.

Thanks for the new patch and interdiff, @ dhirendra.mishra

All the points from #15 have been addressed.

But applying the patch gives the following error:

../drupal-add_fourth_parameter_to_batch_finished_callback_sample-2939645-17.patch:11: trailing whitespace.
 *   // See callback_batch_finished() for more information about these 
warning: 1 line adds whitespace errors.

This seems to be a coding standards error.

The next step is to check the coding standards error. These can be viewed on the test results page (click the green/gray/red element near the patch file) and there will be a coding standards messages link.

Fixed trailing space issue.

Thank you for the patch @govind.maloo.

The only change between patches #17 and #19 is the removal of white space. and patch #19 applies cleanly.

As the issues pointed out in previous comments are also fixed, I'm setting this issue to RTBC.

This review point from @alexpott was not covered AFAIS, however it looks important to avoid random fails.

Could the elapsed time be in multiple minutes (like "2 mins 10 secs")? Do the parts get pluralised (adding 's')? Would the regex match in these cases?

Reroll and modify the regex pattern mentioned in #21.

+++ b/core/lib/Drupal/Core/Form/form.api.php
@@ -109,13 +109,17 @@ function callback_batch_operation($multiple_params, &$context) {
+ *   A string which represents the elapsed time for the batch process, eg.
+ *   e.g. 1 min 30 sec.

There are two versions of e.g. instead of one.

1. +++ b/core/includes/form.inc
   @@ -650,7 +650,9 @@ function template_preprocess_form_element_label(&$variables) {
   + *   // See callback_batch_finished() for more information about these
   

   #12.1 addressed

2. +++ b/core/lib/Drupal/Core/Form/form.api.php
   @@ -109,13 +109,17 @@ function callback_batch_operation($multiple_params, &$context) {
   + *   A string representing the elapsed time for the batch process, e.g., 1 min
   + *   30 sec.
   

   #12.2 addressed. BUT, I think it should be 30 secs.

   Suggestion:

   - *   A string representing the elapsed time for the batch process, e.g., 1 min
   - *   30 sec.
   + *   A string representing the elapsed time for the batch process, e.g.,
   + *   '1 min 30 secs'.
   

   Do not separate 1 min and 30 sec(s) due to over 80 chars. Instead, treat them together as ONE, surrounding with single quotes, stays at the 2nd line. Neither over chars, nor wrapped early.

3. +++ b/core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
   @@ -313,38 +313,40 @@ public function _resultStack($id, $value = 0) {
   +    // The elapsed time should be either in minutes and seconds or only seconds.
   +    $pattern_elapsed = ' \((\d+ mins? )?\d+ secs?\)';
   

   Addressed #12.3, and the reg looks good.

4. Tagging "Bug Smash Initiative"

In all, this is RTBC to me.

Committed and pushed 7ca98bac22 to 9.1.x and 28c8fbe32e to 9.0.x. Thanks!