Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As pointed on #652200: standard to document form api callbacks, now we have a standard way to document(doxygen block of the callback function) form callbacks.
So, we need to verify that on places like:
/**
* Helper function for use as a form submit callback.
*/
function update_cache_clear_submit($form, &$form_state) {
// Clear all update module caches.
_update_cache_clear();
}
Comment | File | Size | Author |
---|---|---|---|
#18 | block.admin_.inc_.18.patch | 4.62 KB | trevjs |
#15 | block.admin_.inc_.15.patch | 4.61 KB | trevjs |
#11 | block.admin_.inc_.form_ds_10.patch | 4.41 KB | trevjs |
#10 | block.admin_.inc_.form_ds_10.patch | 4.4 KB | trevjs |
#5 | form_callbacks5_block-admin.patch | 563 bytes | trevjs |
Comments
Comment #1
jhodgdonThis seems like a good project for a novice doc contributor to look into
Comment #2
ldpm CreditAttribution: ldpm commentedComment #3
ldpm CreditAttribution: ldpm commentedI 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.
Comment #4
ldpm CreditAttribution: ldpm commentedComment #5
trevjs CreditAttribution: trevjs commentedLet'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.
Comment #6
jhodgdon- 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().
Comment #7
jhodgdonAlso, if you could at the same time bring the validation handler for this same form up to standards, that would be good. Thanks!
Comment #8
trevjs CreditAttribution: trevjs commentedWhich 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
Comment #9
jhodgdonSorry, 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.
Comment #10
trevjs CreditAttribution: trevjs commentedI 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.
Comment #11
trevjs CreditAttribution: trevjs commentedPlease ignore the last patch. I had to make a fix, one of the functions in an @see was incorrect.
Comment #12
jhodgdonLooks pretty good... Just a couple of comments/questions that need to be addressed:
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. ???
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.
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
Comment #13
trevjs CreditAttribution: trevjs commentedThanks 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.
Comment #14
jhodgdonRegarding (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.
Comment #15
trevjs CreditAttribution: trevjs commentedIn 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.
Comment #16
jhodgdonAll 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.
Comment #17
aspilicious CreditAttribution: aspilicious commentedLine lengths are ok ;)
Comment #18
trevjs CreditAttribution: trevjs commentedComment #19
jhodgdonThanks, this looks good!
Comment #20
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #21
trevjs CreditAttribution: trevjs commentedI believe there are other files that need this.
Comment #22
jhodgdonYes 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.
Comment #23
ohnobinki CreditAttribution: ohnobinki commented+
Comment #24
jhodgdonno need to postpone now
Comment #25
NancyDruWhile 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.