In the docblock for hook_block_view:

/**
*  content: The content of the block's body. This may be a renderable array (preferable) or a string containing *  rendered HTML content. 
*/

But in the example code, both block examples return strings. Since a renderable array is preferred, at least one of the block examples should return a renderable array for its content.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: Documentation problem with hook_block_view » hook_block_view example should return a render array

Seems like a reasonable suggestion. Thanks!

duellj’s picture

Though if the example from hook_block_view is pulled from node_block_view, shouldn't node_block_view also be switched to use the render API?

jhodgdon’s picture

Should *have* been. If it isn't by now, it's probably too late. But if we do change the example here to something other than what node_block_view() does, we should verify that the example works.

jhodgdon’s picture

Status: Active » Needs review
FileSize
1.61 KB

I took an example from the menu module, which seemed to be the best one out there.

drewish’s picture

That's definitely a valid example but I'm not sure it's the most helpful since it delegates to a helper function and doesn't show the format of the render array.

jhodgdon’s picture

Status: Needs review » Needs work

True. OK, we need a better example.

drewish’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

Does blog_block_view() seem like a better example? The downside of it is that it ignores $delta. Do we try to always use a core example? Or can we roll custom examples?

jhodgdon’s picture

Custom examples that are concise and clear are great. Maybe the Examples project would be a good place to look? http://drupal.org/project/examples

jhodgdon’s picture

Status: Needs review » Needs work
jhodgdon’s picture

And what I would do is take the function body from
http://api.drupal.org/api/examples/block_example--block_example.module/f...
and put it directly into the hook_block_view body as well rather than calling a second function, for better illustration. And probably the comments don't need to be quite so verbose.

Lars Toomre’s picture

That is a bit of an odd example.

If I am reading the patch correctly, the $block array is only returned sometimes; at other times, when the if statement is not TRUE, there is no return at all. I think this needs to be corrected before going in the docs to always return an array, even if empty.

jhodgdon’s picture

RE #11 - which example are you referring to by "this"?

Lars Toomre’s picture

As I understand, the above patch when applied results in the final example:

function hook_block_view($delta = '') {
  // This example comes from blog.module which only provides a single block
  // so it ignores the $delta parameter.
  global $user;
 
  if (user_access('access content')) {
    $result = db_select('node', 'n')
      ->fields('n', array('nid', 'title', 'created'))
      ->condition('type', 'blog')
      ->condition('status', 1)
      ->orderBy('created', 'DESC')
      ->range(0, variable_get('blog_block_count', 10))
      ->addTag('node_access')
      ->execute();
 
    if ($node_title_list = node_title_list($result)) {
      $block['subject'] = t('Recent blog posts');
      $block['content']['blog_list'] = $node_title_list;
      $block['content']['blog_more'] = array(
        '#theme' => 'more_link',
        '#url' => 'blog',
        '#title' => t('Read the latest blog entries.'),
      );

      return $block;
    }
  }
}

This is the example function that has an inconsistent return.

jhodgdon’s picture

Ah. We're asking for a different patch to be made with a better example. That's why this patch has been set to "needs work". See #9-10 for suggested example to include in the new patch.

jn2’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
1.76 KB

How about adapting the current example to return renderable arrays? It's a good example with no helper functions, concise and clear, doesn't ignore $delta, and has two places for renderable arrays.

Here's a patch that does that.

Lars Toomre’s picture

That is a good example and appears to be quite clear. My only small nit is how odd 'node.module.' looks upon first reading of the comments.

jhodgdon’s picture

node.module - agreed it looks a bit odd with a . after it, but nonetheless that is standard in Drupal comments.

That example looks OK to me. Has someone tested it to make sure it actually works?

jn2’s picture

Yes, I checked it out. Made the changes on a local Git branch of the latest version of core and found no problems in the site. I also ran the SimpleTest tests for the node module, all green.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Sounds good then, thanks!

Dries’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/block/block.api.php
@@ -205,23 +205,27 @@ function hook_block_save($delta = '', $edit = array()) {
-      $block['content'] = theme('feed_icon', array('url' => url('rss.xml'), 'title' => t('Syndicate')));
+      $block['content'] = array(
+        '#theme' => 'feed_icon',
+        '#url' => 'rss.xml',
+        '#title' => t('Syndicate'),
+      );

Is that actually correct? We lost a call to url() in the process it seems.

Now, I did some grepping (read: investigation) and found this:

modules/block/block.api.php:      $block['content'] = theme('feed_icon', array('url' => url('rss.xml'), 'title' => t('Syndicate')));
modules/node/node.module:      $block['content'] = theme('feed_icon', array('url' => 'rss.xml', 'title' => t('Syndicate')));

Note that one call uses url('rss.xml') and one uses just 'rss.xml'. In other words, it looks like we might have fixed a bug along the way -- whether that was intentional or not. I have not tested the update example.

Last but not least, shouldn't we make sure core follows our examples? :)

Thoughts before I commit this patch?

jn2’s picture

Status: Needs work » Needs review

I checked into it some, and there does indeed seem to be a bug in modules/block/block.api.php. I added the call to url() to node.module, and the feed icon doesn't work right with that inserted. (I ran the node tests, and those all pass with any of these versions.)

In Firefox 4, with the call to url() inserted, the browser looks for rss.xml and returns a 'Server not found' page. With node.module unchanged, or with it changed as in the patch, it looks for example.com/rss.xml and shows the Syndicate page.

Yes, I agree that core should follow the examples. :) If this is confirmed, I'll make a patch for node.module reflecting this change.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looking at theme_feed_icon() http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_feed... ... It looks like the url theme variable is passed into l(), so it should either be an internal path or an external URL. So there is no need for wrapping with url().

So, the current node_block_view() is correct http://api.drupal.org/api/drupal/modules--node--node.module/function/nod...

And the example hook function body in the patch in #15 is also correct, and makes the example here match the code in node.module, so I will set this patch back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for investigating this a bit more, jn2 and jhodgdon. That was helpful.

I've committed the documentation fix to 7.x and 8.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Novice

Automatically closed -- issue fixed for 2 weeks with no activity.