Improve doc and examples for hook_block_info() and friends

I'm not sure that I did this correctly, since it's my first try at api documentation, but here's a patch using user_block_info as the example.

Say it ain't so - I think this was my first ever API docs contribution! Well, yeah, it is kind of weird, though, and probably should go away :)

However:

1. I think we should change the other hooks as well to not use 'amazing' or 'exciting' either - it is good if the example is consistent between them:
   http://api.drupal.org/api/function/hook_block_configure/7
   http://api.drupal.org/api/function/hook_block_save/7
   http://api.drupal.org/api/function/hook_block_view/7
2. I don't think we should lose the 'cache' => DRUPAL_CACHE_PER_ROLE | DRUPAL_CACHE_PER_PAGE example, since it's pretty unintuitive how to do that unless you are familiar with bitmasks. And actually, it's not 100% clear at first glance what that combination even means - I think the deal is supposed to be that the block content can change per role and (even for users with the same role) can also change per page, but it could probably use a good code comment there too...

One thing to consider is that we have a fully-worked-out blocks example module, so the example functions in hook_block_* don't have to do illustrate every possible case:
http://api.drupal.org/api/drupal/developer--examples--block_example--blo...
That would probably be the place to suggest we add examples for caching.

None of our other hook sample functions illustrate every possible case... I think we should just choose the search block, user login block, or some other block in core to use as an example, and put that in for all the hook_block_*, since that's what we've done with other hook example functions.

Should have a patch for this in the next day or so.

Following jhodgdon's guidance, I took the examples from an existing Drupal module (specifically, node module). After reviewing a number of examples I believe the optimal solution is borrowing the examples and language from the example module. However, that would be a wholesale change and somewhat outside the scope of this issue (which was changing "exciting" and "amazing").

I did identify some language still referencing hook_block(), which I replaced.

The one thing that bothers me is the "weight, status, region, visibility, pages" reference in the hook_block_info doc header. These are items you rarely define in practice and if the caching examples are out these should be should be moved to the example module, as well. I kept them, but added a note that they are rarely set from the module.

Since no one commented on "weight, status, region, visibility, pages" being grouped together in a single bullet point instead of being split out per usually parameter specification, I left them grouped. If we keep them, I think each item should be split into its own bullet.

I think the best thing to do with weight etc. is to make them each separate bullet points, and clearly label them as (optional). e.g.:
- 'weight': (optional) Initial value for the ordering weight of this block. Usually not set by the module.

These definitely do need to be documented in hook_block_info(), but I don't think they need to be illustrated in the example.

Other than that change, I think this patch is great. Oh... You might put a code comment in each example function saying something like:

// This example comes from node.module.

Per #7 feedback:
- Added "// This example comes from node.module." as appropriate
- All parameters in the hook_block_api() doc header have their own bullet
- weight and friends are labeled optional
- weight and friends have the line: "Usually not set by the module. Initial value can be later modified by the block admin menu."
I wanted to make it clear specifying the initial value didn't "lock" the value from being changed later.
- Since we've told people how pre-enable a block I also updated the last paragraph that stated: "After completing your blocks, do not forget to enable them in the block admin menu."

Thanks -- looks much better, almost there. A few small things that should be improved:

a) I think we should have @see lines on each of these 4 hooks pointing to each other.

b) In the weight and friends section, all the lines should still wrap close to 80 characters. For instance:

+ *   - 'region': (optional) Initial value for theme region within which this
+ *     block is set.
+ *     Note: If you set a region that isn't available in a given theme, the

Note: should be moved up to the previous line.

c) In the weight and friends section, you have written for each one: "Usually not set by the module. Initial value can be later modified by the block admin menu.". This seems a bit off to me... How about instead:
"Most modules do not provide an initial value, and any value provided can be modified by a user on the block configuration screen."
Definitely "block admin menu" is innacurate.

d) Needs . at end of this:

+ *   - 'status': (optional) Initial value for block enabled status. (1 =
+ *     enabled, 0 = disabled)

e) I'm not sure what this refers to:
first item in the _regions array

