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().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

I 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!

thedavidmeister’s picture

Depending 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 :)

thedavidmeister’s picture

We should also change/remove:

This function is usually called from within another function, like drupal_get_form() or a theme function.

From the docs of drupal_render().

thedavidmeister’s picture

Issue tags: +Novice

this could be a novice task

jlindsey15’s picture

Assigned: Unassigned » jlindsey15

Well, since I am a novice, might as well get a few more simple tasks under my belt!

jlindsey15’s picture

Status: Active » Needs review
FileSize
1.14 KB

I 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.

pplantinga’s picture

Status: Needs review » Needs work

Nitpick: one line in documentation is missing a '*'

Other than that, gets my +1

jhodgdon’s picture

Also:

+ * 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

put parentheses after theme() in the second line.

The wording is good though and thanks for the patch!

jlindsey15’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

Fixed.

Kingdutch’s picture

Status: Needs review » Reviewed & tested by the community

I'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 : )

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs work

trailing whitespace in #9

jlindsey15’s picture

FileSize
1.14 KB

Whitespace fixed.

jlindsey15’s picture

Status: Needs work » Needs review
pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

One more nitpick: There is an extra space after the . in this line:

+ * Avoid calling this function directly.  It is preferable to replace direct calls
jlindsey15’s picture

FileSize
1.14 KB

Here you go.

jlindsey15’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I'll get this committed shortly.

Kiphaas7’s picture

Status: Needs work » Reviewed & tested by the community

Nitpick:

* 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.

Kiphaas7’s picture

Status: Reviewed & tested by the community » Needs work
jlindsey15’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.15 KB

Okay hopefully this is the last one!

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

I hope so too.

Kiphaas7’s picture

Seems like a huge improvement in documentation, looks great!

thedavidmeister’s picture

Status: Reviewed & tested by the community » Needs work

Can we get an @see drupal_render() in the docblock as well?

jlindsey15’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
pplantinga’s picture

Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

Thanks for the iterations and thanks Kiphaas7 for catching the 80-character limit! I'll get this committed shortly.

jhodgdon’s picture

Assigned: jlindsey15 » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks 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.

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