The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.For a while I thought the drupal_render_children just populated #children entries, not actually returning a concatenated string. Obviously I didn't read the documentation closely, but (thanks to Eclipse) trusted the lack of a @return tag.
I attach a small patch with the @return tag added (made against D7.24, but the issue exists also in D8).
I also include a fix for a slightly unrelated issue, where the drupal_render documentation mentions drupal_get_form as a typical caller (as in D6), while in D7 drupal_get_form returns the unrendered array structure. I put drupal_render_page as an example, but maybe there are better examples?
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2164597-drupal_render_children-doc-D7.patch | 1.02 KB | longwave |
| #7 | 2164597-drupal_render_children-doc.patch | 1.07 KB | longwave |
Comments
Comment #2
jhodgdonThanks for the issue report and and the patch! A few thoughts:
- The patch needs to be made with git diff in order to apply properly. See https://drupal.org/novice for patching instructions.
- You need to have a blank line before @return.
- We need to fix this in Drupal 8 first.
- And if you want to fix a different issue, please don't combine it with this patch, as it's unrelated. File a separate issue.
Comment #3
stephaneqBased on initial patch by emok, here is a D8 patch
Comment #4
longwaveLooks good to me.
Comment #5
jhodgdonCould we perhaps move the existing text that is in the function doc:
This renders all children of an element using drupal_render() and then joins them together into a single string.
into the @return (with a little editing to make it into more like "what this returns" text)? It seems silly to have that explanation of what is returned outside of the @return.
Comment #6
longwaveSo, something like this?
Comment #7
longwaveWhile we're here we can typehint the other parameters and add an (optional).
Comment #8
jhodgdonThanks -- looks good to me!
Comment #10
jhodgdonThat last failure message was for the patch in #6. The patch in #7 passed.
Comment #11
jhodgdonThere's an "avoid commit conflicts" issue that also touches common.inc and although I don't think there's a conflict, I'm being a bit cautious and will wait to commit this for a while.
#1996238: Replace hook_library_info() by *.libraries.yml file
Comment #12
xjmI confirmed that this applies fine on top of or under that patch, btw.
Comment #13
xjmComment #14
jhodgdonRE #12, that may be but recently I had a patch that both webchick and I were certain wouldn't conflict, which she then committed, I got berated for triggering the need for a reroll of a big issue. So I'm being extra cautious now. It's not like this issue is too urgent.
Comment #15
webchickAt the moment there are only two viable (non-"needs work") "avoid commit conflicts" patches: #2002134: Move TypedData metadata introspection from data objects to definition objects and #1862202: Objectify the language system. I confirmed this patch doesn't conflict with either of them, so getting it in while it's hot. Since it's also confusing people in D7, I think it'd be good to get it in sooner than later.
Committed and pushed to 8.x. Thanks! Moving to 7.x for backport.
Comment #16
longwaveComment #17
jhodgdonLooks fine, thanks! I will get it committed sometime soon, unless another committer gets to it before me.
Comment #18
jhodgdonThanks all! Committed to 7.x.