_regions array where? Is this the regions array in the theme.info file?

f) (i.e., include, exclude or PHP) -- needs a comma before the "or" to conform to our style standards. See http://drupal.org/node/338208#english]

g) I think since you've given this information in the weight-and-friends section, this bit can be omitted:

+ *
+ * By default blocks are not enabled or set to display in a theme region.
+ * This is usually done in the block admin menu but can also be pre-configured
+ * in hook_block_info() or configured programatically via a module or script.
+ * 

If you do think it's necessary, again it shouldn't say "block admin menu".

Regarding e):
That was part of the original text, so I'm not sure. You don't need to specify regions in the theme .info file if you're using the Drupal standard regions, so that's not it. I suspect it's in the theme engine somewhere. I just ran a quick test, and if the value doesn't match a region the block is disabled. There is a different behaviour if a theme changes and the old region doesn't exist in the new theme. In this case the block was assigned to first region on the region list. I've updated the text to say the block is disabled.

All other changes made as noted.

Excellent work! Let's get this in.

This looks good. jhodgdon is a Documentation Reviewing Machine. :-)

When proofreading/reviewing the proposed changes, it occurred to me that we mention $delta without explaining it. We don't use $delta in the examples either, I think. hook_block_info() sounds like it would be the appropriate place to explain $delta. I'm happy to postpone $delta documentation to another patch, but I'd love to get your thoughts on that before committing this patch.

Good catch - let's get it in here. I think delta is important enough to warrant mentioning in the first full paragraph of the docblock (or in its own paragraph right after that). How about adding something like this:

Each block your module provides must be given a value of delta in hook_block_info(), which is passed into the other block hooks as an argument to identify the block being configured or viewed. The values of delta can be strings or numbers, and they only need to be unique within your module. Typically you would use a string identifier for a simple block that your module provides, and a numeric identifier if you allow users to create several similar blocks that you identify within your module code with numeric IDs.

??

And by the way, I am happy to be a Doc Reviewing Machine -- that means that many other people are stepping up and writing doc patches! Hooray! :)

We currently do define what $delta is, in three places:
http://api.drupal.org/api/function/hook_block_view/7
http://api.drupal.org/api/function/hook_block_configure/7
http://api.drupal.org/api/function/hook_block_save/7

$delta Which block to save the settings for. This is a descriptive string used to identify blocks within each module and also within the theme system. The $delta for each block is defined within the array that your module returns when the hook_block_info() implementation is called.

However, I agree it is a good idea to define it in the hook_block_info() docs instead of (or in addition?) to the other places. I mostly like @jhodgdon's suggested text, but I think we also ought to continue to mention the connection to the theme system and strongly encourage people to use (meaningful!) strings rather than integers here, whenever it is possible for them to do so.

Agreed. There isn't actually much information about what "within the theme system" means.

Hmmm...

Taking a look at the code, I see delta being used in hook_block_view_MODULE_DELTA_alter(), and it looks like we should have an @see from hook_block_view to both hook_block_view_alter() and this hook.

And then I see it coming into block.tpl.php as a theming variable, and as a template suggestion (i.e. you can use block-MODULE-DELTA.tpl.php instead of block.tpl.php).

I think we should mention these specific uses instead of being vague?

I'll make a new patch...

Sounds good to me. From a theming perspective I think the most important thing is that the delta is used to construct an ID. For example, the HTML for the user login block looks like this:

<div id="block-user-login" ....>

where "login" is the block's delta. That was one of the main motivations for using meaningful string deltas in D7 ('block-user-1' leads to CSS that is a lot more annoying to write and read).

OK, how's this? I took a look through the rest of the block hook docs and made a few adjustments...

Looks pretty good - my main comment would be here:

