As per #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(), apparently since D7 was launched we're supposed to be calling drupal_render() exclusively and not call theme() directly.
The current docs for theme() https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/t... say this:
All requests for themed output must go through this function. It examines the request and routes it to the appropriate theme function or template, by checking the theme registry.
While this is technically true, it is confusingly worded. While all themed output must go through this function, the preferred way to ensure the themed output goes through this function is to build a render array with a #theme key set and then pass that array to drupal_render().
Comment | File | Size | Author |
---|---|---|---|
#25 | theme-docs-2031319-25.patch | 1.44 KB | jlindsey15 |
#21 | theme-docs-2031319-21.patch | 1.15 KB | jlindsey15 |
#16 | theme-docs-2031319-16.patch | 1.14 KB | jlindsey15 |
#12 | theme-docs-2031319-12.patch | 1.14 KB | jlindsey15 |
#9 | theme-docs-2031319-9.patch | 1.14 KB | jlindsey15 |
Comments
Comment #1
jhodgdonI don't necessarily agree that we should not ever call theme() directly... but I agree that it's preferable to call drupal_render() or to return renderable arrays rather than markup strings wherever possible from functions. So yes, please provide a patch with your suggested change, thanks!
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedDepending on what happens at #2008450: Provide for a drillable variable structure in Twig templates it may become a really bad idea to call theme() directly.
Regardless, in the meantime I'm happy to go with the "preferable" wording. Anything is better than nothing :)
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedWe should also change/remove:
From the docs of drupal_render().
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commentedthis could be a novice task
Comment #5
jlindsey15 CreditAttribution: jlindsey15 commentedWell, since I am a novice, might as well get a few more simple tasks under my belt!
Comment #6
jlindsey15 CreditAttribution: jlindsey15 commentedI think this makes it clear that calling theme() directly should be avoided, but doesn't say that the world will end if you do call it.
Comment #7
pplantinga CreditAttribution: pplantinga commentedNitpick: one line in documentation is missing a '*'
Other than that, gets my +1
Comment #8
jhodgdonAlso:
put parentheses after theme() in the second line.
The wording is good though and thanks for the patch!
Comment #9
jlindsey15 CreditAttribution: jlindsey15 commentedFixed.
Comment #10
KingdutchI'm going to dare mark this as RTBC as I see that all the comments in the issue thread have been fulfilled, it's been otherwise approved to be clear and it seems clear to me : )
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedtrailing whitespace in #9
Comment #12
jlindsey15 CreditAttribution: jlindsey15 commentedWhitespace fixed.
Comment #13
jlindsey15 CreditAttribution: jlindsey15 commentedComment #14
pplantinga CreditAttribution: pplantinga commentedLooks good to me.
Comment #15
jhodgdonOne more nitpick: There is an extra space after the . in this line:
Comment #16
jlindsey15 CreditAttribution: jlindsey15 commentedHere you go.
Comment #17
jlindsey15 CreditAttribution: jlindsey15 commentedComment #18
jhodgdonThanks! I'll get this committed shortly.
Comment #19
Kiphaas7 CreditAttribution: Kiphaas7 commentedNitpick:
* the theme() function directly is strongly discouraged - see next paragraph). It
* Avoid calling this function directly. It is preferable to replace direct calls
* to the theme() function with calls to drupal_render() by passing a render array
These are 81 character lines.
OT: dreditor (greasemonkey userscripts) makes it real easy to spot these small errors.
Comment #20
Kiphaas7 CreditAttribution: Kiphaas7 commentedComment #21
jlindsey15 CreditAttribution: jlindsey15 commentedOkay hopefully this is the last one!
Comment #22
pplantinga CreditAttribution: pplantinga commentedI hope so too.
Comment #23
Kiphaas7 CreditAttribution: Kiphaas7 commentedSeems like a huge improvement in documentation, looks great!
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commentedCan we get an
@see drupal_render()
in the docblock as well?Comment #25
jlindsey15 CreditAttribution: jlindsey15 commentedComment #26
pplantinga CreditAttribution: pplantinga commentedComment #27
jhodgdonThanks for the iterations and thanks Kiphaas7 for catching the 80-character limit! I'll get this committed shortly.
Comment #28
jhodgdonThanks again everyone! I committed this patch to 8.x, and since the information was also correct for 7.x and the patch applied (with a little fuzz), I also committed it to 7.x.