Problem/Motivation

As part of improving the theme system and making everything renderable in Drupal 8, we are removing all calls to theme() from core, and instead calling drupal_render() on structured data.

This particular issue is where we will post the patch to remove all calls to theme() from the simpletest module in core. This is one of the sub-issues from our meta: #2006152: [meta] Don't call theme() directly anywhere outside drupal_render()

In most core modules, we simply remove all calls to theme(), but in simpletest module we need to be more careful, because we do have tests that need to call theme() in order to test that the theme_hook_suggestions are working correctly.

Note that WebTestBase::assertThemeOutput() must work for both theme hook types, but is currently broken for theme hooks that work with a render element instead of variables. The patch in #47 proves and fixes this.

Proposed resolution

Remove all calls to theme() from simpletest module.

Remaining tasks

Remove all calls to theme() from simpletest module.

User interface changes

None.

API changes

None.

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

Files: 
CommentFileSizeAuthor
#47 simpletest-assertThemeOutput-2009670-47-without-fix.patch3.67 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 58,125 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#47 simpletest-assertThemeOutput-2009670-47-with-fix.patch4.72 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 58,087 pass(es).
[ View ]
#27 simpletest_replace_theme_with_drupal_render-2009670-27.patch1.05 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 56,551 pass(es).
[ View ]
#16 simpletest_replace_theme_with_drupal_render-2009670-16.patch6.31 KBhussainweb
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es).
[ View ]
#12 simpletest_replace_theme_with_drupal_render-2009670-12.patch6.31 KBJeroenT
PASSED: [[SimpleTest]]: [MySQL] 57,835 pass(es).
[ View ]
#10 simpletest_replace_theme_with_drupal_render-2009670-10.patch7.32 KBJeroenT
PASSED: [[SimpleTest]]: [MySQL] 58,753 pass(es).
[ View ]
#8 simpletest-replace_theme_with_drupal_render-2009670-8.patch7.37 KBJeroenT
FAILED: [[SimpleTest]]: [MySQL] 55,380 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#6 drupal_replace_theme_with_drupal_render_simpletest_module-2009670-6.patch7.48 KBesunger
PASSED: [[SimpleTest]]: [MySQL] 56,816 pass(es).
[ View ]
#3 drupal_replace_theme_with_drupal_render_simpletest_module-2009670-3.patch7.48 KBesunger
PASSED: [[SimpleTest]]: [MySQL] 56,066 pass(es).
[ View ]
#2 drupal_remove_theme_simpletest_module-2009670-2.patch6.51 KBesunger
FAILED: [[SimpleTest]]: [MySQL] 56,088 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

Comments

esunger’s picture

Assigned:Unassigned» esunger

WDG (Ukraine,Kharkov) want to implement this on CodeSprintUA.

esunger’s picture

Assigned:esunger» Unassigned
Status:Active» Needs review
StatusFileSize
new6.51 KB
FAILED: [[SimpleTest]]: [MySQL] 56,088 pass(es), 0 fail(s), and 4 exception(s).
[ View ]
esunger’s picture

StatusFileSize
new7.48 KB
PASSED: [[SimpleTest]]: [MySQL] 56,066 pass(es).
[ View ]

Updated assertThemeOutput function in /core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php.

esunger’s picture

Assigned:Unassigned» esunger
Issue tags:+CodeSprintUA
podarok’s picture