+ * identify the block being configured or viewed. The values of delta can be
+ * strings or numbers; preferably meaningful strings, since string deltas lead
+ * to self-documenting code. Meaningful string deltas are also good for theming:
+ * delta is a variable in block.tpl.php, it's used to define the default CSS ID
+ * applied to the block when rendered, and it's used to define a template
+ * suggestion of block__MODULE__DELTA. Delta can also be used by other modules
+ * to identify your module's blocks, in hooks such as
+ * hook_MODULE_DELTA_alter(). So use a string if possible, and only use a
+ * numeric identifier if you have to (for instance if your module allows users
+ * to create several similar blocks that you identify within your module code
+ * with numeric IDs).

First, the hook name near the bottom is wrong; I assume hook_block_view_MODULE_DELTA_alter() is the one you want here :)

Second, after reading this, given that hook_block_info() is a very common hook that new developers might look at a lot as a starting point, I wonder if this assumes too much technical knowledge? Maybe rewording it so the technical level is somewhere in between the previous level and this one would be a good compromise, e.g something like this:

The values of delta can be strings or numbers, but you should use meaningful strings whenever possible, since this leads to self-documenting code. This is especially useful for theming: a default HTML ID of "block-[MODULE]-[DELTA]" is applied to each block when it is rendered, so a meaningful string makes it easy to target the block via CSS. Delta is also used to define a template suggestion of block__MODULE__DELTA (for more advanced theming possibilities) and by other modules when implementing hooks such as hook_block_view_MODULE_DELTA_alter().

****

Other less important comments:

  *   - 'info': (required) The human-readable name of the block.
...
+ *   An array containing required elements 'subject' (the block's localized
+ *   title)

While we are in here, it might really be nice to clarify somewhere that 'info' in hook_block_info() controls the block's title on admin screens, while 'subject' in hook_block_view() controls the block's title when it is being displayed. I always forget which is which, personally :)

+ *   - 'cache': (optional) A bitmask describing what kind of caching is
+ *     appropriate for the block. common.inc provides the following bitmask
+ *     constants for defining cache granularity:

Instead of "common.inc" in the last sentence, how about just "Drupal"?

+ *     screen. Note: If you set a region that isn't available in the currently
+ *     enabled theme the block will be disabled.

Add a comma after "theme".

+ *   - 'visibility': (optional) Initial value for flag controlling how to 
+ *     interpret the 'pages' value. See schema definition in block.install
+ *     for values.

I had trouble understanding what this meant, even though I already know :) The first sentence doesn't seem like a complete sentence, and for the second, I think it's worth listing the values explicitly rather than referring people to block.install.

+ *   - 'pages': (optional) Initial value for a list of paths or PHP code that
+ *     determines the pages this block should be displayed on. Interpretation
+ *     of this value (i.e., include, exclude, or PHP) is controlled by the

If doing the above, then we can probably just remove the "include, exclude or PHP" parenthetical remark here.

+  $blocks['syndicate'] = array(
+    'info' => t('Syndicate'),
+    'cache' => DRUPAL_NO_CACHE
   );

Trailing comma on the last array element?

+  $blocks['recent'] = array(
+    'info' => t('Recent content'),
+    // DRUPAL_CACHE_PER_ROLE will be assumed
   );

