Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
- This issue is postponed on #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system.
- Review #2009670: Replace theme() with drupal_render() in simpletest module and #2009674: Replace theme() with drupal_render() in system module to see if these uses were deliberately skipped, and why.
- Review each remaining uses of
_theme()
- Where possible, refactor the tests or test modules to use
drupal_render()
. - File separate issues for specific problem spaces -- for example, the use in
WebTestBase
might be one issue, Twig self-tests might be another, andThemeTest
andTableTest
might be a third.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-2191135-36.txt | 5.55 KB | botanic_spark |
#36 | refactor-theme-tests-2191135-36.patch | 5.02 KB | botanic_spark |
#31 | interdiff-2191135-31.txt | 1.17 KB | botanic_spark |
#31 | refactor-theme-tests-2191135-31.patch | 5.15 KB | botanic_spark |
#28 | interdiff-2191135-28.txt | 5.23 KB | botanic_spark |
Comments
Comment #1
xjmComment #2
star-szrTagging for rocketship purposes. Thanks @xjm!
Comment #3
RainbowArrayI 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.
Comment #4
RainbowArrayHere'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.
Comment #7
RainbowArrayjoelpittet explained to me that drupal_render can't have arrays passed in directly. Revised patch to take that into account.
Comment #8
RainbowArrayComment #10
RainbowArrayFixed some dumb PHP syntax errors on my part.
Comment #11
RainbowArraySome 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.
Comment #13
RainbowArrayTwo more syntax bugs.
Comment #15
xjmThis 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.
Comment #16
RainbowArraySorry, 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.
Comment #17
xjm#2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system is in now, so this is good to proceed. Thanks @mdrummond for the start!
Comment #18
star-szrThanks for getting this started @mdrummond. Here are a few notes to help move things along:
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.
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.
'node' =>
should be'#node' =>
This is handled by #2201789: Don't print "_theme()" in twig_debug output.
This is passing an array of theme hooks with no variables, should be closer to:
Comment #19
botanic_spark CreditAttribution: botanic_spark commentedComment #20
botanic_spark CreditAttribution: botanic_spark commentedRegarding the test:
/core/modules/system/lib/Drupal/system/Tests/Theme/TwigDebugMarkupTest.php
When we replace
with
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
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
instead?
Comment #21
botanic_spark CreditAttribution: botanic_spark commentedRerolled the patch #13, applied the changes recommended in #18.
Here is the patch and interdiff.
Changes from WebTestBase are excluded.
The part
is replaced with
which solves all the problems from previous comment.
Comment #23
botanic_spark CreditAttribution: botanic_spark commentedReplaced
with
Comment #25
star-szrThanks 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 :)
This should be left as is, _theme() accepts $variables but drupal_render() does not.
Please leave all these changes to Twig debug output to #2201789: Don't print "_theme()" in twig_debug output :)
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 :)
Comment #26
star-szrAnd I think once the patch is scaled back (points 2 and 3 from my review).
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.
Comment #27
botanic_spark CreditAttribution: botanic_spark commented@Cottser
Are you saying in comment #26 that we should return just
instead of
Comment #28
botanic_spark CreditAttribution: botanic_spark commentedHere is a re-rolled patch with the changes you noted in comment #25.
Comment #29
star-szr#27 - exactly! That applies to TwigThemeTestController::phpVariablesRender() as well.
Comment #30
star-szrI created a sub-issue to look at WebTestBase: #2227569: Add a helper method to WebTestBase for checking the output of render arrays and deprecate assertThemeOutput()
Comment #31
botanic_spark CreditAttribution: botanic_spark commentedChanges from comment #26
Comment #33
star-szrThanks @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.
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.
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 :)The @return needs to be updated here, to @return array and something starting with "A render array..."
Please change this menu/page callback to return an array as well like the other two that were updated in the most recent patch.
Comment #35
star-szrThose 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:
…and there a number of instances in theme_test_theme() that define neither. Let's just fix them all here.
Comment #36
botanic_spark CreditAttribution: botanic_spark commentedRe-rolled and applied changes from #33 and #35
Comment #37
botanic_spark CreditAttribution: botanic_spark commentedComment #39
star-szrLooks like we're missing one more 'variables' definition somewhere, try running
Drupal\system\Tests\Theme\ThemeTestTwig
locally or start digging there anyway :)Comment #40
sunHm. 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 bothWebTestBase
andDrupalUnitTestBase
, which allows to calldrupal_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()
Comment #41
botanic_spark CreditAttribution: botanic_spark commentedComment #42
star-szrIt 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.
Comment #43
andypostgrepping 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