Problem/Motivation

Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().

There are numerous uses of _theme() (or theme() in current HEAD) in tests:

./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');

Some of these might be appropriate internal tests for the theme system, but others should probably be replaced.

Proposed resolution

Refactor the tests and test modules to use drupal_render() where possible.

Remaining tasks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

star-szr’s picture

Issue tags: +Twig

Tagging for rocketship purposes. Thanks @xjm!

RainbowArray’s picture

Assigned: Unassigned » RainbowArray
Status: Postponed » Active

I just converted a few of the tests for a patch to #1939008: Convert theme_table() to Twig, so I can try converting the rest of the tests and then post a patch.

RainbowArray’s picture

Assigned: RainbowArray » Unassigned
Status: Active » Needs review
FileSize
11.91 KB

Here's a first stab at this. There's a decent chance I have done at least a few of these horribly wrong. Hopefully at least some of these changes are helpful.

Status: Needs review » Needs work

The last submitted patch, 4: refactor-theme-tests-2191135-4.patch, failed testing.

The last submitted patch, 4: refactor-theme-tests-2191135-4.patch, failed testing.

RainbowArray’s picture

joelpittet explained to me that drupal_render can't have arrays passed in directly. Revised patch to take that into account.

RainbowArray’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: refactor-theme-tests-2191135-5.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
12.12 KB
5.08 KB

Fixed some dumb PHP syntax errors on my part.

RainbowArray’s picture

Some of these patches have the wrong comment numbers in the titles and interdiffs. Sorry about that. Time to stop patching and go to sleep I think.

Status: Needs review » Needs work

The last submitted patch, 10: refactor-theme-tests-2191135-6.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
12.12 KB
1.68 KB

Two more syntax bugs.

Status: Needs review » Needs work

The last submitted patch, 13: refactor-theme-tests-2191135-14.patch, failed testing.

xjm’s picture

This was postponed on #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system because they will collide and the other is going to be an "Avoid commit conflicts" sort of patch. If you can get this green before that issue gets rerolled, great! That will mean fewer instances to rename and less outdated code lyting around. But the other will come first because it blocks beta.

RainbowArray’s picture

Sorry, I misunderstood the sequencing of these patches. Given the number of fails and exceptions with my attempt, I'm fine with letting the other patch finish up first.

xjm’s picture

star-szr’s picture

