The theme() doc block sums up this issue nicely:

 * Generates themed output.
 * All requests for themed output must go through this function (however,
 * calling the theme() function directly is strongly discouraged - see next
 * paragraph). It examines the request and routes it to the appropriate
 * @link themeable theme function or template @endlink, by checking the theme
 * registry.
 * 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 with a #theme key to drupal_render(), which in turn calls
 * theme().

In order to really drive home this dissuasion, we should mark the function as being "private" with an underscore.

Calling the theme() function directly raises several issues:

  • Circumvents caching.
  • Circumvents defaults of types defined in hook_element_info(), including attached assets
  • Circumvents the pre_render and post_render stages.
  • Circumvents JavaScript states information.

theme() should only be invoked as part of the drupal_render process in order to convert data structures into output.

This issue has become clear as we have progressively hidden asset-adding functions like drupal_add_js:

#2171071: Rename drupal_add_library() to _drupal_add_library() and remove its uses
#2073819: [META] Remove direct calls to drupal_add_css()
#2073823: [META] Remove drupal_add_js() calls
#2168113: Add leading underscore and other discouragement to drupal_add_css() and drupal_add_js()

By making _theme() explicitly a private Drupal function, we will also reduce confusion for module developers. This is something that until only recently was still fuzzy for me -- Do I use drupal_render() or theme()?

Proposed resolution

  1. Prefix theme() with an underscore to mark it as private: _theme()
  2. Write a very detailed Change Record that details why theme() should not be called directly. Draft at
  3. Update documentation to provide best-practice approaches to building data structures, attaching assets and ushering them efficiently into the drupal_render() pipeline.

User interface changes


API changes

The theme() function will be removed from the public API.

Original report by @jessebeach

#45 interdiff.txt986 bytesCottser
#45 2173655-45.patch52.16 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 64,363 pass(es). View
#44 2173655.44.patch52.16 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 64,454 pass(es). View
#44 37-44-patch_diff.txt1001 bytesalexpott
#40 non-rename-changes.txt10.97 KBxjm
#37 interdiff.txt1.48 KBrlmumford
#37 2173655-theme_to__theme-37.patch52.15 KBrlmumford
PASSED: [[SimpleTest]]: [MySQL] 64,312 pass(es). View
#29 theme-to-_theme-2173655-29.patch53.7 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 64,346 pass(es). View
#21 theme-2173655-20.patch62 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#7 interdiff.txt872 bytesCottser
#7 2173655-7.patch62.06 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,879 pass(es). View
#5 replace-theme-with-_theme-2173655-1.patch66.66 KBjessebeach
FAILED: [[SimpleTest]]: [MySQL] 59,743 pass(es), 1 fail(s), and 0 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more


jessebeach’s picture

Issue summary: View changes
xjm’s picture

Issue tags: +beta blocker

This is something that we need to do before beta IMO, if it's critical.

Cottser’s picture

Yes please! Thanks for opening this.

jessebeach’s picture

Issue summary: View changes
jessebeach’s picture

Status: Active » Needs review
66.66 KB
FAILED: [[SimpleTest]]: [MySQL] 59,743 pass(es), 1 fail(s), and 0 exception(s). View

I used the following regex to find instances of theme() to change to _theme().


I found one false positive in Drupal/views/Plugin/views/field/FieldPluginBase.php. This plugin has a theme method that is a wrapper for _theme(). By not including [A-Z] in the portion of the regex that matches the arguments to theme(), this false positive is avoided.

Lowercase letters are matched in the arguments portion of the regex -- [a-z] -- in order to match array() which is a valid argument to theme() when passing multiple theme hook suggestions.

The regex found 134 matches across 41 files.

I insert the underscore with $1_$2.

Pages are loading locally without issue. I'd like to give the bot a chance to pass judgement.

Status: Needs review » Needs work

The last submitted patch, 5: replace-theme-with-_theme-2173655-1.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
62.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,879 pass(es). View
872 bytes

Nice! Hope you don't mind, this should fix the one test fail.

jessebeach’s picture

Nice! Thanks for the test fix Cottser.

jessebeach’s picture

At this point, we really need input from additional Theme layer devs. I'd like to get some consensus around this change.

Cottser’s picture

Definitely, I will bring it up in the Twig/Theme layer call tonight! Adding to the agenda right now.

And adding a very related meta.

joelpittet’s picture

