document Batch API callbacks as callback implementations

If a Novice contributor wants to work on this... The way I'd suggest to find the Batch API callbacks that need to be marked as implementations would be to look for places in Core that call batch_set(). Which you can do from the batch_set() page on api.drupal.org, and see what functions are being passed in as the callbacks for the operation and for "finished".

And the way to document them is shown at:
https://drupal.org/node/1354#callback-def
Basically, you want to change the documentation block so it just says:

/**
* Implements callback_batch_finished().
*/

(or callback_batch_operation()). And you can have additional paragraphs in the docs if that makes sense, to explain in particular what this callback function does.

@jhodgdon Fixed Batch API callbacks documentation block.

Also:

  * This is a multistep operation: we go through all nodes by packs of 20. The
  * batch processing engine interrupts processing and sends progress feedback
  * after 1 second execution time.
  *
- * @param array $context
- *   An array of contextual key/value information for rebuild batch process.
  */
 function _node_access_rebuild_batch_operation(&$context) {

You need to remove that line with just a * that will be left at the end of the doc block.

Also, you did a nice job of documenting the 'finished' callback for node_mass_update, but the main operations callback of _node_mass_update_batch_process() also needs to be fixed up.

As joachim suggested in #3, I think there are probably additional examples in Core that also need to be documented. Good start though!

@jhodgdon Fixed the comment.

@joachim I searched the D8 core files but was not able to find callbacks which are defined in https://api.drupal.org/api/drupal/core%21includes%21module.inc/group/cal... this is what issue title says, there are other callbacks which we can create separate issues to cover them.

@sandykadam: The point of this issue is that when batch_set() is called, you pass in the names of "operations" and optionally "finished" callback functions. In a separate issue, we documented a callback function template for callback_batch_operations() and callback_batch_finished(), which are listed in https://api.drupal.org/api/drupal/core!includes!module.inc/group/callbac... -- and in this issue, we need to find where batch_set() is being called, figure out which functions are being used as those callback functions, and change their documentation so it matches the standards cited above.

This patch addresses just a couple of places in Core, and there are many more, so it's not done yet. The existing docs look good though -- we just need a bigger patch that does this for more functions. Thanks!

Following the original instructions by searching for "batch_set()", only a half dozen items come up. Changing the search string to be "batch_set(", leaving off the trailing parenthesis, reveals a much larger result set.

Uploading patch from Austin sprint for someone else to review. It's my first patch and would really like some feedback.

The best way to find these is probably to look at the places that call batch_set() and see what they are passing in, not by using grep.

Here is another attempt at a patch to address the concerns mentioned previously.

Setting to "Needs review" so the testbot can do its thing.

Thanks! I have checked this patch carefully, to make sure the functions are correctly marked, and it is mostly right.

A few small things to fix:

a) In most of the patch, you did a great job of adding the new "Implements ... " line and keeping the existing documentation. But:

 /**
- * Batch callback for batch installation of modules.
+ * Implements callback_batch_operation().
  */
 function _install_module_batch($module, $module_name, &$context) {

We lost documentation here on what this function does. Can you add a new line (after a blank line) that will still tell us that this function installs modules in batches?

b)
locale_translate_batch_import() -- I don't think we should mark this as a batch callback only, because it is also directly called in code. So let's just leave this one as it is.