Status:Needs review» Needs work
+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -319,7 +319,11 @@ function _simpletest_batch_operation($test_list_init, $test_id, &$context) {
+   );   ¶
+  $context['message'] .= drupal_render($items_list);
...
+++ b/core/modules/simpletest/simpletest.theme.incundefined
@@ -27,13 +27,30 @@ function theme_simpletest_test_table($variables) {
+   );  ¶
+ ¶
   $js = array(
...
+++ b/core/modules/simpletest/simpletest.theme.incundefined
@@ -27,13 +27,30 @@ function theme_simpletest_test_table($variables) {
+ ¶
   // Cycle through each test group and create a row.
...
+++ b/core/modules/simpletest/simpletest.theme.incundefined
@@ -120,7 +137,13 @@ function theme_simpletest_test_table($variables) {
+      '#rows' => $rows,
+      '#attributes' => array('id' => 'simpletest-form-table'),     ¶
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
     // Initialize image mapping property.
+    ¶
+    $image_pass = array(
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
+    );
+    ¶
+    $image_fail = array(
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
+    );
+    ¶
+    $image_exception = array(
...
+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestResultsForm.phpundefined
@@ -47,31 +47,44 @@ public static function create(ContainerInterface $container) {
+    );
+    ¶
+    $image_debug = array(
...
+    );
+    ¶
     $this->statusImageMap = array(

https://drupal.org/coding-standards#indenting
whitespace errors

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -2577,7 +2577,12 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') {
+    $theme = array('#theme'=>$callback);

needs whitespaces around of
=>

esunger’s picture

Status:Needs work» Needs review
StatusFileSize
new7.48 KB
PASSED: [[SimpleTest]]: [MySQL] 56,816 pass(es).
[ View ]

Thanks, whitespace errors were solved.

podarok’s picture

Status:Needs review» Needs work

#6 not solved
try http://dgo.to/dreditor and look at Your patch
whitespace indenting not tabs
whitespace endings
still needs work

JeroenT’s picture

Status:Needs work» Needs review
StatusFileSize
new7.37 KB
FAILED: [[SimpleTest]]: [MySQL] 55,380 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

Solved whitespace issues in patch #6.

Status:Needs review» Needs work

The last submitted patch, simpletest-replace_theme_with_drupal_render-2009670-8.patch, failed testing.

JeroenT’s picture

Status:Needs work» Needs review
StatusFileSize
new7.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,753 pass(es).
[ View ]

Solved whitespace issues in patch #6.

thedavidmeister’s picture

Status:Needs review» Needs work
+      // Make sure that we are locked out of the installer when prefixing
+      // using the user-agent header. This is an important security check.
       global $base_url;
-      $this->drupalGet(url($base_url . '/core/install.php', array('external' => TRUE, 'absolute' => TRUE)));
-      $this->assertResponse(403, 'Cannot access install.php.');
+
+      $this->drupalGet($base_url . '/core/install.php', array('external' => TRUE));
+      $this->assertResponse(403, 'Cannot access install.php with a "simpletest" user-agent header.');

This has nothing to do with this issue. Remove it from the patch please.

+    $theme = array('#theme' => $callback);
+    foreach($variables as $key => $variable) {
+      $theme["#$key"] = $variable;
+    }
+    $output = drupal_render($theme);

This variable should be called $build, not $theme. Please see the issue summary of the parent issue for more details.

+  $items_list = array (
+    '#theme' => 'item_list',
+    '#items' => $items,
+   );
+  $context['message'] .= drupal_render($items_list);

This variable should be $item_list not $items_list.

+  $image_collapsed = array(
+    '#theme' => 'image',
+    '#uri' => 'core/misc/menu-collapsed.png',
+    '#width' => '7',
+    '#height' => '7',
+    '#alt' => t('Expand'),
+    '#title' => t('Expand'),
+  );
+  $image_extended = array(
+    '#theme' => 'image',
+    '#uri' => 'core/misc/menu-expanded.png',
+    '#width' => '7',
+    '#height' => '7',
+    '#alt' => t('Collapse'),
+    '#title' => t('Collapse'),
+   );
   $js = array(
     'images' => array(
-      theme('image', array('uri' => 'core/misc/menu-collapsed.png', 'width' => 7, 'height' => 7, 'alt' => t('Expand'), 'title' => t('Expand'))) . ' <a href="#" class="simpletest-collapse">(' . t('Expand') . ')</a>',
-      theme('image', array('uri' => 'core/misc/menu-expanded.png', 'width' => 7, 'height' => 7, 'alt' => t('Collapse'), 'title' => t('Collapse'))) . ' <a href="#" class="simpletest-collapse">(' . t('Collapse') . ')</a>',
+      drupal_render($image_collapsed) . ' <a href="#" class="simpletest-collapse">(' . t('Expand') . ')</a>',
+      drupal_render($image_extended) . ' <a href="#" class="simpletest-collapse">(' . t('Collapse') . ')</a>',

I do feel like the concatenation of the links to the rendered images could just be included as #suffix attributes on the image arrays themselves.

JeroenT’s picture

StatusFileSize
new6.31 KB
PASSED: [[SimpleTest]]: [MySQL] 57,835 pass(es).
[ View ]

Made changes as suggested by thedavidmeister.

I also changed the alt and title because i think this is wrong.

+  $image_collapsed = array(
+    '#theme' => 'image',
+    '#uri' => 'core/misc/menu-collapsed.png',
+    '#width' => '7',
+    '#height' => '7',
+    '#alt' => t('Expand'),
+    '#title' => t('Expand'),
+  );
+  $image_extended = array(
+    '#theme' => 'image',
+    '#uri' => 'core/misc/menu-expanded.png',
+    '#width' => '7',
+    '#height' => '7',
+    '#alt' => t('Collapse'),
+    '#title' => t('Collapse'),
+   );
JeroenT’s picture

Status:Needs work» Needs review
Issue tags:-Novice

Marking as needs review.

JeroenT’s picture

Issue tags:+Novice

accidentally deleted tag.

thedavidmeister’s picture

Status:Needs review» Needs work

The alt and title were correct originally. Please don't change functionality in a conversion patch like this, open a separate issue if you want to change things.

When something is currently "collapsed" you want to show the verb that will happen when you click it, which is "expand" not "collapse". And vice versa.

Regardless, need a separate issue for proposed changes.

hussainweb’s picture

Status:Needs work» Needs review
StatusFileSize
new6.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es).
[ View ]

Removing changes to 'alt' and 'title' and fixed minor whitespace issues. Also note that there is a possible error with the class name for the link in suffix:

+  $image_collapsed = array(
+    '#theme' => 'image',
...
+    '#suffix' => '<a href="#" class="simpletest-collapse">(' . t('Expand') . ')</a>',
+  );
+  $image_extended = array(
+    '#theme' => 'image',
...
+    '#suffix' => ' <a href="#" class="simpletest-collapse">(' . t('Collapse') . ')</a>',
+   );

I have left it like this for now.

Status:Needs review» Needs work

The last submitted patch, simpletest_replace_theme_with_drupal_render-2009670-16.patch, failed testing.

thedavidmeister’s picture

Status:Needs work» Needs review

#16 - the class name does look like an error, why not open a followup issue for it?

hussainweb’s picture

Status:Needs review» Needs work

The last submitted patch, simpletest_replace_theme_with_drupal_render-2009670-16.patch, failed testing.

jenlampton’s picture

Status:Needs work» Needs review
Issue tags:+Novice, +CodeSprintUA
hussainweb’s picture

Looks like all the tests have passed on the patch in #16.

heddn’s picture

Status:Needs review» Reviewed & tested by the community

#16 looks good to go. #2021551: Possibly confusing class name in simpletest.pages.inc is opened to fix the CSS class name.

YesCT’s picture

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 6038891 and pushed to 8.x. Thanks!

Eric_A’s picture

Status:Fixed» Active
diff --git a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
index 8c46872..6ba54b4 100644
--- a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -2639,7 +2639,11 @@ protected function assertNoTitle($title, $message = '', $group = 'Other') {
    *   TRUE on pass, FALSE on fail.
    */
   protected function assertThemeOutput($callback, array $variables = array(), $expected, $message = '', $group = 'Other') {
-    $output = theme($callback, $variables);
+    $build = array('#theme' => $callback);
+    foreach($variables as $key => $variable) {
+      $build["#$key"] = $variable;
+    }
+    $output = drupal_render($build);
     $this->verbose('Variables:' . '<pre>' .  check_plain(var_export($variables, TRUE)) . '</pre>'
       . '<hr />' . 'Result:' . '<pre>' .  check_plain(var_export($output, TRUE)) . '</pre>'
       . '<hr />' . 'Expected:' . '<pre>' .  check_plain(var_export($expected, TRUE)) . '</pre>'

assertThemeOutput() was designed for modules to prevent regressions in their theme hook implementations. It takes just a theme callback en variables. This hunk should be reverted.

Eric_A’s picture

Status:Active» Needs review
StatusFileSize
new1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 56,551 pass(es).
[ View ]
Eric_A’s picture

Category:task» bug
Priority:Normal» Critical

The breaking of test coverage cannot be a normal task...

jenlampton’s picture

Category:bug» task
Priority:Critical» Normal
Status:Needs review» Fixed
Issue tags:-CodeSprintUA

@Eric_A The change above should not break tests. Are you sure it does?

Calling drupal_render() on a render array with "#theme' => 'callback' should produce exactly the same output as calling theme('callback'). Since this test only needs the $output from that call, there should be no regression here.

Additionally, since there will be no calls to theme() anywhere else in core, this test *should* test the output in the same way it will be generated, which is via drupal_render().

Changing issue status back.

If there is actually a problem with this test, can you create a follow-up issue that references this one?

Eric_A’s picture

Category:task» bug
Priority:Normal» Critical
Status:Fixed» Needs review

Calling drupal_render() on a render array with "#theme' => 'callback' should produce exactly the same output as calling theme('callback').

Agreed that it *should* produce the exact same output, but this is not always the case (See #2041283: theme_status_report() is severely broken). Anyway, this is a low level method that was introduced to test theme function output directly, not the drupal_render() route. See #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output. If we had an assertThemeOutput() for theme_status_report() it would have failed with the patch that was committed here. So yes, the drupal_render() implementation is useful, it may make loads of sense to create a method for testing the output returned from the drupal_render() route, but that one should get a name different from assertThemeOutput() and we should not just throw away the low level testing in a completely different issue.

Fixing this regression here as a quick follow-up does look like the right thing to do.

Fabianx’s picture

Hm, if drupal_render() != theme() that is a bug in itself. So I don't see the need for it that core calls a soon-to-be-deprecated function.

Eric_A’s picture

Hm, if drupal_render() != theme() that is a bug in itself. So I don't see the need for it that core calls a soon-to-be-deprecated function.

I don't think the theme() system is soon-to-be-deprecated at all. Rather, soon-to-be-considered-internal.
Yes, the idea is to use the higher level render API instead of calling theme() directly. But the theme() system will still be around, theme hook implementations will still need to be provided by modules, and there should still be a way to *test* these components on their own, by directly calling these functions. So yes, I'm saying drupal_render() != theme() and I don't consider that a bug at all.

Fabianx’s picture

Well, from the function definition of drupal_render => drupal_render with appropriate input is always equal to theme and anything else is a bug.

And if we deprecate theme() as public function, which we do, then Drupal internally would call a deprecated public function, which feels strange, but should be okay.

But I don't have strong feelings about this here and I am fine with either way and let the core committers to decide.

catch’s picture

If we had an assertThemeOutput() for theme_status_report() it would have failed with the patch that was committed here.

Wouldn't that be a good thing?

Eric_A’s picture

It is true that a drupal_render() version of this test helper can uncover bugs in theme hook implementations that take a render element. But theoretically it could go another way. A bug in drupal_render() could make a test pass or fail when it shouldn't. We're adding more and more unit testing. Surely we can't do this at the same time? At least not hidden in this issue.

Expanding the original test helper by adding a #theme property when the hook needs a render element gets us the same coverage, but without the dependency on drupal_render(). I'd be happy to do that.

Eric_A’s picture

When you look at the two tests that currently call assertThemeOutput() there's more evidence that we've broken the contract:

\Drupal\system\Tests\Theme\FunctionsTest::testItemList() - "Tests theme_item_list()."
\Drupal\system\Tests\Theme\FunctionsTest::testLinks() - "Tests theme_links()."

I just learned that we have \Drupal\system\Tests\Common\RenderTest which extends \Drupal\simpletest\WebTestBase. We could override WebTestBase::assertThemeOutput() in RenderTest (or whichever test class makes the most sense). So we'd be restoring the original contracts and have a drupal_render() version for tests that want to "Assert basic render element output" rather than "Assert themed output".

eromero1’s picture

Status:Needs review» Reviewed & tested by the community

The patch was installed and the testing module ran swimmingly. Everything is looking dandy. GO TEAM DRUPAL!!!!

catch’s picture

Status:Reviewed & tested by the community» Needs review

Still needs review.

Eric_A’s picture

Assigned:esunger» Unassigned

Still needs review.

Could this perhaps be assigned to @tim.plunkett or @effulgentsia? @tim.plunkett did #1706878: Add WebTestBase::assertThemeOutput() to allow modules to test theme function output and @effulgentsia maintains both Theme system and Render system.

Fabianx’s picture

Issue tags:-Novice, -RTBC July 1

We will be deprecating theme() as it is - that has been decided now on twig call. The only way to get output from theme() will be drupal_render() going forward (from D9 then). This solves a lot of major tasks for the theme system cleanup and drillability and how to make #type work.

There will be a theme_info() call and theme() will just be a wrapper around it probably that calls the #render_function gotten from theme_info() with the #render_variables. (Issue link is coming as soon as I have written this up).

I am fine to change this back and forth, but in the end this will be changed anyway.

Unit Testing is great - yes. The question is: Where is the "border" of calling internal APIs?

Maybe another option is to change the function name of themeTest to renderTest to make clear it is testing drupal_render() and not theme() - soon to be deprecated?

Thanks!

( Removed tags on purpose as they don't apply to the current patch and this is not Novice. )

Eric_A’s picture

Why would deprecation of theme() in itself mean that we no longer care about (unit) testing module provided parts in a low-level output generating subsystem? If we cannot use theme() anymore, then we'll use what comes closest. Which can never be any higher level Render API. If we want to remove this level of testing, that's a different issue. (It's not a conversion issue, that's for sure.)
When testing the output of a template and its (pre)processors we really don't care much what clients will be using all of this. The notion of Drupal render elements and #type properties are irrelevant here.

Right now assertThemeOutput() has become incomprehensible for anyone who knows what it was about, reads the current doxygen and then takes a look at the current implementation.

/**
* Asserts themed output.
*
* @param $callback
*   The name of the theme function to invoke; e.g. 'links' for theme_links().
* @param $variables
*   An array of variables to pass to the theme function.
* @param $expected
*   The expected themed output string.
webchick’s picture

Status:Needs review» Postponed (maintainer needs more info)
Issue tags:+Needs issue summary update

Hm. I don't fully grok what the problem is here, and it seems like there is disagreement within the Twig team about whether or not it actually is a problem.

If something needs to get rolled back, let's get agreement on this and an updated issue summary that explains what's going on to core committers.

jenlampton’s picture

Status:Postponed (maintainer needs more info)» Needs review

Changing status back to CNR

jenlampton’s picture

Issue summary:View changes

actual summary

jenlampton’s picture

Assigned:Unassigned» jenlampton

doing my homework.

jenlampton’s picture

Status:Needs review» Needs work
Issue tags:-Needs issue summary update+Needs tests

Okie dokie, I just re-read everything and updated the issue summary. And here's what I think we should do:

1) Use the patch in #27 since that fixes broken tests, which we introduced in the initial commit.

2) We should add a test that will fail appropriately without the patch in #27, as per #34.

3) We will be deprecating theme() - or making it internal - or whatever - later on. Let's postpone the rest of the debate (including calling only drupal_render() in tests, renaming themeTest to renderTest, etc) until then, and at least get things back in a working order right now.

Eric_A’s picture

Another regression: assertThemeOutput() no longer works when passing a renderable array as the only argument. The current implementation just expects that it is passed an associative array of variables, which I believe is an unneeded assumption. The assertThemeOutput() method "Asserts themed output", so a full functional mapping to theme() is what really helps client code.

1) Use the patch in #27 since that fixes broken tests, which we introduced in the initial commit.

2) We should add a test that will fail appropriately without the patch in #27, as per #34.

3) We will be deprecating theme() - or making it internal - or whatever - later on. Let's postpone the rest of the debate (including calling only drupal_render() in tests, renaming themeTest to renderTest, etc) until then, and at least get things back in a working order right now.

@jenlampton, are you working on this? Otherwise, I might give it a shot.

Eric_A’s picture

Status:Needs work» Needs review
StatusFileSize
new4.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,087 pass(es).
[ View ]
new3.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,125 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

I went looking for existing assertions involving renderable array theme hooks and found a few in \Drupal\system\Tests\System\ThemeTest. I converted all assertions to assertThemeOutput() and expect the ones involving renderable array theme hooks to fail without the fix from #27.

Eric_A’s picture

Issue tags:-Needs tests

So the testbot revealed yet another issue: theme() and the fixed version of assertThemeOutput() return the same value if the passed $callback is not implemented. (Currently this is the boolean FALSE.) The current version of assertThemeOutput() returns what drupal_render() returns. (Currently the empty string.)

Eric_A’s picture

So the testbot revealed yet another issue: theme() and the fixed version of assertThemeOutput() return the same value if the passed $callback is not implemented. (Currently this is the boolean FALSE.) The current version of assertThemeOutput() returns what drupal_render() returns. (Currently the empty string.)

jenlampton’s picture

Assigned:jenlampton» Unassigned

Yes, I was working on these tests. That's why I assigned it to myself ;)

I decided to use assertIdentical() instead of assertThemeOutput() to confirm that the output from theme() and drupl_render() are identical. Unfortunately, the output was identical. Even without this patch.

Since your tests catch the bug you were referring to (and mine don't) I'll leave this to you.

jenlampton’s picture

Issue summary:View changes

update

Eric_A’s picture

Issue summary:View changes

WebTestBase::assertThemeOutput() currently broken for theme hooks that work with a render element instead of variables.

Eric_A’s picture

Added the most pressing part of the current state to the issue summary.

Note that WebTestBase::assertThemeOutput() must work for both theme hook types, but is currently broken for theme hooks that work with a render element instead of variables. The patch in #47 proves and fixes this.

catch’s picture

Status:Needs review» Reviewed & tested by the community

Test looks fine. Let's go back to how it was for now and revisit later.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed cc1f3fd and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

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

Anonymous’s picture

Issue summary:View changes

Indicated one of the differences with the earlier patch.