I totally for this issue! RTBC++
Any theme(), render(), drupal_render() call from preprocess/alter hooks makes it tricky to autoescape and mark them as "safe strings" #1825952: Turn on twig autoescape by default. So this would help:)

joelpittet’s picture

We will still need to convert these _theme() calls to renderable arrays, but we can still do that in conversion, not here because some can cause trouble...

webchick’s picture

Hm. In other similar issues, such as #2073819: [META] Remove direct calls to drupal_add_css() and #2073823: [META] Remove drupal_add_js() calls, we removed the calls first, then deprecated the function. I think we should do the same here, since according to the patch there are 131 remaining calls in core. If core can't even not use this function itself, it's not really reasonable to expect contrib not to use it either, so marking it deprecated feels premature.

Also, this should be run past a core committer (probably catch) to ensure the API change is actually approved + we're comfortable adding yet another beta blocker.

jessebeach’s picture

Assigned: Unassigned » catch

Assigning to catch to obtain his opinion.

I'll link the "theme() to drupa_render()" issue here once I've created it.

jessebeach’s picture

Status: Needs review » Postponed

Postponing on #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(). We should still come to consensus about whether this change should be applied if and once the issue is unblocked.

catch’s picture

Assigned: catch » Unassigned

I'm personally fine with renaming the function, it's been 'discouraged' for ages, but should've been marked deprecated for ages. It's also a hard blocker to certain Twig functionality

However agreed on #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() first.

thedavidmeister’s picture

Yup, I totally agree with this issue, and #13, #15 and #16.

I believe this is the direction we want to move in.

markcarver’s picture

jessebeach’s picture

Status: Postponed » Needs work
catch’s picture

Looks like there's a few theme() calls outside tests in Views left. However that issue is over 300 comments so not keen on re-opening - could we open one more sub-issue for those and postpone this on that? This issue should stick to only the rename/docs if at all possible.

The calls in tests I'm fine with leaving as _theme() at least for now.

xjm’s picture

Status: Needs work » Needs review
62 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Good times.

Edit: Crossposted. I'm unclear on which other uses we need to remove. There's a lot of references to theme('whatever') in the docs.

Edit 2: Empty patch anyway...

xjm’s picture

Ah. From #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() #60:

I just committed the last of these from the meta issue. WOOT!

