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.
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.
Comment | File | Size | Author |
---|---|---|---|
#15 | hook_fix-1034248-15.patch | 1.76 KB | jn2 |
#7 | docs_1034248.patch | 1.97 KB | drewish |
#4 | 1034248.patch | 1.61 KB | jhodgdon |
Comments
Comment #1
jhodgdonSeems like a reasonable suggestion. Thanks!
Comment #2
duellj CreditAttribution: duellj commentedThough 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?
Comment #3
jhodgdonShould *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.
Comment #4
jhodgdonI took an example from the menu module, which seemed to be the best one out there.
Comment #5
drewish CreditAttribution: drewish commentedThat'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.
Comment #6
jhodgdonTrue. OK, we need a better example.
Comment #7
drewish CreditAttribution: drewish commentedDoes 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?
Comment #8
jhodgdonCustom examples that are concise and clear are great. Maybe the Examples project would be a good place to look? http://drupal.org/project/examples
Comment #9
jhodgdonThis one looks good:
http://api.drupal.org/api/examples/block_example--block_example.module/f...
Comment #10
jhodgdonAnd 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.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedThat 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.
Comment #12
jhodgdonRE #11 - which example are you referring to by "this"?
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedAs I understand, the above patch when applied results in the final example:
This is the example function that has an inconsistent return.
Comment #14
jhodgdonAh. 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.
Comment #15
jn2 CreditAttribution: jn2 commentedHow 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.
Comment #16
Lars Toomre CreditAttribution: Lars Toomre commentedThat 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.
Comment #17
jhodgdonnode.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?
Comment #18
jn2 CreditAttribution: jn2 commentedYes, 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.
Comment #19
jhodgdonSounds good then, thanks!
Comment #20
Dries CreditAttribution: Dries commentedIs that actually correct? We lost a call to
url()
in the process it seems.Now, I did some grepping (read: investigation) and found this:
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?
Comment #21
jn2 CreditAttribution: jn2 commentedI checked into it some, and there does indeed seem to be a bug in
modules/block/block.api.php
. I added the call tourl()
tonode.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 forrss.xml
and returns a 'Server not found' page. With node.module unchanged, or with it changed as in the patch, it looks forexample.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.Comment #22
jhodgdonLooking 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.
Comment #23
Dries CreditAttribution: Dries commentedThanks 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!