Thanks for getting this started @mdrummond. Here are a few notes to help move things along:

  1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -2888,7 +2888,8 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') {
    -    $output = theme($callback, $variables);
    +    $theme_output = array('#theme' => $callback, $variables);
    +    $output = drupal_render($theme_output);
    

    This is one we should probably leave alone to use the internal _theme() method for now since it's right on WebTestBase. It would make sense to refactor the function to accept a render array (or perhaps add a new method that does this) but that can happen in a separate issue.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
    @@ -34,7 +34,7 @@ public static function getInfo() {
    -    theme_enable(array('test_theme'));
    +    \Drupal::service('theme_handler')->enable(array('test_theme'));
    

    Out of scope, please leave this for #2151469: Clean-up usage of deprecated list_themes() and _system_rebuild_theme_data() in favor of theme_handler service.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
    @@ -48,9 +48,10 @@ function testTwigDebugMarkup() {
    -    $output = theme('node', node_view($node));
    +    $node_output = array('#theme' => 'node', 'node' => node_view($node));
    +    $output = drupal_render($node_output);
    
    @@ -59,7 +60,8 @@ function testTwigDebugMarkup() {
    -    $output = theme('node', node_view($node2));
    +    $node2_output = array('#theme' => 'node', 'node' => node_view($node2));
    +    $output = drupal_render($node2_output);
    
    @@ -68,7 +70,8 @@ function testTwigDebugMarkup() {
    -    $output = theme('node', node_view($node));
    +    $node_output2 = array('#theme' => 'node', 'node' => node_view($node));
    +    $output = drupal_render($node_output2);
    

    'node' => should be '#node' =>

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
    @@ -48,9 +48,10 @@ function testTwigDebugMarkup() {
    -    $this->assertTrue(strpos($output, "CALL: theme('node')") !== FALSE, 'Theme call information found.');
    +    $this->assertTrue(strpos($output, "CALL: drupal_render($node_output)") !== FALSE, 'Theme call information found.');
    

    This is handled by #2201789: Don't print "_theme()" in twig_debug output.

  5. +++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/ThemeTestController.php
    @@ -65,7 +66,8 @@ public function testTemplate() {
    -    return theme(array('theme_test__suggestion', 'theme_test'), array());
    +    $theme_test__suggestion = array('#theme' => 'theme_test__suggestion', 'theme_test' => array());
    +    return drupal_render($theme_test__suggestion);
    

    This is passing an array of theme hooks with no variables, should be closer to:

    '#theme' => array('theme_test__suggestion', 'theme_test');
    
botanic_spark’s picture

Assigned: Unassigned » botanic_spark
botanic_spark’s picture

Regarding the test:
/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php

When we replace

$output = theme('node', node_view($node));

with

$node_output = array('#theme' => 'node', '#node' => node_view($node));
$output = drupal_render($node_output);

the test could not be completed. The error that comes out is:
Fatal error: Call to a member function bundle() on a non-object in /home/www/ddd/www/core/modules/node/node.module on line 610

The error is happening in the function node_theme_suggestions_node

/**
 * Implements hook_theme_suggestions_HOOK().
 */
function node_theme_suggestions_node(array $variables) {
  $suggestions = array();
  $node = $variables['elements']['#node'];

  $suggestions[] = 'node__' . $node->bundle();
  $suggestions[] = 'node__' . $node->id();

  return $suggestions;
}

The $node is been used from $variables['elements']['#node'], but in this situation, this is not node object, but renderable array received from node_view($node);

Should we use

$node_output = array('#theme' => 'node', '#node' => $node);

instead?

botanic_spark’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
7.84 KB

Rerolled the patch #13, applied the changes recommended in #18.
Here is the patch and interdiff.

Changes from WebTestBase are excluded.

The part

$node_output = array('#theme' => 'node', '#node' => node_view($node));

is replaced with

$node_output = entity_view($node, 'full');

which solves all the problems from previous comment.

Status: Needs review » Needs work

The last submitted patch, 21: refactor-theme-tests-2191135-21.patch, failed testing.

botanic_spark’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
809 bytes

Replaced

$this->assertIdentical($output, FALSE, 'drupal_render() returns FALSE when a hook suggestion is not implemented.');

with

$this->assertIdentical($output, "", 'drupal_render() returns empty string when a hook suggestion is not implemented.');

Status: Needs review » Needs work

The last submitted patch, 23: refactor-theme-tests-2191135-23.patch, failed testing.

star-szr’s picture

Thanks for jumping on this @botanic_spark! I think for testing Twig debug markup we should really be building render arrays when possible (comment #21) rather than using entity_view(). I haven't had a chance yet to delve into this deeply yet. If we have to do node_view() that's fine but just hoping there is another way :)

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
    @@ -41,7 +41,7 @@ function setUp() {
    -   *   - $variables['attributes'] as passed in to _theme()
    +   *   - $variables['attributes'] as passed in to drupal_render()
    

    This should be left as is, _theme() accepts $variables but drupal_render() does not.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
    @@ -48,9 +48,10 @@ function testTwigDebugMarkup() {
    -    $this->assertTrue(strpos($output, "CALL: _theme('node')") !== FALSE, 'Theme call information found.');
    +    $this->assertTrue(strpos($output, "CALL: drupal_render('node')") !== FALSE, 'Theme call information found.');
    
    @@ -58,16 +59,17 @@ function testTwigDebugMarkup() {
    -    $this->assertTrue(strpos($output, "CALL: _theme('node__foo__bar')") !== FALSE, 'Theme call information found.');
    +    $this->assertTrue(strpos($output, "CALL: drupal_render('node__foo__bar')") !== FALSE, 'Theme call information found.');
    
    +++ b/core/themes/engines/twig/twig.engine
    @@ -54,7 +54,7 @@ function twig_render_template($template_file, $variables) {
    -    $output['debug_prefix'] .= "\n<!-- CALL: _theme('{$variables['theme_hook_original']}') -->";
    +    $output['debug_prefix'] .= "\n<!-- CALL: drupal_render('{$variables['theme_hook_original']}') -->";
    

    Please leave all these changes to Twig debug output to #2201789: Don't print "_theme()" in twig_debug output :)

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
    @@ -58,21 +58,23 @@ function testAttributeMerging() {
    -   * Test that _theme() returns expected data types.
    +   * Test that drupal_render() returns expected data types.
        */
       function testThemeDataTypes() {
    -    // theme_test_false is an implemented theme hook so _theme() should return a
    +    // theme_test_false is an implemented theme hook so drupal_render() should return a
         // string, even though the theme function itself can return anything.
         $foos = array('null' => NULL, 'false' => FALSE, 'integer' => 1, 'string' => 'foo');
         foreach ($foos as $type => $example) {
    -      $output = _theme('theme_test_foo', array('foo' => $example));
    +      $theme_test_foo = array('#theme' => 'theme_test_foo', '#foo' => $example);
    +      $output = drupal_render($theme_test_foo);
           $this->assertTrue(is_string($output), format_string('_theme() returns a string for data type !type.', array('!type' => $type)));
         }
     
    -    // suggestionnotimplemented is not an implemented theme hook so _theme()
    +    // suggestionnotimplemented is not an implemented theme hook so drupal_render()
         // should return FALSE instead of a string.
    -    $output = _theme(array('suggestionnotimplemented'));
    -    $this->assertIdentical($output, FALSE, '_theme() returns FALSE when a hook suggestion is not implemented.');
    +    $suggestionnotimplemented = array('#theme' => 'suggestionnotimplemented');
    +    $output = drupal_render($suggestionnotimplemented);
    +    $this->assertIdentical($output, FALSE, 'drupal_render() returns FALSE when a hook suggestion is not implemented.');
    

    I actually think we want to leave all this test coverage as is because it's _theme() that returns FALSE, drupal_render() by design always returns a string and we want to test the return value of _theme(). So I think here it makes sense to use the internal _theme() function in the tests :)

star-szr’s picture

And I think once the patch is scaled back (points 2 and 3 from my review).

+++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/ThemeTestController.php
@@ -55,7 +55,8 @@ public function testInfoStylesheets() {
-    return _theme('theme_test_template_test');
+    $theme_test_template_test = array('#theme' => 'theme_test_template_test');
+    return drupal_render($theme_test_template_test);

@@ -65,7 +66,8 @@ public function testTemplate() {
-    return _theme(array('theme_test__suggestion', 'theme_test'), array());
+    $theme_test__suggestion = array('#theme' => array('theme_test__suggestion', 'theme_test'));
+    return drupal_render($theme_test__suggestion);

Also, the docblocks will need to be updated a bit here but these should just return render arrays rather than calling drupal_render() since they are essentially page callbacks.

botanic_spark’s picture

@Cottser
Are you saying in comment #26 that we should return just

return array('#theme' => 'theme_test_template_test');

instead of

$theme_test_template_test = array('#theme' => 'theme_test_template_test');
return drupal_render($theme_test_template_test);
botanic_spark’s picture

Here is a re-rolled patch with the changes you noted in comment #25.

star-szr’s picture

Status: Needs work » Needs review

#27 - exactly! That applies to TwigThemeTestController::phpVariablesRender() as well.

star-szr’s picture

botanic_spark’s picture

Changes from comment #26

The last submitted patch, 28: refactor-theme-tests-2191135-28.patch, failed testing.

star-szr’s picture

Title: [meta] Refactor use of theme() in tests where possible » Refactor use of theme() in tests where possible
Status: Needs review » Needs work

Thanks @botanic_spark, I think this is looking really close! A lot of the items from the grep in the issue summary have already been fixed so the set of changes here ends up being pretty small. De-meta-ing.

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
    @@ -34,7 +34,7 @@ public static function getInfo() {
    -    theme_enable(array('test_theme'));
    +    \Drupal::service('theme_handler')->enable(array('test_theme'));
    

    Please leave this change for #2151469: Clean-up usage of deprecated list_themes() and _system_rebuild_theme_data() in favor of theme_handler service, it's out of scope here.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
    @@ -48,7 +48,8 @@ function testTwigDebugMarkup() {
    -    $output = _theme('node', node_view($node));
    +    $node_output = entity_view($node, 'full');
    +    $output = drupal_render($node_output);
    

    I misread this before. This is a good fix and we're still working with a render array which is cool. I would just go with $node_output = node_view($node); though - that defaults to the full view mode even :)

  3. +++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/ThemeTestController.php
    @@ -65,7 +67,9 @@ public function testTemplate() {
        *   An HTML string containing the themed output.
        */
       public function testSuggestion() {
    -    return _theme(array('theme_test__suggestion', 'theme_test'), array());
    +    return array(
    +      '#theme' => array('theme_test__suggestion', 'theme_test'),
    +    );
    

    The @return needs to be updated here, to @return array and something starting with "A render array..."

  4. +++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.php
    @@ -16,7 +16,8 @@ class TwigThemeTestController {
    -    return _theme('twig_theme_test_php_variables');
    +    $twig_theme_test_php_variables = array('#theme' => 'twig_theme_test_php_variables');
    +    return drupal_render($twig_theme_test_php_variables);
    

    Please change this menu/page callback to return an array as well like the other two that were updated in the most recent patch.

The last submitted patch, 31: refactor-theme-tests-2191135-31.patch, failed testing.

star-szr’s picture

Those exceptions are legitimate and are exposing a weakness in theme_test_theme(). hook_theme() states that either 'variables' or 'render element' needs to be defined:

Each information array must contain either a 'variables' element (for _theme() calls) or a 'render element' element (for render elements), but not both.

…and there a number of instances in theme_test_theme() that define neither. Let's just fix them all here.

botanic_spark’s picture

Re-rolled and applied changes from #33 and #35

botanic_spark’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: refactor-theme-tests-2191135-36.patch, failed testing.

star-szr’s picture

Looks like we're missing one more 'variables' definition somewhere, try running Drupal\system\Tests\Theme\ThemeTestTwig locally or start digging there anyway :)

sun’s picture

Hm. Pretty much all of these tests are now part of #2258173: [meta] Various web tests are not performing any HTTP requests

i.e., they should not be web tests in the first place.

#2257519: Move content assertion methods into a trait, so DUTB can consume it, too is about to add a render() method that will be exposed to both WebTestBase and DrupalUnitTestBase, which allows to call drupal_render() on an arbitrary render element array AND immediately use that as the "content" of the "internal browser" → assertRaw(), assertText(), etc.pp. will work natively, without having to perform any additional operations. :-)

Beyond that, I don't know whether we need additional helper methods, but perfectly possible + I'd love to discuss in:

#2227569: Add a helper method to WebTestBase for checking the output of render arrays and deprecate assertThemeOutput()

botanic_spark’s picture

Assigned: botanic_spark » Unassigned
star-szr’s picture

Title: Refactor use of theme() in tests where possible » Refactor use of _theme() in tests where possible

It sounds like this needs a new plan based on #40, or we need to accept that things won't be perfect and continue as originally planned.

andypost’s picture

Status: Needs work » Closed (cannot reproduce)
Related issues: +#2388247: Documentation refers to _theme() function, which has been removed

grepping shows that no more _theme() left in core, except few comments that should be cleaned-up in #2388247: Documentation refers to _theme() function, which has been removed