Trailing period on the code comment?

 function hook_block_view($delta = '') {
...
+    case 'syndicate':
+      $block['subject'] = t('Syndicate');
+      $block['content'] = theme('feed_icon', array('url' => url('rss.xml'), 'title' => t('Syndicate')));

It's a shame that the example we're using here does not follow our own advice of putting a renderable array in 'content', but I guess we can fix that in some other issue (if/when we fix the node module itself to do that!).

Perhaps time to rename this issue? What started as a simple example change has turned into a fairly major rewrite.

I find this line confusing: "Each block your module provides must be given a value of "delta" in hook_block_info()." To me it implies I must code something $value = "delta". The next line is great. Suggest something like:
Each block your module provides is identified by a unique identifier referred to as "delta". Delta is passed into the other block hooks as an argument to identify the block being configured or viewed.

This line seems out of place: "preferably meaningful strings, since string deltas lead to self-documenting code". It seems more like a coding style guide comment belonging in a handbook than something in the API documentation. The next sentence provides a more compelling argument for string based deltas in any event.

Regarding common.inc. I found it very helpful having the reference. There is some additional information there that someone researching caching could find useful. I vote to keep it. Actually, in some ways it makes more sense to remove all of the caching information and only refer to common.inc so there isn't double comment maintenance. Most of the caching information in the doc header is taken verbatim from common.inc.

Regarding "(i.e., include, exclude, or PHP)" in the pages value description, since we appear to be going towards inclusiveness of information I think we should keep this regardless of whether or not the value parameters are included in visibility. If someone is doing a lookup and goes straight to that point the explanation is clearer.

The extended information on selecting a good value for delta in #20 is great information but it seems more of an implementation detail than documentation of the interface. It seems better suited for a handbook page or a new Block section under "A few components of Drupal" on http://api.drupal.org/api/7. Actually, a Block section might be a good place to expand on how block caching works.

I did a search and didn't see an issue for the non standards-compliant code in node module (My search-foo is not strong, though). Does/should an issue be created?

All/most good points. I will make a new patch later today.

Here's a new patch (and new issue title). Regarding common.inc - on api.drupal.org those constants will appear as links, so I think it is correct to leave out the name of the file they appear in (besides which, they could always move).

Regarding the early rendering of node blocks, we should probably file an issue on the node module.

Great job on reworking hook_block_info() and nice catch on the renderable array stuff!

I'm not entirely comfortable with leaving out the common.inc reference. The link created by doxygen doesn't give an information scent that there's an entire paragraph of additional information (on a topic that can be hard to get information on). However, in this case I think the important information has been captured in the patch and the remaining information is of arguable importance so I can go along with the decision.

Looks good to go.

This looks good now. Found one thing which didn't feel quite right:

+++ modules/block/block.api.php	12 Aug 2010 17:17:11 -0000
@@ -14,19 +14,38 @@
+ * In hook_block_info(), you assign each block your module provides a unique
+ * identifier referred to as "delta" (the array key in the return value); delta

What's not feeling right about that? Happy to try to improve it, but I don't understand what's wrong... :)

"You assign each block your module provides" doesn't sound like proper English but maybe it is.

I think it's "proper", but if it's not clear enough for the average highly-literate but non-native speaker to understand, let's fix it up so it's better.

I think I did it that way in an attempt not to use the passive voice, but I guess probably it would be clearer with passive voice. So let's try this...

This looks great to me.

@torelad:

I'm not entirely comfortable with leaving out the common.inc reference. The link created by doxygen doesn't give an information scent that there's an entire paragraph of additional information (on a topic that can be hard to get information on).

I see the paragraph in common.inc that you're referring to, and I think it's a bug that it doesn't show up e.g. on api.drupal.org. I think those constants and that paragraph need to be grouped together inside a @defgroup, and then they will display linked together correctly as part of the API docs - that seems to be the right way to do it. Maybe we can create a separate issue for that?

Read really well now. Great. Committed to CVS HEAD. Thanks a lot.

RE #30 - there is already an issue on this:
#863428: DRUPAL_CACHE_* documentation unclear and incomplete in common.inc
The issue title may not indicate it, but it does address this issue.

Regarding this issue, I thinkwe should consider adding some of this to D6. It wouldn't be a straight port, because the hooks aren't the same, but the text about delta for instance, and a better example, would not be bad... Note that for D6 this is in the contrib repository. I will make the changes, possibly later today.

@David_Rothstein/@jhodgdon: Re common.inc, excellent. Thanks.

OK, I've updated the D6 hook_block doc. Here's the result. I didn't update the example for D6, as it wasn't quite as silly as the D7 example. This should be up on api.drupal.org at some point in the near future...

/**
 * Declare a block or set of blocks.
 *
 * Any module can declare a block (or blocks) to be displayed by implementing
 * hook_block(), which also allows you to specify any custom configuration
 * settings, and how to display the block.
 *
 * In hook_block(), each block your module provides is given a unique
 * identifier referred to as "delta" (the array key in the return value for the
 * 'list' operation). Delta values only need to be unique within your module,
 * and they are used in the following ways:
 * - Passed into the other hook_block() operations as an argument to
 *   identify the block being configured or viewed.
 * - Used to construct the default HTML ID of "block-MODULE-DELTA" applied to
 *   each block when it is rendered (which can then be used for CSS styling or
 *   JavaScript programming).
 * - Used to define a theming template suggestion of block__MODULE__DELTA, for
 *   advanced theming possibilities.
 * The values of delta can be strings or numbers, but because of the uses above
 * it is preferable to use descriptive strings whenever possible, and only use a
 * numeric identifier if you have to (for instance if your module allows users
 * to create several similar blocks that you identify within your module code
 * with numeric IDs).
 *
 * @param $op
 *   What kind of information to retrieve about the block or blocks.
 *   Possible values:
 *   - 'list': A list of all blocks defined by the module.
 *   - 'configure': Configuration form for the block.
 *   - 'save': Save the configuration options.
 *   - 'view': Process the block when enabled in a region in order to view its
 *     contents.
 * @param $delta
 *   Which block to return (not applicable if $op is 'list'). See above for more
 *   information about delta values.
 * @param $edit
 *   If $op is 'save', the submitted form data from the configuration form.
 * @return
 *   - If $op is 'list': An array of block descriptions. Each block description
 *     is an associative array, with the following key-value pairs:
 *     - 'info': (required) The human-readable name of the block. This is used
 *       to identify the block on administration screens, and is not displayed
 *       to non-administrative users.
 *     - 'cache': A bitmask of flags describing how the block should behave with
 *       respect to block caching. The following shortcut bitmasks are provided
 *       as constants in block.module:
 *       - BLOCK_CACHE_PER_ROLE (default): The block can change depending on the
 *         roles the user viewing the page belongs to.
 *       - BLOCK_CACHE_PER_USER: The block can change depending on the user
 *         viewing the page. This setting can be resource-consuming for sites
 *         with large number of users, and should only be used when
 *         BLOCK_CACHE_PER_ROLE is not sufficient.
 *       - BLOCK_CACHE_PER_PAGE: The block can change depending on the page
 *         being viewed.
 *       - BLOCK_CACHE_GLOBAL: The block is the same for every user on every
 *         page where it is visible.
 *       - BLOCK_NO_CACHE: The block should not get cached.
 *     - 'weight': (optional) Initial value for the ordering weight of this
 *       block. Most modules do not provide an initial value, and any value
 *       provided can be modified by a user on the block configuration screen.
 *     - 'status': (optional) Initial value for block enabled status. (1 =
 *       enabled, 0 = disabled). Most modules do not provide an initial value,
 *       and any value provided can be modified by a user on the block
 *       configuration screen.
 *     - 'region': (optional) Initial value for theme region within which this
 *       block is set. Most modules do not provide an initial value, and
 *       any value provided can be modified by a user on the block configuration
 *       screen. Note: If you set a region that isn't available in the currently
 *       enabled theme, the block will be disabled.
 *     - 'visibility': (optional) Initial value for the visibility flag, which
 *       tells how to interpret the 'pages' value. Possible values are:
 *       - 0: Show on all pages except listed pages. 'pages' lists the paths
 *         where the block should not be shown.
 *       - 1: Show only on listed pages. 'pages' lists the paths where the block
 *         should be shown.
 *       - 2: Use custom PHP code to determine visibility. 'pages' gives the PHP
 *         code to use.
 *       Most modules do not provide an initial value for 'visibility' or
 *       'pages', and any value provided can be modified by a user on the block
 *       configuration screen.
 *     - 'pages': (optional) See 'visibility' above.
 *   - If $op is 'configure': optionally return the configuration form.
 *   - If $op is 'save': return nothing; save the configuration values.
 *   - If $op is 'view': return an array which must define a 'subject' element
 *     (the localized block title) and a 'content' element (the block body)
 *     defining the block indexed by $delta. If the "content" element
 *     is empty, no block will be displayed even if "subject" is present.
 *
 * For a detailed usage example, see block_example.module.
 */
function hook_block($op = 'list', $delta = 0, $edit = array()) {