Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system

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

Yes please! Thanks for opening this.

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

Definitely, I will bring it up in the Twig/Theme layer call tonight! Adding to the agenda right now. http://lb.cm/twig#meetings

And adding a very related meta.

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

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

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.

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.

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

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

Adding appropriate tag per #16

Also, a personal: w00t!

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.

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

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.

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

Makes sense to me, thanks @xjm!

Now postponed on:
#2191101: Replace calls to theme() with drupal_render() in Views plugins

Followups:
#2191107: Update API documentation for _theme() to clearly indicate that it is for internal use only
#2191115: Clean up stale references to theme('foo') in documentation
#2191135: Refactor use of _theme() in tests where possible

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

#2094585: [policy, no patch] Core review bonus from https://drupal.org/comment/8461329#comment-8461329

#2191101: Replace calls to theme() with drupal_render() in Views plugins has been committed.

Nice, thanks!

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

Just a couple of nit-picky things.

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

   Double semi-colon.

2. +++ b/core/includes/pager.inc
   @@ -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'); ?

Added a draft change notice for this at https://drupal.org/node/2195739 :)

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

https://drupal.org/node/2195739/revisions/view/6916007/6916139

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

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.

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/toolbar.menu.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. :)

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.info.yml:name: 'Block test theme'

Not the function, the word. :)

MOAR TAGS

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\('

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!

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

@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?

That's fine with me too.

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.

Published the change record.

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