c) typo:

 /**
+ * Implements callback_batch_finisherd().
+ * 
  * Finished callback of system page locale import batch.
  */
 function locale_translate_batch_finished($success, $results) {

finisherd -> finished
But... This function also has a direct call, so I think we should not document it as a batch callback. So again, let's leave it as it is.

d) locale_config_batch_refresh_name():

 /**
- * Performs configuration translation refresh as a batch step.
+ * Implements callback_batch_operation().
+ * 
+ * Performs configuration translation.

Last line here is not quite right. It performs configuration translation *refresh*. It doesn't do translation.

e) See (b) and (c) - also applies to locale_config_batch_finished().

f)

+++ b/core/modules/node/node.module
@@ -1570,7 +1570,7 @@
 }
 
 /**
- * Performs batch operation for node_access_rebuild().
+ * Implements callback_batch_operation().
  *
  * This is a multistep operation: we go through all nodes by packs of 20. The
  * batch processing engine interrupts processing and sends progress feedback
@@ -1612,7 +1612,7 @@
 }
 
 /**
- * Performs post-processing for node_access_rebuild().
+ * Implements callback_batch_finished().
  *
  * @param bool $success
  *   A boolean indicating whether the re-build process has completed.

In both of these, we lost the information that it is being called from node_access_rebuild().

g) The two changes in form.api.php -- those were where the callbacks were defined! We do not want them to be changed.

h) Lost information here:

 /**
- * Performs a batch operation setting up its own batch.
+ * Imlements callback_batch_operation().
  */
 function _batch_test_nested_batch_callback() {

i)

 /**
- * Tests the progress page theme by performing a batch callback.
+ * Implements callback_batch_operation().
+ * 
+ * Tests the progress page theme
  */
 function _batch_test_theme_callback() {

Documentation line at end needs to end in ".".

j) And in user.module, again lost information:

 /**
- * Finished batch processing callback for cancelling a user account.
+ * Implements callback_batch_finished().
  *
  * @see user_cancel()
  */

OK, I can live with that. :)

We still need to make sure we don't lose documentation when we convert the docs headers of these functions.

Oh. One thing I thought of last night and forgot to add here:

When you are changing a function docs header to now say "Implements callback_batch_finished()." as its first line (or the other callback), you can take out the @param and @return documentation below, because the idea is that the callback documents this.

However, if the @param/@return has something in it that is specific to this particular callback (as opposed to the generic batch callback param/return docs), you can leave it in.

Good question. I personally think having an incomplete list of @param statements is a bit odd, so I think I would put it in. But could go either way.

I think that if we are at all listing @param, then we should document all the parameters, even if we are saying the same thing as the callback documentation. Even if the parsers support mixing the parameters, I think it is still useful to have it in the source code for someone who is going to refer the files themselves.

I found some other places where this can be documented. I will look over all instances and work on the patch tomorrow. Assigning this issue to myself.

I rerolled the patch for latest HEAD and added most of the edits from #16. I omitted any edits that would remove information (callbacks that also have direct calls and callbacks that have documented @params).

 /**
- * Performs batch operation for node_access_rebuild().
+ * Implements callback_batch_operation().
  *
  * This is a multistep operation: we go through all nodes by packs of 20. The
  * batch processing engine interrupts processing and sends progress feedback
@@ -1612,7 +1612,7 @@
}

 /**
- * Performs post-processing for node_access_rebuild().
+ * Implements callback_batch_finished().
  *
  * @param bool $success
  *   A boolean indicating whether the re-build process has completed.
  */

 /**
- * Finished batch processing callback for cancelling a user account.
+ * Implements callback_batch_finished().
  *
  * @see user_cancel()
  */

Still losing information in these 3 places.

I don't see the harm in adding these lines back in. It makes the comment a bit clearer.

I also cleaned up a few whitespace problems.

Looking pretty close, thanks!

A few more things still to fix:

a) We've still lost some information:

+++ b/core/modules/node/node.admin.inc
@@ -95,7 +95,7 @@ function _node_mass_update_helper(NodeInterface $node, array $updates, $langcode
 }
 
 /**
- * Executes a batch operation for node_mass_update().
+ * Implements callback_batch_operation().
  *
  * @param array $nodes
  *   An array of node IDs.
@@ -143,7 +143,7 @@ function _node_mass_update_batch_process(array $nodes, array $updates, $load, $r
 }
 
 /**
- * Reports the 'finished' status of batch operation for node_mass_update().
+ * Implements callback_batch_finished().
  *
  * @param bool $success
  *   A boolean indicating whether the batch mass update operation successfully

b) Missing . at end of comment:

+++ b/core/modules/locale/locale.bulk.inc
@@ -165,7 +165,9 @@ function locale_translate_batch_build(array $files, array $options) {
 }
 
 /**
- * Perform interface translation import as a batch step.
+ * Implements callback_batch_operation().
+ *
+ * Perform interface translation import

c) Lost information:

+++ b/core/modules/update/update.authorize.inc

 /**
- * Batch callback: Performs actions when the authorized install batch is done.
+ * Implements callback_batch_finished().
  *
  * This processes the results and stashes them into SESSION such that
  * authorize.php will render a report. Also responsible for putting the site

Lost information that it's for the "authorized install batch".

The rest looks good to me! Almost there!

Thanks for the quick review. I applied the changes from #27.

Great, looks good now!

Much clearer now. Great work!

Committed and pushed to 8.0.x. Thanks!

Marking for backport.

hi - trying my hand at backporting to D7

I wasn't sure about how to include in the documentation block in conditional cases where batch_set() is called.

thanks!

whoops - sorry - meant to attach to #32

Thanks for the patch!

This mostly looks good. The only problem is the placement of the "Implements callback_batch_..." lines. They need to be either at the top of the documentation block, or near the top. Not in the middle of @param sections. Thanks!

Also, in: modules/simpletest/tests/batch_test.callbacks.inc

 /**
+ * Implements callback_batch_finished().
+ *
  * 'finished' callback for batch 0.
  */
 function _batch_test_finished_0($success, $results, $operations) {

In this one, I think we can get rid of the other line that was there; same for the next few changes.

And in

 +++ b/modules/simpletest/tests/batch_test.module
@@ -116,6 +116,8 @@ function batch_test_simple_form() {
 }
 
 /**
+ * Implements callback_batch_operation().
+ *
  * Submit handler for the simple form.
  */
 function batch_test_simple_form_submit($form, &$form_state) {

I do not believe this is actually a batch callback -- it looks like a form submit handler? Check on the others in that file too -- none of them look like batch callbacks to me.

thanks for the lightening fast review - most of the submit handlers in batch_test.module are calling batch_set() - so still pull them out?

Yeah... Just to reword what joachim says...

So. When you call batch_set(), you tell the Drupal batch system what how to process your batch. In that call, you pass in (among other things), the names of some callback functions, the "process" callback and the "finished" callback.

So what we want to happen in this patch is that any functions that are passed into batch_set() as "process" or "finished" callbacks get those lines saying "Implementation of ... " in them.

Functions that *call* batch_set() should not be changed.

Is that clearer?

Thanks for the feedback - incorporated #35

When you upload a patch, set the issue status to "Needs review". Also, if it's a new patch that updates a previous patch, make an interdiff file. Thanks! (Don't bother for this one though, I've already reviewed the patch.)

So... here are some suggestions:

a)

+++ b/modules/locale/locale.admin.inc
@@ -344,6 +344,8 @@ function locale_languages_predefined_form_validate($form, &$form_state) {
 
 /**
  * Process the language addition form submission.
+ *
+ * Implements callback_batch_operation() on language file import.
  */
 function locale_languages_predefined_form_submit($form, &$form_state) {

Again, this is a form submit handler. It is NOT a batch operation callback. Please remove that line of added documentation.

b)

+++ b/modules/locale/locale.module
@@ -879,6 +879,9 @@ function locale_themes_enabled($themes) {
  * right away, or start a batch if more files need to be imported.
  *
  * @param $components
+ *
+ *   Implements callback_batch_operation().
+ *
  *   An array of component (theme and/or module) names to import
  *   translations for.
  */

You've inserted three lines between an @param and the description of that parameter. Do not insert it there. Also I do not think this is actually an operations callback at all.

c)

+++ b/modules/simpletest/simpletest.module
+ * @return $test_id
  */
 function simpletest_run_tests($test_list, $reporter = 'drupal') {

Why are you adding this line? It isn't correct (that isn't how we do @return), and this patch is not about doing this type of thing anyway.

d)

 +++ b/modules/simpletest/tests/batch_test.callbacks.inc

 /**
- * 'finished' callback for batch 0.
+ * Implements callback_batch_finished().
  */
 function _batch_test_finished_0($success, $results, $operations) {

I guess I was wrong about these. We are actually losing information that it is for "batch 0". So probably we just need to add the new line but not lose the existing lines in this file.

e)

++ b/modules/simpletest/tests/batch_test.module
@@ -471,6 +471,8 @@ function _batch_test_batch_5() {
 }
 
 /**
+ * Implements callback_batch_operation().
+ *
  * Menu callback: run a batch for testing theme used on the progress page.
  */
 function batch_test_theme_batch() {

Again I do not think this is actually a batch callback. I think it's a function that calls batch_set(). Same with this one:

+++ b/modules/update/update.fetch.inc
@@ -6,6 +6,8 @@
  */
 
 /**
+ * Implements callback_batch_operation().
+ *
  * Page callback: Checks for updates and displays the update status report.
  *
  * Manually checks the update status without the use of cron.

I haven't checked everything in this patch... some of the others may not be callbacks either. Please check them again.

Oh here's another one:

+++ b/modules/update/update.manager.inc
@@ -301,6 +301,8 @@ function update_manager_update_form_validate($form, &$form_state) {
 }
 
 /**
+ * Implements callback_batch_operation().
+ *
  * Form submission handler for update_manager_update_form().

I rerolled the patch for HEAD and cleaned up the comments. I addressed the issues in #40, plus the following changes:

includes/install.core.inc:
install_run_task() is not a batch operation callback - removed

includes/locale.inc:
_locale_batch_language_finished() implements callback_batch_finished() - added
_locale_batch_system_finished() implements callback_batch_finished() - added
_locale_batch_import() implements callback_batch_operation() - added

includes/update.inc:
DrupalUpdateException::update_batch() is not a batch operation callback
update_finished implements callback_batch_finished() - added

modules/node/node.admin.inc:
node_mass_update() sets up the batch but is not a callback - removed
_node_mass_update_batch_finished() is not a menu callback, but it is a callback_batch_finished() - fixed

modules/node/node.module:
node_access_rebuild() sets up the batch but is not a callback - removed

modules/simpletest/simpletest.module:
simpletest_run_tests() sets up the batch but is not a callback - removed

modules/simpletest/tests/batch_test.callbacks.inc:
added descriptive comments for batches 0-5

modules/update/update.authorize.inc:
update_authorize_run_update() sets up the batch but is not a callback - removed
update_authorize_run_install() sets up the batch but is not a callback - removed
update_authorize_update_batch_finished() implements callback_batch_finished() - added

modules/update/update.fetch.inc:
update_fetch_data_batch() implements callback_batch_operation() - added
update_fetch_data_finished() implements callback_batch_finished() - added

modules/update/update.manager.inc:
update_manager_download_batch_finished() implements callback_batch_finished() - added

Thanks for the patch! This mostly looks good... A few things to fix:

1. --- a/includes/install.core.inc
   +++ b/includes/install.core.inc
   

   It looks like
   _install_profile_modules_finished() in this file is the "finished" callback for this batch, so that should be added to the patch?

2. +++ b/includes/update.inc
   @@ -1008,6 +1010,8 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   + * Implements callback_batch_operation().
   + *
     * Starts the database update batch process.
     *
   

   This is the doc block for the update_batch() function. This is NOT a batch operations callback -- it is the function that actually calls batch_set(). So, remove this change.

   The other two fixes in update.inc look good though.

3. +++ b/modules/node/node.module
   @@ -3596,6 +3596,8 @@ function node_access_needs_rebuild($rebuild = NULL) {
   + * Implements callback_batch_operation().
   + *
     * Rebuilds the node access database.
     *
   

   This is the docblock for node_access_rebuild(). It's a function that sets up a batch, not a batch operation callback, so remove this change.

4. +++ b/modules/simpletest/tests/batch_test.callbacks.inc
   @@ -127,6 +145,7 @@ function _batch_test_finished_3($success, $results, $operations) {
   + * Implements callback_batch_finished().
     * 'finished' callback for batch 4.
     */
    function _batch_test_finished_4($success, $results, $operations) {
   @@ -134,6 +153,8 @@ function _batch_test_finished_4($success, $results, $operations) {
   

   Needs blank line between 1st and 2nd line.

Thanks @jhodgdon,

I addressed the issues identified in #42. Should be close now :)

Thanks for the new patch!

Looking at the interdiff, I think we still wanted to have the finished callback docs in update.inc from the previous patch. Can you put that back? Everything else in the interdiff looks right. Thanks!

Thanks for the review. I took a closer look at update_finished and it definitely is a finished callback. I added back the callback reference to the docs for that function.

Thanks everyone! Looks good now.

Committed to 7.x - thanks!