But unfortunately, I found a few more. :(

./core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php:    $output = theme($callback, $variables);
./core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php:    $this->content = theme('table', array('header' => $header, 'rows' => $rows, 'sticky' => TRUE));
./core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php:    $this->content = theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => $attributes, 'caption' => $caption, 'colgroups' => $colgroups, 'sticky' => FALSE));
./core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php:    $this->content = theme('table', array('header' => $header, 'rows' => array(), 'empty' => t('No strings available.')));
./core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php:    $this->content = theme('table', array('rows' => $rows));
./core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php:   *   - $variables['attributes'] as passed in to theme()
./core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php:   * Test that theme() returns expected data types.
./core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php:    // theme_test_false is an implemented theme hook so theme() should return a
./core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php:      $output = theme('theme_test_foo', array('foo' => $example));
./core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php:    // suggestionnotimplemented is not an implemented theme hook so theme()
./core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php:    $output = theme(array('suggestionnotimplemented'));
./core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php:    $output = theme('node', node_view($node));
./core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php:    $this->assertTrue(strpos($output, "CALL: theme('node')") !== FALSE, 'Theme call information found.');
./core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php:    $output = theme('node', node_view($node2));
./core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php:    $output = theme('node', node_view($node));
./core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.php:      $GLOBALS['theme_test_output'] = theme('more_link', array('url' => 'user', 'title' => 'Themed output generated in a KernelEvents::REQUEST listener'));
./core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/ThemeTestController.php:    return theme('theme_test_template_test');
./core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/ThemeTestController.php:    return theme(array('theme_test__suggestion', 'theme_test'), array());
./core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.php:    return theme('twig_theme_test_php_variables');
./core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php:    return theme($this->themeFunctions(),
./core/modules/views/lib/Drupal/views/Plugin/views/row/RssFields.php:    return theme($this->themeFunctions(),
./core/modules/views/lib/Drupal/views/Plugin/views/style/Rss.php:    $output = theme($this->themeFunctions(),

It might be that the test-related ones need to stay there, but at least core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php and core/modules/views/lib/Drupal/views/Plugin/views/style/Rss.php should be looked into more.

There are a ton of other instances grep picks up, but they're all in comments, including out-dated ones that no longer make sense for the code below now that it's been converted to render arrays. The issue that deprecates theme() is definitely going to have its work cut out for it.

xjm’s picture

So, I'd suggest:

  1. An issue for the views plugins (blocking this one).
  2. This issue (simply the rename of theme() to _theme() so that it's scriptable).
  3. A followup issue to update the docs for _theme() so that they're more accurate. (Replace "Avoid" / "It is preferable" with clear indication that the function is for internal use only, and possibly marking it @deprecated if we have an existing replacement for all uses including the internal ones.) (Probably also beta-blocking.)
  4. An issue to correct documentation references to _theme('all_the_things') in comments where appropriate. (Not beta-blocking.)
  5. An issue to convert usage of _theme() in tests and test modules where appropriate. (Not beta-blocking.)
Cottser’s picture

Makes sense to me, thanks @xjm!

Cottser’s picture

Issue tags: +Twig

Tagging for rocketship. Thanks for creating those issues @xjm!

thedavidmeister’s picture

ianthomas_uk’s picture

Status: Postponed » Needs work
jessebeach’s picture

Status: Needs work » Needs review
53.7 KB
PASSED: [[SimpleTest]]: [MySQL] 64,346 pass(es). View

Some references to theme() should really be replaced with drupal_render() in order to provide better examples to developers using core code as a model. I've replaced these with drupal_render().

Some references to theme() are legitimately calling theme() in order to test it, so I've left these and prepended them with an underscore.

There are a few instances of theme() where the word 'theme' should not be prepended with an underscore, such as the phrase 'Block test theme' in BlockHiddenRegionTest.php, a theme in the styling sense.

Only a few functional changes were made and they all follow from this

function theme() {} becomes function _theme() {}

Because of #2190427: Replace theme() with drupal_render() in, no calls to theme() exist outside of drupal_render() and tests. Tests not directly testing theme() have been converted to drupal_render(). The remaining calls to theme() that are legitimately called in drupal_render() simply become _theme().

I know that Drupal installs with this patch. Let's see what the bot thinks. I also need reviews for content, since I added comments, the most significant one being the following in theme.api.php.

* Calling the _theme() function directly is highly discouraged. Building a
* renderable array is preferred. For example, rather than calling
* _theme('table', array()) in-place, one can assemble a renderable array as
* follows:
* @code
* $table = array(
*   '#type' => 'table',
*   '#header' => '',
*   '#rows' => array(),
* );
* @endcode
* Note that a table is defined as a type as well as a theme function. Building
* it as a type is preferred. The $table array can simply be passed along as
* a renderable array in a page build process. If necessary, the array may be
* rendered to a string by calling drupal_render().
* @code
* $output = drupal_render($table);
* @endcode

This patch is fresh and has no diff from #21.

jessebeach’s picture

Oh, and here's the grep again: /([\s\(]+)(theme\([\$\'a-z\)])/g

Please verify that I found all instances of theme()!

Cottser’s picture

Issue tags: +focus

Nice, thanks!

xjm’s picture

@jessebeach, can you document how you are grepping with that regex and creating the patch? I get:

[tesla:drupal | Thu 06:40:11] $ grep -re "([\s\(]+)(theme\([\$\'a-z\)])" *
grep: parentheses not balanced

Edit: Using -E instead of -e simply returned nothing, for the record.

rlmumford’s picture

Status: Needs review » Needs work

Just a couple of nit-picky things.

  1. +++ b/core/includes/
    @@ -2822,8 +2822,15 @@ function drupal_get_library($module, $name = NULL) {
    + * return drupal_render($table);;

    Double semi-colon.

  2. +++ b/core/includes/
    @@ -67,10 +67,14 @@ function pager_find_page($element = 0) {
    + *   $page = array('#theme' => 'pager');
    + *   $output .= drupal_render($pager);

    Shouldn't this be $pager = array('#theme' => 'pager'); ?

Gábor Hojtsy’s picture

Added a draft change notice for this at :)

Gábor Hojtsy’s picture

Issue summary: View changes
Cottser’s picture

Thanks very much @Gábor Hojtsy! I just added a note about early rendering which we should be avoiding whenever possible.

rlmumford’s picture

Status: Needs work » Needs review
52.15 KB
PASSED: [[SimpleTest]]: [MySQL] 64,312 pass(es). View
1.48 KB

Updated the patch as per comment #33. Also did a search for every instance of 'theme(' and updated where appropriate (I think there was only one that was missed!)

jessebeach’s picture

rlmumford, thanks for the finding those typos and misses on my part.

These are the last three mentions of "theme" that I can find. They refer to "theme" as in "the Bartik theme"

73:     $this->assertText('Block test theme(' . t('active tab') . ')', 'Default local task on blocks admin page is the block test theme.');

875:  * theme(s), and finally for the theme itself. The module order is determined

223:  * base theme(s), and finally for the active theme. The order is

The grep I posted isn't working on some command line versions of grep, including mine :/ I've narrowed this down to the space character \s. grep isn't recognizing it. So I've updated the grep to this, from the root of the repo:

grep -HRIn -E "([ \(]+)(theme\([\$\'a-z\)])" core

This pattern replaces \s with , a space character. This matches the same three entries noted above:

core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.php:73:    $this->assertText('Block test theme(' . t('active tab') . ')', 'Default local task on blocks admin page is the block test theme.');
core/modules/system/system.api.php:875: * theme(s), and finally for the theme itself. The module order is determined
core/modules/system/theme.api.php:223: * base theme(s), and finally for the active theme. The order is

This is my grep version: grep (BSD grep) 2.5.1-FreeBSD

xjm’s picture

Thanks @jessebeach, that works for me!

I discussed this patch with @webchick today and we agreed to target a commit for Friday, Feb. 21, during the "disruptive patch" window. So let's reroll it the 20th if needed and postpone any issues that change theme() calls starting the 19th.

This API change has signoff from @catch and the theme system maintainers, so it's just a matter of reviewing that all the changes in the final patch are correct (use git diff --color-words) and that all calls are converted.

xjm’s picture

10.97 KB

I checked all the theme() to _theme() changes locally and they look good. I also searched using a wider grep than the regex to confirm there were no missed cases:

[tesla:drupal | Fri 22:37:49] $ grep -r "theme(" * | grep -v "\_theme(" | grep -v "\->theme("
core/misc/dropbutton/dropbutton.js:      $primary.after(Drupal.theme('dropbuttonToggle', options));
core/misc/drupal.js:   *      content (checkPlain + Drupal.theme('placeholder'))
core/misc/drupal.js:            args[key] = Drupal.theme('placeholder', args[key]);
core/misc/drupal.js:   * Drupal.theme('placeholder', text).
core/misc/states.js:          $label.append(Drupal.theme('requiredMarker'));
core/misc/tabledrag.js:      var indent = Drupal.theme('tableDragIndentation');
core/misc/tabledrag.js:          $(Drupal.theme('tableDragChangedWarning')).insertBefore(self.table).hide().fadeIn('slow');
core/misc/tabledrag.js:        $group.find('td:first').prepend(Drupal.theme('tableDragIndentation'));
core/misc/tabledrag.js:    var marker = Drupal.theme('tableDragChangedMarker');
core/misc/tabledrag.js:      return '<div class="tabledrag-changed-warning messages messages--warning" role="alert">' + Drupal.theme('tableDragChangedMarker') + ' ' + Drupal.t('You have unsaved changes.') + '</div>';
core/misc/vertical-tabs.js:    $.extend(this, settings, Drupal.theme('verticalTab', settings));
core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.php:    $this->assertText('Block test theme(' . t('active tab') . ')', 'Default local task on blocks admin page is the block test theme.');
core/modules/ckeditor/js/ckeditor.admin.js:        $(Drupal.theme('ckeditorButtonGroupNamesToggle'))
core/modules/ckeditor/js/ckeditor.admin.js:        openGroupNameDialog(this, $(Drupal.theme('ckeditorToolbarGroup')), insertNewGroup);
core/modules/ckeditor/js/ckeditor.admin.js:            .append(Drupal.theme('ckeditorRow'));
core/modules/ckeditor/js/ckeditor.admin.js:            $row.children('.ckeditor-toolbar-groups').append(Drupal.theme('ckeditorNewButtonGroup'));
core/modules/ckeditor/js/ckeditor.admin.js:    var $ckeditorButtonGroupNameForm = $(Drupal.theme('ckeditorButtonGroupNameForm'));
core/modules/contextual/js/contextual.js:      .prepend(Drupal.theme('contextualTrigger'));
core/modules/edit/js/editors/formEditor.js:      var $formContainer = this.$formContainer = $(Drupal.theme('editFormContainer', {
core/modules/edit/js/theme.js:    html += Drupal.theme('editButtons', { buttons: settings.buttons });
core/modules/edit/js/views/EditorView.js:        var $backstage = $(Drupal.theme('editBackstage', { id: backstageId })).appendTo('body');
core/modules/edit/js/views/EntityToolbarView.js:          this.$fence = $(Drupal.theme('editEntityToolbarFence'))
core/modules/edit/js/views/EntityToolbarView.js:      var $toolbar = $(Drupal.theme('editEntityToolbar', {
core/modules/edit/js/views/EntityToolbarView.js:        .prepend(Drupal.theme('editToolgroup', {
core/modules/edit/js/views/EntityToolbarView.js:        label = Drupal.theme('editEntityToolbarLabel', {
core/modules/edit/js/views/EntityToolbarView.js:        label = Drupal.theme('editEntityToolbarLabel', {
core/modules/edit/js/views/FieldToolbarView.js:      this.setElement($(Drupal.theme('editFieldToolbar', {
core/modules/edit/js/views/FieldToolbarView.js:        .append(Drupal.theme('editToolgroup', {
core/modules/edit/js/views/FieldToolbarView.js:        .append(Drupal.theme('editToolgroup', {
core/modules/filter/filter.filter_html.admin.js:        this.$allowedHTMLDescription.append(Drupal.theme('filterFilterHTMLUpdateMessage', this.autoTags));
core/modules/locale/locale.admin.js:          var $marker = $(Drupal.theme('localeTranslateChangedWarning')).hide();
core/modules/locale/locale.admin.js:            marker = Drupal.theme('localeTranslateChangedMarker');
core/modules/locale/locale.admin.js:      return '<div class="localetranslate-changed-warning messages warning">' + Drupal.theme('localeTranslateChangedMarker') + ' ' + Drupal.t('Changes made in this table will not be saved until the form is submitted.') + '</div>';
core/modules/system/system.api.php: * theme(s), and finally for the theme itself. The module order is determined
core/modules/system/theme.api.php: * base theme(s), and finally for the active theme. The order is
core/modules/toolbar/js/            .append(Drupal.theme('toolbarMenuItemToggle', options));
core/modules/toolbar/js/views/ToolbarVisualView.js:        .append(Drupal.theme('toolbarOrientationToggle'));
core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php:  function theme(ResultRow $values) {

So "theme(s)" as a word (as above), the FieldPluginBase method, and JavaScript.

Attached includes the changes from the patch that are not simple renames, for reviewer convenience. It all looks correct to me, but would be great if someone with more frontend sense than me could RTBC. :)

xjm’s picture

Oh, and this one:
core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.php: $this->assertText('Block test theme(' . t('active tab') . ')', 'Default local task on blocks admin page is the block test theme.');

Is matching:

core/modules/block/tests/modules/block_test/themes/block_test_theme/ 'Block test theme'

Not the function, the word. :)

xjm’s picture

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

I had another thorough look too with another tool and just 'theme\(' just in case and same results. This is good to go, great work!

ag '(?<!>|_)theme\(' or ack '(?<!>|_)theme\('

alexpott’s picture

1001 bytes
52.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,454 pass(es). View

Rerolling because I broke it with #2028025: Expand CommentInterface to provide methods.

First, rewinding head to replay your work on top of it...
Applying: Init commit
Using index info to reconstruct a base tree...
M	core/modules/comment/comment.module
Falling back to patching base and 3-way merge...
Auto-merging core/modules/comment/comment.module

Yay for git!

Cottser’s picture

52.16 KB
PASSED: [[SimpleTest]]: [MySQL] 64,363 pass(es). View
986 bytes

Cleaning up super-duper-minor coding standards in @code samples. Thanks for all the work on this everyone :)

catch’s picture

@xjm and @webchick #39 - this is currently the only RTBC critical issue, and there's only a couple of majors, and we're only going to be committing crticals/majors until after the alpha, so why wait with this one?

xjm’s picture

That's fine with me too.

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK. Went ahead and committed/pushed to 8.x so this is in the next alpha. It'll mean contrib modules need to update for the next alpha, but shouldn't break much in the way of core patches.

Change notice is in place so moving straight to fixed.

Gábor Hojtsy’s picture

Published the change record.

Cottser’s picture

Related issue to remove more references to _theme(): #2201789: Don't print "_theme()" in twig_debug output

Status: Fixed » Closed (fixed)

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