Use standard to document form callbacks

This seems like a good project for a novice doc contributor to look into

I don't think it is really possible to judge whether the documentation for any given function is complete without having a good understanding of how the function works. I'm removing the "Novice" tag.

Let's make sure I have the right idea here before I go to far with this. Taking just the first form passed to drupal_get_form according to the api pages.

I wasn't sure how much I needed to say about the parameters. Let me know if there is anything more I can do there.

- The params need to be @param $nameofparam, not @nameofparam.

- The standard wasn't followed for the first line:
http://drupal.org/node/1354#forms
It should say "Form builder for ...".

- I would also be a bit more verbose in the params, such as "Array containing all blocks returned by modules in hook_block_info().

Also, if you could at the same time bring the validation handler for this same form up to standards, that would be good. Thanks!

Which function is #7 referring too? Do you mean block_admin_display_form_submit? I don't see a form_validate for it, but I might be blind.

There are the functions for block_admin_configure which I'm pretty sure need the same treatment. Both configure_submit, and configure_validate do not have doc blocks. Perhaps these are what you were referring to? Here is a link to the file's page in the api: http://api.drupal.org/api/drupal/modules--block--block.admin.inc/7

Sorry, yes, I meant the submit and not validate handler in #7. Fingers faster than brain...

And I'm sure there are more functions in Drupal that need doc. Feel free to do as many as you want. :) For a single patch, though, I think a minimum would be to bring a form builder function and its associated (if any) submit and validate functions up to standards.

I went through and did everything in the file. Please make sure I didn't take it too far. I did it for everything that generated a form, even the block delete confirmation. If this wasn't correct I'll fix it, but at least then I'll have a better idea of what this issue covers.

Please ignore the last patch. I had to make a fix, one of the functions in an @see was incorrect.

Looks pretty good... Just a couple of comments/questions that need to be addressed:

+ * Form builder for the block configuration form. Also used for adding a block.
+ *
+ * @param $module
+ *   Name of the module that implements the block to be configured.
+ * @param $delta
+ *   Unique ID of the block within the context of $module. Pass NULL to return
+ *   an empty $block object for $module.

a) The "Also used for adding a block."... should either be left out or put on a different line.
b) Is that correct that you can pass NULL to return an empty $block object for $module? If so... well I have no idea what that statement really means, so maybe it can be explained better? I though $delta would be required, because modules use that to determine which type of block is being requested. ???

+ * Form builder for the custom block addition form.

+ * Form validation handler for the add block form.

+ * Form submission handler for the add block form. Saves the new custom block.

c) I would like to see all of these use the same terminology - either "block addition form" or "add block form".
d) Again, the "Saves the..." should be on its own line.

+ * Form builder for the custom block deletion form.
+ *
+ * @param $module
+ *   A string referencing the module that implements the block to be deleted.
+ *   For user created blocks this should be 'block'.
+ * @param $delta
+ *   The unique ID of the block within the context of $module.

+ * Form submission handler for deletion of custom blocks.

e) Again, parallel terminology
f) "A string referencing"... How about just "The name of the module that...", like you did in the block config form?
g) Actually, $module just has to be 'block'. You can't delete a block that is not user-created. It's a bug (that I'm just going to file) that the function here doesn't verify that the module is block, because it assumes it. See
#851628: Block deletion form making assumption it doesn't check

Thanks for the thorough going over. In regards to 'b,' I borrowed the language from block_load: http://api.drupal.org/api/function/block_load/7. It looks to me that block_load will return an empty block even if delta is set, but the query to load the block results in null. So if the block doesn't exist in the db, the function still returns an empty block.

As for 'a,' I just wanted to note that the configuration form for a new block used the same function as for configuring an existing block. I'll make this clearer with the next submission.

Regarding (b) in #12, the form builder function doesn't return the empty block object. It displays a form for editing the empty block object (for example, when you are creating a new block, I would guess). So maybe:

Unique ID of the block within the context of $module, if configuring an existing block. NULL indicates creating a new block, if $module is 'block'.

Or something like that. I'm pretty sure NULL wouldn't work except if $module is 'block'.

Regarding (a), it's fine to include that information, but please make it a separate paragraph rather than putting a second sentence on what is supposed to be a one-line, one-sentence summary line in the first line of any documentation block.

In addition to your other comments, I've removed the bits about passing null. My thinking here is that it is documented in block_load, and that putting it here is just distracting. Instead I just made a reference to block_add_block_form from block_admin_configure, so that people understand that those two forms are tied together.

For block_custom_block_delete I tried to describe the functionality that you are trying to add in #851628: Block deletion form making assumption it doesn't check. I can re-roll this if there is a conflict, or a change in how you end up doing that, but from what I understand of the other issue, it still won't delete if you try passing this a block from any module other than 'block' because of the way block_custom_block_get works.

All function names, such as hook_block_info() at the top of this patch file, need to have () after them, so that they will be recognized as functions and turned into links on api.drupal.org.

Other than that, this patch looks good. I didn't check line lengths though.

Line lengths are ok ;)

Thanks, this looks good!

Committed to CVS HEAD. Thanks.

I believe there are other files that need this.

Yes there are. But right now webchick and Dries are trying mostly to focus on code bugs, and they would prefer that we postpone wholesale documentation standards updates like this until later if possible (this was requested on another issue). So I am going to mark this "postponed" for a few weeks. We can revisit later.

+

no need to postpone now

While you're at it, please document that $delta is a STRING, not an integer, as most people seem to think. I much prefer to give my blocks a name rather than a number; it sort of becomes self-documenting.