CommentFileSizeAuthor
#85 system-replace-theme-2006152-85.patch8.12 KBpplantinga
#85 interdiff.txt2.9 KBpplantinga
#83 system-replace-theme-2006152-83.patch12.82 KBpplantinga
#83 patch-diff.txt1.33 KBpplantinga
#81 system-replace-theme-2006152-81.patch14.1 KBpplantinga
#81 interdiff.txt561 bytespplantinga
#77 system-replace-theme-2006152-77.patch14.08 KBpplantinga
#73 system-replace-theme-2006152-73.patch14.05 KBdmitrii
#73 interdiff.txt663 bytesdmitrii
#71 system-replace-theme-2006152-71.patch14.08 KBpplantinga
#71 interdiff.txt869 bytespplantinga
#69 system-replace-theme-2006152-69.patch14.08 KBpplantinga
#69 interdiff.txt887 bytespplantinga
#67 system-replace-theme-2006152-67.patch13.95 KBpplantinga
#64 system-replace-theme-2006152-64.patch14.75 KBpplantinga
#60 system-replace-theme-2006152-60.patch15.59 KBpplantinga
#60 interdiff.txt5.4 KBpplantinga
#56 system-replace-theme-2006152-56.patch20.99 KBpplantinga
#56 interdiff.txt3.53 KBpplantinga
#46 system-replace-theme-2006152-46.patch23.92 KBpplantinga
#44 testAttributeMerging-drupal_render.patch974 bytespplantinga
#44 system-replace-theme-2006152-44.patch26.7 KBpplantinga
#40 system-replace-theme-2006152-40.patch25.79 KBpplantinga
#38 system-replace-theme-2006152-38.patch25.21 KBpplantinga
#32 system-replace-theme-2006152-32.patch25.86 KBpplantinga
#29 system-replace-theme-2006152-29.patch25.78 KBpplantinga
#22 system-replace-theme-2006152-22.patch27.19 KBpplantinga
#20 system-replace-theme-2006152-19.patch27.19 KBpplantinga
#14 system-replace-theme-2006152-13.patch21.09 KBpplantinga
#5 replace-theme-with-render-2009674-5.patch15.71 KBSamvel
#2 replace-theme-with-render-2009674-2.patch16.71 KBSamvel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Samvel’s picture

Assigned: Unassigned » Samvel
Samvel’s picture

Status: Active » Needs review
FileSize
16.71 KB

attached patch

andypost’s picture

Issue tags: +CodeSprintUA

I suggested to separate this conversion
Filed separate issue #2010086: Replace theme() with drupal_render() in system module for theme_system_themes_page() for the theme_system_themes_page() that needs more work

andypost’s picture

Issue summary: View changes

related

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009674-2.patch, failed testing.

Samvel’s picture

Status: Needs work » Needs review
FileSize
15.71 KB

attached new one

And created separate task for system_status() #2010982: Replace theme() with drupal_render() in system module for system_status() Assigned to: somepal

Samvel’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-render-2009674-5.patch, failed testing.

andypost’s picture

Awesome patch!
I'd like to point that better to make none-theme_* functions (mostly callbacks) to return render array
But usage in theme_* functions is right!

+++ b/core/modules/system/system.admin.incundefined
@@ -99,7 +105,7 @@ function system_admin_index() {
-  $menu_items = array();
+  $system_admin_index = array('#theme' => 'system_admin_index');

@@ -114,10 +120,13 @@ function system_admin_index() {
-      $menu_items[$info->info['name']] = array($info->info['description'], $admin_tasks);
+      $system_admin_index['#menu_items'][$info->info['name']] = array(
+        $info->info['description'],
+        $admin_tasks,
...
-  return theme('system_admin_index', array('menu_items' => $menu_items));
+  return drupal_render($system_admin_index);

Better provide a $build array with $build['#theme'] = 'system_admin_index';

+++ b/core/modules/system/system.admin.incundefined
@@ -129,7 +138,7 @@ function system_themes_page() {
-  $theme_groups  = array();
+  $system_themes_page = array('#theme' => 'system_themes_page');

@@ -232,22 +241,26 @@ function system_themes_page() {
-  return theme('system_themes_page', array('theme_groups' => $theme_groups, 'theme_group_titles' => $theme_group_titles)) . drupal_render($admin_form);
+  return drupal_render($system_themes_page) . drupal_render($admin_form);

Same here! just convert this to render array

Also I see no reason to rename local variable.

+++ b/core/modules/system/system.admin.incundefined
@@ -1636,7 +1656,8 @@ function theme_system_modules_details($variables) {
-  return theme('table', array('header' => $form['#header'], 'rows' => $rows));
+  $table = array('#theme' => 'table', '#header' => $form['#header'], '#rows' => $rows);
+  return drupal_render($table);

this exactly should be renderable too.

+++ b/core/modules/system/system.admin.incundefined
@@ -1855,10 +1879,10 @@ function system_date_format_language_overview_page() {
-  return theme('table', array('header' => $header, 'rows' => $rows));
+  return drupal_render($table);

same here

+++ b/core/modules/system/system.installundefined
@@ -105,7 +105,7 @@ function system_requirements($phase) {
-  $missing_extensions = array();
+  $item_list = array('#theme' => 'item_list');

same

andypost’s picture

andypost’s picture

Issue summary: View changes

append separated task

somepal’s picture

@andypost what about other calls to theme() in system module like one I spotted in theme.api.php -

$variables[$variable_name] = theme('rdf_template_variable_wrapper', array('content' => $variables[$variable_name], 'attributes' => $attributes, 'context' => $context));

Do we create separate issue for it. most of them are in Test related files.

somepal’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

yes, please fine new issue! nice piece of ugly code....

andypost’s picture

@Joe9 I think theme.api should be separate issue with review of @jhodgdon to be commited

pplantinga’s picture

Is it okay for me to take a look at this? @Samvel hasn't posted in a month.

thedavidmeister’s picture

Assigned: Samvel » Unassigned

go for it.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
21.09 KB

Cool. Here's a patch. I don't think it'll pass all tests, but it's worth a shot.

Also, just checking: we're ignoring theme('table') and theme(array()) right?

thedavidmeister’s picture

why are we ignoring some theme() calls?

Status: Needs review » Needs work

The last submitted patch, system-replace-theme-2006152-13.patch, failed testing.

pplantinga’s picture

Well theme(table) is eventually going to be converted to '#type' => 'table' as per #1938930: Convert theme_system_modules_details() to #type table

Looking again at theme(array()) it looks like the arrays just mimic default behavior, so i guess using '#theme' => first_arg__suggestion should work, yes? I'll update the patch (plus fix errors).

thedavidmeister’s picture

array('X', 'Y', 'Z') is not the same as X__Y__Z for theme hooks. Just use '#theme' => array('X', 'Y', Z') and it will work.

Yes, there are a lot of #theme style render arrays that may be converted to #type later, but that whole conversion is basically a follow-up to this conversion as it doesn't even make sense until we're using drupal_render() for rendering those tables.

Step 1. Get everything through drupal_render()

Step 2. Discuss what should be #theme based and what should be #type based :)

pplantinga’s picture

Status: Needs work » Needs review

Here's another stab at it, this time with all instances of theme() changed to drupal_render().

In order to get it to pass tests, I had to change system_theme(). Does that need to be filed as a separate issue?

pplantinga’s picture

Navigating drupal with only a keyboard is a bit difficult, had trouble attaching the patch

Status: Needs review » Needs work

The last submitted patch, system-replace-theme-2006152-19.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
27.19 KB

Gah syntax error... Okay here we go.

Status: Needs review » Needs work

The last submitted patch, system-replace-theme-2006152-22.patch, failed testing.

pplantinga’s picture

This is proving quite difficult.

One slew of errors is caused by converting

theme('status_report', array('requirements' => $requirements));

to

$status_report = array(
  '#theme' => 'status_report',
  '#requirements' => $requirements,
);
drupal_render($status_report);

Any idea why that might be?

Still trying to track down the others.

thedavidmeister’s picture

pplantinga’s picture

Status: Needs work » Postponed

I guess this issue should wait til that one is done then....

Eric_A’s picture

Status: Postponed » Needs work
Eric_A’s picture

Status: Needs work » Postponed
pplantinga’s picture

Status: Postponed » Needs review
FileSize
25.78 KB

here's another patch that hopefully fixes all errors. It makes the same change as #2041283: theme_status_report() is severely broken

Status: Needs review » Needs work

The last submitted patch, system-replace-theme-2006152-29.patch, failed testing.

Eric_A’s picture

#2041283: theme_status_report() is severely broken is in, thereby resolving #2030805: Make theme_status_report compatible with drupal_render() and automatically reviving the old patches.

I did a quick, superficial review and noticed two things.
There are calls to drupal_render() where it is not passed a variable.
There are places where elements are rendered immediately: ['#markup'] .= drupal_render( and '#children' => drupal_render(. This is the safest choice when converting #markup and #children, but in #2031301: Replace theme_more_link() and replace with #type 'more_link' @Fabianx and @catch requested to do lazy rendering and keeping the structure intact for as long as possible. I'm assuming they'll want the same thing here where possible. (Meaning that in some cases we might not want to break existing exposed structures, but even if there is API breakage, such a better conversion might still be on the table.)

pplantinga’s picture

Status: Needs work » Needs review
FileSize
25.86 KB

Another hopefully clean patch, except for one error that I need some input on. This code:

$theme_test_render_element = array(
  '#theme' => 'theme_test_render_element',
  '#elements' => array('#attributes' => array('data-foo' => 'bar')),
  '#attributes' => array('id' => 'bazinga'),
);
drupal_render($theme_test_render_element);

ignores the '#elements' attribute, where theme() doesn't.

This patch also incorporates changes requested in #31, except for:

There are calls to drupal_render() where it is not passed a variable.

which I couldn't find. Perhaps you were looking at a line from the comments?

Status: Needs review » Needs work

The last submitted patch, system-replace-theme-2006152-32.patch, failed testing.

Eric_A’s picture

Perhaps you were looking at a line from the comments?

In the patch from #29 you had those in TwigDebugMarkupTest. Fixed in the patch from #32.

except for one error that I need some input on

theme_test_render_element() does not take variables, it takes a render element as its only argument, just like theme_status_report() did before. https://api.drupal.org/api/drupal/core%21modules%21system%21tests%21modu...
So when converting this:

theme('theme_test_render_element', array(
  'elements' => array(
    '#attributes' => array('data-foo' => 'bar'),
  ),
  'attributes' => array(
    'id' => 'bazinga',
  ),
))

your render element should look like this:

array(
  '#theme' => 'theme_test_render_element',
  'elements' => array(
     '#attributes' => array('data-foo' => 'bar'),
  ),
  'attributes' => array(
    'id' => 'bazinga',
  ),
)
Eric_A’s picture

In the previous comment I forgot to add the theme callback as #theme property. Edited that.

Eric_A’s picture

Note that core has tests around that are actually testing theme hook implementation output or perhaps the theme() function itself, which we still have to do even if we make it an internal function. So it's possible that some of these tests in fact should not be converted. I haven't looked into that here, now. See also my comment regarding WebTestBase::assertThemeOutput() in #2009670: Replace theme() with drupal_render() in simpletest module.

pplantinga’s picture

I tried the code from #34 and still failed. The output of that code is

<div data-variables-are-preprocessed></div>

So now we're missing both attributes.

I also tried putting it all into a giant array like this

$theme_test_render_element = array(
  '#theme' => 'theme_test_render_element',
  array(
    'elements' => array(
      '#attributes' => array('data-foo' => 'bar'),
    ),
    'attributes' => array(
      'id' => 'bazinga'
    ),
  ),
);

with no success.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
25.21 KB

Well if we assume that testAttributeMerging is testing theme() then this patch should cover it. Otherwise someone who knows more about render elements will have to update this test.

Status: Needs review » Needs work

The last submitted patch, system-replace-theme-2006152-38.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
25.79 KB

Gah! These missing 'variables' arrays have no end.

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

The last submitted patch, system-replace-theme-2006152-40.patch, failed testing.

pplantinga’s picture

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

Issue tags: -Novice

From core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php

<?php
/**
* @file
* Definition of Drupal\system\Tests\Theme\ThemeTest.
*/
namespace Drupal\system\Tests\Theme;
use Drupal\simpletest\WebTestBase;
use Drupal\test_theme\ThemeClass;
/**
* Tests low-level theme functions.
*/
class ThemeTest extends WebTestBase { 

I'm fairly sure that more methods from this class should should be left alone in this issue... I wonder if we should split off test classes in all these conversion issues. Those are just a different kind of animal.
Removing the "Novice" tag for now.

I tried the code from #34 and still failed.

I would not be surprised if you have uncovered a bug around theme_test_render_element(). It would have been helpfull if you had just thrown the patch to the testbot here before removing the conversion of ThemeTest::testAttributeMerging(), but anyway, I'm pretty sure we should not convert ThemeTest::testAttributeMerging().

pplantinga’s picture

Here's a test patch with the code from #34 to verify that it fails.

Also a patch with updated documentation.

thedavidmeister’s picture

Status: Needs review » Needs work
-    $this->assertIdentical(theme('theme_test_foo', array('foo' => 'a')), 'a', 'The theme registry contains theme_test_foo.');
+    $theme_test_foo = array('#theme' => 'theme_test_foo', '#foo' => 'a');
+    $this->assertIdentical(drupal_render($theme_test_foo), 'a', 'The theme registry contains theme_test_foo.');
 
     module_disable(array('theme_test'), FALSE);
     // After enabling/disabling a module during a test, we need to rebuild the
-    // container and ensure the extension handler is loaded, otherwise theme()
-    // throws an exception.
+    // container and ensure the extension handler is loaded, otherwise
+    // drupal_render() throws an exception.
     $this->rebuildContainer();
     $this->container->get('module_handler')->loadAll();
-    $this->assertIdentical(theme('theme_test_foo', array('foo' => 'b')), FALSE, 'The theme registry does not contain theme_test_foo, because the module is disabled.');
+    $theme_test_foo = array('#theme' => 'theme_test_foo', '#foo' => 'b');
+    $this->assertIdentical(drupal_render($theme_test_foo), '', 'The theme registry does not contain theme_test_foo, because the module is disabled.');
 
     module_enable(array('theme_test'), FALSE);
     // After enabling/disabling a module during a test, we need to rebuild the
-    // container and ensure the extension handler is loaded, otherwise theme()
-    // throws an exception.
+    // container and ensure the extension handler is loaded, otherwise
+    // drupal_render() throws an exception.
     $this->rebuildContainer();
     $this->container->get('module_handler')->loadAll();
-    $this->assertIdentical(theme('theme_test_foo', array('foo' => 'c')), 'c', 'The theme registry contains theme_test_foo again after re-enabling the module.');
+    $theme_test_foo = array('#theme' => 'theme_test_foo', '#foo' => 'c');
+    $this->assertIdentical(drupal_render($theme_test_foo), 'c', 'The theme registry contains theme_test_foo again after re-enabling the module.');

This stuff shouldn't be converted to drupal_render() as it's attempting to test the theme registry and you can't directly see the theme registry from drupal_render(). The second point in particular, you have no idea if drupal_render() *why* drupal_render() is returning an empty string - one explanation is that the theme registry did not contain theme_test_foo, but there are potentially others. Seeing FALSE returned by theme() can *only* happen if the hook is not in the registry, so it's a better test to leave this as theme(). See #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched.

Similarly, the other two tests are testing the way that theme works, not that the output of drupal_render() is what is expected (that's what RenderTest is for). These tests could be more precise after #1886448: Rewrite the theme registry into a proper service, but for now we should just leave them alone.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
23.92 KB

That makes sense. Here's an updated patch (plus re-roll).

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

However, if more work does need to be done here, for a patch this size please provide interdiffs with each re-roll as 24k is tedious to manually review from start to finish every time. https://drupal.org/documentation/git/interdiff

Eric_A’s picture

Status: Reviewed & tested by the community » Needs work
diff --git a/core/modules/system/theme.api.php b/core/modules/system/theme.api.php
index aedc6b0..2dd2e92 100644
--- a/core/modules/system/theme.api.php
+++ b/core/modules/system/theme.api.php
@@ -9,14 +9,20 @@
  * layer. Each theme can take control over most of Drupal's output, and
  * has complete control over the CSS.
  *
- * Inside Drupal, the theme layer is utilized by the use of the theme()
- * function, which is passed the name of a component (the theme hook)
- * and an array of variables. For example,
- * theme('table', array('header' => $header, 'rows' => $rows));
- * Additionally, the theme() function can take an array of theme
+ * Inside Drupal, the theme layer is utilized by the use of the drupal_render()
+ * function, which is passed a Drupal render array. For example,
+ * @code
+ * $table = array(
+ *   '#theme' => 'table',
+ *   '#header' => $header,
+ *   '#rows' => $rows,
+ * );
+ * drupal_render($table);
+ * @endcode
+ * Additionally, the '#theme' property can take an array of theme
  * hooks, which can be used to provide 'fallback' implementations to
- * allow for more specific control of output. For example, the function:
- * theme(array('table__foo', 'table'), $variables) would look to see if
+ * allow for more specific control of output. For example, the property:
+ * '#theme' => array('table__foo', 'table'), would look to see if
  * 'table__foo' is registered anywhere; if it is not, it would 'fall back'
  * to the generic 'table' implementation. This can be used to attach specific
  * theme functions to named objects, allowing the themer more control over

Changing the theme layer API itself is not the point here at all. This should go away.

Also, there are still tests converted here that are testing the theme system itself. In that particular case you *really* don't want to involve drupal_render(). Already a few from \Drupal\system\Tests\Theme\ThemeTest were removed along the way, but it's not finished yet. If you still think some of the low-level theme function tests from \Drupal\system\Tests\Theme\ThemeTest must be converted, let's discuss those explicitely.

thedavidmeister’s picture

Changing the theme layer API itself is not the point here at all. This should go away.

Fair enough, we could move/discuss these changes further in #2031319: The documentation for theme() should be clearer about saying not to call it directly. and remove them from this patch. I'm easy either way.

If you still think some of the low-level theme function tests from \Drupal\system\Tests\Theme\ThemeTest must be converted, let's discuss those explicitely.

kk. Let's review the tests:

testThemeTableStickyHeaders() / testThemeTableNoStickyHeaders() - fine to convert to drupal_render() as we're testing the markup produced, not theme() internals. This test is about how our tables look, not how our theme() function works. I'd even be ok to convert this to a #type 'table' array but that's out of scope here. We could move this to RenderTest though.

testThemeTableWithEmptyMessage() - same deal as testThemeTableNoStickyHeaders(). We're testing rendered markup as it would be generated "in the wild" so drupal_render() is the right choice. Could also live in RenderTest.

testThemeTableWithNoStriping() - same again, testing rendered markup, not theme() internals. We want to simulate how people will be using this in practise, so drupal_render() is the way to go.

testDrupalRenderChildren() - This test is checking that when a theme function as #theme or #theme_wrappers calls drupal_render() on the element being themed we don't get infinite recursion. Technically it should be irrelevant whether theme() or drupal_render() is called first, as it's an infinite loop cycling between these two functions anyway, but we do have to be sure that the conversion is done correctly so that the loop is "kicked off" after the conversion. A very easy way to check is to remove all references to #render_children in drupal_render() and see if your test fails, if the test doesn't fail when #render_children is removed we're testing the wrong thing and our conversion is flawed. I'd be happy to either wait for a patch with a failing test demonstrating the conversion preserves the functionality of the test, or remove this conversion from the patch.

testTwigDebugMarkup() - this test is checking that node_view() (which produces a renderable array) produces some expected markup when rendered. I'd say that it's an improvement to use drupal_render() for this test and that using theme() is actually incorrect here.

Eric_A’s picture

Ideally we'd have lots of tests for both high-level render functions/ render element output and low-level theme functions/ theme hook output. But:
-expanding RenderTest is not a requirement to get this conversion in.
-improving the tests for low-level theme functions is not a requirement to get this conversion in.

Right now I agree with some bits of the analysis by @thedavidmeister and disagree with other parts.
Can we get this conversion in now without trying to convert \Drupal\system\Tests\Theme\ThemeTest and open a separate issue to discuss further and improve on test coverage for the the low-level theme functions?

thedavidmeister’s picture

ok, so to be clear:

you're happy with changes to /core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php and /core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php

you just want the changes in /core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php to be removed.

is that right?

Eric_A’s picture

you're happy with changes to /core/modules/system/lib/Drupal/system/Tests/Theme/TableTest.php and /core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php

Sorry, no I'm not happy with adding a dependency on drupal_render() to these theme layer specific tests. I'd prefer the opposite. I'd rather look into turning them into even more of unit tests then adding more dependencies. But either way, that's not for this conversion issue.
I agree with not calling theme() directly from normal code, but I see no reason to stop trying to test the theme layer and its hook implementations. In fact, ideally every Twig conversion issue would have added a theme hook output test to make sure nothing was changed unintentionally. Twig is about just the theme layer. Not about drupal_render().

/**
 * Unit tests for theme_table().
 */
class TableTest extends WebTestBase {
  public static function getInfo() {
    return array(
      'name' => 'Theme Table',
      'description' => 'Tests built-in theme functions.',
      'group' => 'Theme',
    );
  }

If something fails here, you'll know you can find your solution somewhere inside the theme layer. If someone makes a mistake when improving PHPTemplate based logic or Twig based logic one of these tests will fail and you will know where to look. You don't want to get drupal_render() into the mix here, because this is a theme layer specific test.

/**
 * Tests for Twig debug markup.
 */
class TwigDebugMarkupTest extends WebTestBase {

  /**
   * Modules to enable.
   *
   * @var array
   */
  public static $modules = array('theme_test');

  public static function getInfo() {
    return array(
      'name' => 'Twig debug markup',
      'description' => 'Tests Twig debug markup.',
      'group' => 'Theme',
    );
  }

Again, this is about Twig and the theme layer. No need whatsoever to introduce drupal_render() into tests that are bound by the theme layer.

thedavidmeister’s picture

Sorry, no I'm not happy with adding a dependency on drupal_render() to these theme layer specific tests.

It did sound to me like you were specifically talking about one of the test files and not the other two.

Anyway, TableTest is testing rendered markup for tables. It's not a "low level" theme engine test. Core is moving towards #type 'table' in drupal_render() for all tables so that's probably what the tests should use to create the markup being checked for sticky headers or zebra striping. Using theme() here could hide problems. We don't need layers of tests all checking sticky headers in #type 'table', #theme 'table', theme('table'), theme_table() - we just need one test that is giving us table markup in the same way that you'd expect to build the markup while building a site following current "best practise".

TwigDebugMarkupTest is just rendering a node entity using node_view() and looking at the rendered output. The test doesn't and shouldn't care how the rendered output was generated, other than the rendering should be done correctly as you would if you were generating that output while building a site.

Docs for node_view() - https://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fu...

Description:

Generates an array for rendering the given node.

Return value:

An array as expected by drupal_render().

So, it makes perfect sense to me that if we're simulating a "wild" rendering of a node we want to use drupal_render().

Eric_A’s picture

It did sound to me like you were specifically talking about one of the test files and not the other two.

True, my mistake. I forgot one moment about the exact location of all the tests for this particular issue, thinking that here it was all in \Drupal\system\Tests\Theme\ThemeTest. (Perhaps it would have been easier if the issues had worked on theme hooks rather than modules?)

TwigDebugMarkupTest is just rendering a node entity using node_view()

This is an implementation detail. What matters are signatures, interfaces, and doxygen. And I already quoted those in my previous and earlier comments.

No matter how many (new) render layers core will use, we'll need theme layer tests as long as there is a theme layer. Adding higher level render tests is great. Add them to RenderTest and the like. Just don't make the (unit) tests for the lower level layers depend on functions or classes they don't need to depend on.

Eric_A’s picture

Anyway, TableTest is testing rendered markup for tables. It's not a "low level" theme engine test. Core is moving towards #type 'table' in drupal_render() for all tables so that's probably what the tests should use to create the markup being checked for sticky headers or zebra striping. Using theme() here could hide problems. We don't need layers of tests all checking sticky headers in #type 'table', #theme 'table', theme('table'), theme_table() - we just need one test that is giving us table markup in the same way that you'd expect to build the markup while building a site following current "best practise".

Doing theme('table') is essentially about what is passed to the hook and marking it up it as a table, using a theme engine.
Rendering a type-less element with just #theme table, is almost the same but it still goes through the render layer next to the theme layer.
Rendering a #type table means dealing with a value callback, a validate callback, a process callback, a pre_render callback before finally theming the table.

The work that is done in the theme layer is tested with the theme layer and nothing more. And apparently it is here that we disagree, because I don't want to put it all in one test. I would rather like to see unit tests all over the place. A test like TwigDebugMarkupTest could probably be improved a lot. Why create a node and do that whole dance? In the end it is about Twig adding debug info to HTML markup. It is in the "Theme" group, not in "Common". The implementation using the node_view() function is distracting us way too much here.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
20.99 KB

Here's a patch (with interdiff.txt, sorry about not posting them before) that reverts the documentation change (#48) and all the changes to ThemeTest (#51).

How about we get this patch in and argue about Drupal's testing principles in another issue?

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

The last submitted patch, system-replace-theme-2006152-56.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
Issue tags: +CodeSprintUA
thedavidmeister’s picture

Status: Needs review » Needs work

+1 for #56. Apologies for getting bogged down in academic discussions. I think we'll revisit conversions for these tests in the near-mid term future if the proposed changes to the theme system elsewhere make it into D8, otherwise it's fine to leave them as theme() - either way it can be a separate issue.

The patch in #56 still touches tests though, so it won't get past Eric_A's reviews. Can we rip conversions out of table and twig debug tests to get all the other conversion work in?

pplantinga’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
15.59 KB

Here's the patch without table and twig tests.

thedavidmeister’s picture

Great, thanks. @Eric_A, is this ok now?

Eric_A’s picture

Status: Needs review » Needs work
diff --git a/core/modules/system/theme.api.php b/core/modules/system/theme.api.php
index aedc6b0..b25636c 100644
--- a/core/modules/system/theme.api.php
+++ b/core/modules/system/theme.api.php
@@ -186,7 +186,13 @@ function hook_process(&$variables, $hook) {
         'variable_name' => $variable_name,
         'variables' => $variables,
       );
-      $variables[$variable_name] = theme('rdf_template_variable_wrapper', array('content' => $variables[$variable_name], 'attributes' => $attributes, 'context' => $context));

No changes to theme.api.php, please. Changing the theme layer API itself is not the point here.

@@ -748,6 +786,7 @@ function theme_system_modules_uninstall($variables) {
  * @param $variables
  *   An associative array containing:
  *   - theme_groups: An associative array containing groups of themes.
+ *   - theme_group_titles: An associative array containing titles of themes.
  *
  * @ingroup themeable
  */

Aren't theme_group_titles titles of groups of themes?

+++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.php
@@ -34,7 +34,12 @@ public function onRequest(GetResponseEvent $event) {
       // theme_test_request_listener_page_callback() to test that even when the
       // theme system is initialized this early, it is still capable of
       // returning output and theming the page as a whole.
-      $GLOBALS['theme_test_output'] = theme('more_link', array('url' => 'user', 'title' => 'Themed output generated in a KernelEvents::REQUEST listener'));

I would think that this scenario is targeting specifically the theme system. If so, then simply converting theme() to drupal_render() would not be ok.

The test modules have a lot of menu page callback implementations. I think it would be very good to just return the render elements instead of doing return drupal_render($elements) whenever it's a trivial case. I'm focusing on tests and test modules here because I think in tests using drupal_render() in menu page callbacks is even more of an issue then in normal code.

Eric_A’s picture

hook_process() is gone and with it the theme('rdf_template_variable_wrapper') example so theme.api.php is not an issue anymore.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
14.75 KB

re-roll and tried to remove some obvious calls to drupal_render(). Also fixed groups of titles error and reverted themetestsubscriber.

pplantinga’s picture

Issue tags: -CodeSprintUA

Status: Needs review » Needs work
Issue tags: +CodeSprintUA

The last submitted patch, system-replace-theme-2006152-64.patch, failed testing.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
13.95 KB

re-roll.

thedavidmeister’s picture

Status: Needs review » Needs work
   $admin_form = drupal_get_form('system_themes_admin_form', $admin_theme_options);
-  return theme('system_themes_page', array('theme_groups' => $theme_groups, 'theme_group_titles' => $theme_group_titles)) . drupal_render($admin_form);
+  $system_themes_page = array(
+    '#theme' => 'system_themes_page',
+    '#theme_groups' => $theme_groups,
+    '#theme_group_titles' => $theme_group_titles,
+  );
+  return drupal_render($system_themes_page) . drupal_render($admin_form);

Could this not be two elements in a single render array, rather than two separate arrays concatenated with two drupal_render() calls?

eg:

$output = array();
$output['system_themes_page'] = ....;
$output['admin_form'] = ...;
return drupal_render($output);
pplantinga’s picture

Status: Needs work » Needs review
FileSize
887 bytes
14.08 KB

Here it is.

thedavidmeister’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing
+  $output = array();
+  $output['system_themes_page'] = array(
+    '#theme' => 'system_themes_page',
+    '#theme_groups' => $theme_groups,
+    '#theme_group_titles' => $theme_group_titles,
+  );
+  $output['admin_form'] = drupal_get_form('system_themes_admin_form', $admin_theme_options);
+  return $output;

Actually, this should probably be $build, my bad. Also, we'll need manual testing if we're returning the array instead of returning the rendered markup.

It's also possible that we'll need to add '#sorted' => TRUE to our $build to stop 'admin_form' appearing before 'system_themes_page'

pplantinga’s picture

Status: Needs work » Needs review
FileSize
869 bytes
14.08 KB

Here's the updated patch with $build instead of $output.

I looked at the "appearance" page, changed themes and stuff, and everything looks fine to me. Someone else should probably still take a look though.

andypost’s picture

@@ -57,7 +60,11 @@ function system_admin_config_page() {
+    $admin_page = array(
+      '#theme' => 'admin_page',
+      '#blocks' => $blocks,
+    );
+    return drupal_render($admin_page);
...
     return t('You do not have any administrative items.');

Suppose better to return renderable array

@@ -853,7 +893,12 @@ function system_date_format_language_overview_page() {
+  $table = array(
+    '#theme' => 'table',
+    '#header' => $header,
+    '#rows' => $rows,
+  );
+  return drupal_render($table);

same here

dmitrii’s picture

Issue tags: +CodeSprintCIS
FileSize
663 bytes
14.05 KB

Added #72

drupalmonkey’s picture

Assigned: Unassigned » drupalmonkey
Issue tags: -CodeSprintCIS
stuajc’s picture

Assigned: drupalmonkey » Unassigned
Status: Needs review » Reviewed & tested by the community

Applied patch from #73 and manually reviewed relevant admin pages and site installation process to verify expected output. Looks good.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

Reviewed the patch, overall looking very good!

  1. +++ b/core/modules/system/system.admin.inc
    @@ -57,7 +60,11 @@ function system_admin_config_page() {
    -    return theme('admin_page', array('blocks' => $blocks));
    +    $admin_page = array(
    +      '#theme' => 'admin_page',
    +      '#blocks' => $blocks,
    +    );
    +    return $admin_page;
    
    @@ -853,7 +893,12 @@ function system_date_format_language_overview_page() {
    -  return theme('table', array('header' => $header, 'rows' => $rows));
    +  $table = array(
    +    '#theme' => 'table',
    +    '#header' => $header,
    +    '#rows' => $rows,
    +  );
    +  return $table;
    

    We said we would not return render arrays directly at this stage in the conversion, see #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(). I think for now these should be drupal_render()ed. If we are going to return render arrays then no need to create the variables like this.

    Edit: I see this was suggested in #72 but I think we should stick to the method defined in the meta issue.

  2. +++ b/core/modules/system/system.admin.inc
    @@ -212,8 +220,14 @@ function system_themes_page() {
    -  $admin_form = drupal_get_form('system_themes_admin_form', $admin_theme_options);
    -  return theme('system_themes_page', array('theme_groups' => $theme_groups, 'theme_group_titles' => $theme_group_titles)) . drupal_render($admin_form);
    +  $build = array();
    +  $build['system_themes_page'] = array(
    +    '#theme' => 'system_themes_page',
    +    '#theme_groups' => $theme_groups,
    +    '#theme_group_titles' => $theme_group_titles,
    +  );
    +  $build['admin_form'] = drupal_get_form('system_themes_admin_form', $admin_theme_options);
    +  return $build;
    

    How are we ensuring that system_themes_page comes before admin_form?

pplantinga’s picture

1. attached is the patch from #71 again without the changes suggested by #72

2.

It's also possible that we'll need to add '#sorted' => TRUE to our $build to stop 'admin_form' appearing before 'system_themes_page'

Manual testing showed that this was not needed. I think drupal_render() renders child elements in the order that they're added. I'd be happy to add '#sorted' => TRUE if it is needed for possible future change of behavior, though.

pplantinga’s picture

Status: Needs work » Needs review
thedavidmeister’s picture

Manual testing showed that this was not needed. I think drupal_render() renders child elements in the order that they're added.

If '#sorted' => TRUE then drupal_render() will render child elements in the order they're added, otherwise it does this:

  // Get the children of the element, sorted by weight.
  $children = element_children($elements, TRUE);

Where element_children() does this:

   uasort($children, 'element_sort');

If you've manually tested the sorting, it should be fine.

star-szr’s picture

Status: Needs review » Needs work

Okay, then let's please add #sorted => TRUE. I don't trust the code as is :)

pplantinga’s picture

Status: Needs work » Needs review
FileSize
561 bytes
14.1 KB

Okay. Here it is.

thedavidmeister’s picture

Status: Needs review » Needs work
-      $GLOBALS['theme_test_output'] = theme('more_link', array('url' => 'user', 'title' => 'Themed output generated in a KernelEvents::REQUEST listener'));
+      $more_link = array(
+        '#theme' => 'more_link',
+        '#url' => 'user',
+        '#title' => 'Themed output generated in a KernelEvents::REQUEST listener',
+      );
+      $GLOBALS['theme_test_output'] = drupal_render($more_link);

Hey, look at the last point of #62. We missed that in #81, still trying to convert a theme() test to drupal_render(), which is something that has caused quite a bit of bike-shedding spirited discussion in the other conversion issues touching tests.

Let's just not convert anything in a test as it's debatable whether the conversion of theme() in tests to drupal_render() is actually desirable. I, for one, don't want to have that debate again just yet.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
12.82 KB

Kay removed changes to the test. The diff is just a simple diff command between #81 and #83.

thedavidmeister’s picture

Status: Needs review » Needs work
     'variables' => array('foo' => ''),
   );
   $items['theme_test_template_test'] = array(
+    'variables' => array(),
     'template' => 'theme_test.template_test',
   );
   $items['theme_test_template_test_2'] = array(
+    'variables' => array(),
     'template' => 'theme_test.template_test',
   );
   $items['theme_test_foo'] = array(
@@ -100,7 +102,8 @@ function theme_test_request_listener_page_callback() {
  * Menu callback for testing template overridding based on filename.
  */
 function theme_test_template_test_page_callback() {
-  return theme('theme_test_template_test');
+  $theme_test_template_test = array('#theme' => 'theme_test_template_test');
+  return drupal_render($theme_test_template_test);
 }
 
 /**
@@ -146,7 +149,8 @@ function _theme_test_alter() {
  * Page callback, calls a theme hook suggestion.
  */
 function _theme_test_suggestion() {
-  return theme(array('theme_test__suggestion', 'theme_test'), array());
+  $theme_test = array('#theme' => array('theme_test__suggestion', 'theme_test'));
+  return drupal_render($theme_test);
 }
 
 /**
diff --git a/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.php b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.php
index 6eb318a..e4ba607 100644
--- a/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.php
+++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.phpundefined
@@ -26,7 +26,8 @@ public static function create(ContainerInterface $container) {
    * Menu callback for testing PHP variables in a Twig template.
    */
   public function phpVariablesRender() {
-    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);
   }
 
   /**
diff --git a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
index 934d1b0..cc74184 100644
--- a/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.module
+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.moduleundefined
@@ -5,6 +5,7 @@
  */
 function twig_theme_test_theme($existing, $type, $theme, $path) {
   $items['twig_theme_test_php_variables'] = array(
+    'variables' => array(),

Still a whole bunch of test code being touched here.

pplantinga’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
8.12 KB

Reroll and include changes from #84

c4rl’s picture

Assigned: Unassigned » c4rl

Reviewing

c4rl’s picture

Assigned: c4rl » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

This looks good to me. Tested all callbacks manually to make sure they are working.

Patch filename refers to wrong issue #, but that's probably minor. :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

includes #2013717