Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().
Has some collisions with #1938930: Convert theme_system_modules_details() to #type table
Related issues
#2010086: Replace theme() with drupal_render() in system module for theme_system_themes_page()
#1826514: Convert theme_system_themes_page to twig
#1987854: Convert system_themes_page() to a new style controller
#2010982: Replace theme() with drupal_render() in system module for system_status() Assigned to: somepal
#2013717: Replace theme() with drupal_render() in system module for system_admin_config_page() Assigned to: somepal
Comment | File | Size | Author |
---|---|---|---|
#85 | system-replace-theme-2006152-85.patch | 8.12 KB | pplantinga |
#85 | interdiff.txt | 2.9 KB | pplantinga |
#83 | system-replace-theme-2006152-83.patch | 12.82 KB | pplantinga |
#83 | patch-diff.txt | 1.33 KB | pplantinga |
#81 | system-replace-theme-2006152-81.patch | 14.1 KB | pplantinga |
Comments
Comment #1
Samvel CreditAttribution: Samvel commentedComment #2
Samvel CreditAttribution: Samvel commentedattached patch
Comment #3
andypostI 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
Comment #3.0
andypostrelated
Comment #5
Samvel CreditAttribution: Samvel commentedattached new one
And created separate task for system_status() #2010982: Replace theme() with drupal_render() in system module for system_status() Assigned to: somepal
Comment #5.0
Samvel CreditAttribution: Samvel commentedUpdated issue summary.
Comment #7
andypostAwesome 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!
Better provide a $build array with $build['#theme'] = 'system_admin_index';
Same here! just convert this to render array
Also I see no reason to rename local variable.
this exactly should be renderable too.
same here
same
Comment #8
andypostThe related issue #1938930: Convert theme_system_modules_details() to #type table
Comment #8.0
andypostappend separated task
Comment #9
somepal CreditAttribution: somepal commented@andypost what about other calls to theme() in system module like one I spotted in theme.api.php -
Do we create separate issue for it. most of them are in Test related files.
Comment #9.0
somepal CreditAttribution: somepal commentedUpdated issue summary.
Comment #10
andypostyes, please fine new issue! nice piece of ugly code....
Comment #11
andypost@Joe9 I think theme.api should be separate issue with review of @jhodgdon to be commited
Comment #12
pplantinga CreditAttribution: pplantinga commentedIs it okay for me to take a look at this? @Samvel hasn't posted in a month.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commentedgo for it.
Comment #14
pplantinga CreditAttribution: pplantinga commentedCool. 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?
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedwhy are we ignoring some theme() calls?
Comment #17
pplantinga CreditAttribution: pplantinga commentedWell 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).
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedarray('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 :)
Comment #19
pplantinga CreditAttribution: pplantinga commentedHere'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?
Comment #20
pplantinga CreditAttribution: pplantinga commentedNavigating drupal with only a keyboard is a bit difficult, had trouble attaching the patch
Comment #22
pplantinga CreditAttribution: pplantinga commentedGah syntax error... Okay here we go.
Comment #24
pplantinga CreditAttribution: pplantinga commentedThis is proving quite difficult.
One slew of errors is caused by converting
theme('status_report', array('requirements' => $requirements));
to
Any idea why that might be?
Still trying to track down the others.
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commented#2030805: Make theme_status_report compatible with drupal_render()
Comment #26
pplantinga CreditAttribution: pplantinga commentedI guess this issue should wait til that one is done then....
Comment #27
Eric_A CreditAttribution: Eric_A commented#2030805: Make theme_status_report compatible with drupal_render() has been resolved.
Comment #28
Eric_A CreditAttribution: Eric_A commented#2030805: Make theme_status_report compatible with drupal_render() is partially resolved. Also, another approach is up for discussion in #2041283: theme_status_report() is severely broken.
Comment #29
pplantinga CreditAttribution: pplantinga commentedhere's another patch that hopefully fixes all errors. It makes the same change as #2041283: theme_status_report() is severely broken
Comment #31
Eric_A CreditAttribution: Eric_A commented#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.)Comment #32
pplantinga CreditAttribution: pplantinga commentedAnother hopefully clean patch, except for one error that I need some input on. This code:
ignores the '#elements' attribute, where theme() doesn't.
This patch also incorporates changes requested in #31, except for:
which I couldn't find. Perhaps you were looking at a line from the comments?
Comment #34
Eric_A CreditAttribution: Eric_A commentedIn the patch from #29 you had those in
TwigDebugMarkupTest
. Fixed in the patch from #32.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:
your render element should look like this:
Comment #35
Eric_A CreditAttribution: Eric_A commentedIn the previous comment I forgot to add the theme callback as #theme property. Edited that.
Comment #36
Eric_A CreditAttribution: Eric_A commentedNote 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.
Comment #37
pplantinga CreditAttribution: pplantinga commentedI tried the code from #34 and still failed. The output of that code is
So now we're missing both attributes.
I also tried putting it all into a giant array like this
with no success.
Comment #38
pplantinga CreditAttribution: pplantinga commentedWell 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.
Comment #40
pplantinga CreditAttribution: pplantinga commentedGah! These missing 'variables' arrays have no end.
Comment #42
pplantinga CreditAttribution: pplantinga commented#40: system-replace-theme-2006152-40.patch queued for re-testing.
Comment #43
Eric_A CreditAttribution: Eric_A commentedFrom
core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
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 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 ofThemeTest::testAttributeMerging()
, but anyway, I'm pretty sure we should not convertThemeTest::testAttributeMerging()
.Comment #44
pplantinga CreditAttribution: pplantinga commentedHere's a test patch with the code from #34 to verify that it fails.
Also a patch with updated documentation.
Comment #45
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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.
Comment #46
pplantinga CreditAttribution: pplantinga commentedThat makes sense. Here's an updated patch (plus re-roll).
Comment #47
thedavidmeister CreditAttribution: thedavidmeister commentedThis 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
Comment #48
Eric_A CreditAttribution: Eric_A commentedChanging 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.Comment #49
thedavidmeister CreditAttribution: thedavidmeister commentedFair 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.
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.
Comment #50
Eric_A CreditAttribution: Eric_A commentedIdeally 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?Comment #51
thedavidmeister CreditAttribution: thedavidmeister commentedok, 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?
Comment #52
Eric_A CreditAttribution: Eric_A commentedSorry, 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().
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.
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.
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commentedIt 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().
Comment #54
Eric_A CreditAttribution: Eric_A commentedTrue, 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?)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.
Comment #55
Eric_A CreditAttribution: Eric_A commentedDoing 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.
Comment #56
pplantinga CreditAttribution: pplantinga commentedHere'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?
Comment #58
pplantinga CreditAttribution: pplantinga commented#56: system-replace-theme-2006152-56.patch queued for re-testing.
Comment #59
thedavidmeister CreditAttribution: thedavidmeister commented+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?
Comment #60
pplantinga CreditAttribution: pplantinga commentedHere's the patch without table and twig tests.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commentedGreat, thanks. @Eric_A, is this ok now?
Comment #62
Eric_A CreditAttribution: Eric_A commentedNo changes to theme.api.php, please. Changing the theme layer API itself is not the point here.
Aren't theme_group_titles titles of groups of themes?
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.Comment #63
Eric_A CreditAttribution: Eric_A commentedhook_process()
is gone and with it thetheme('rdf_template_variable_wrapper')
example so theme.api.php is not an issue anymore.Comment #64
pplantinga CreditAttribution: pplantinga commentedre-roll and tried to remove some obvious calls to drupal_render(). Also fixed groups of titles error and reverted themetestsubscriber.
Comment #65
pplantinga CreditAttribution: pplantinga commented#64: system-replace-theme-2006152-64.patch queued for re-testing.
Comment #67
pplantinga CreditAttribution: pplantinga commentedre-roll.
Comment #68
thedavidmeister CreditAttribution: thedavidmeister commentedCould this not be two elements in a single render array, rather than two separate arrays concatenated with two drupal_render() calls?
eg:
Comment #69
pplantinga CreditAttribution: pplantinga commentedHere it is.
Comment #70
thedavidmeister CreditAttribution: thedavidmeister commentedActually, 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'
Comment #71
pplantinga CreditAttribution: pplantinga commentedHere'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.
Comment #72
andypostSuppose better to return renderable array
same here
Comment #73
dmitrii CreditAttribution: dmitrii commentedAdded #72
Comment #74
drupalmonkey CreditAttribution: drupalmonkey commentedComment #75
stuajc CreditAttribution: stuajc commentedApplied patch from #73 and manually reviewed relevant admin pages and site installation process to verify expected output. Looks good.
Comment #76
star-szrReviewed the patch, overall looking very good!
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.
How are we ensuring that system_themes_page comes before admin_form?
Comment #77
pplantinga CreditAttribution: pplantinga commented1. attached is the patch from #71 again without the changes suggested by #72
2.
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.
Comment #78
pplantinga CreditAttribution: pplantinga commentedComment #79
thedavidmeister CreditAttribution: thedavidmeister commentedIf '#sorted' => TRUE then drupal_render() will render child elements in the order they're added, otherwise it does this:
Where element_children() does this:
If you've manually tested the sorting, it should be fine.
Comment #80
star-szrOkay, then let's please add #sorted => TRUE. I don't trust the code as is :)
Comment #81
pplantinga CreditAttribution: pplantinga commentedOkay. Here it is.
Comment #82
thedavidmeister CreditAttribution: thedavidmeister commentedHey, 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-sheddingspirited 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.
Comment #83
pplantinga CreditAttribution: pplantinga commentedKay removed changes to the test. The diff is just a simple diff command between #81 and #83.
Comment #84
thedavidmeister CreditAttribution: thedavidmeister commentedStill a whole bunch of test code being touched here.
Comment #85
pplantinga CreditAttribution: pplantinga commentedReroll and include changes from #84
Comment #86
c4rl CreditAttribution: c4rl commentedReviewing
Comment #87
c4rl CreditAttribution: c4rl commentedThis 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. :)
Comment #88
catchCommitted/pushed to 8.x, thanks!
Comment #89.0
(not verified) CreditAttribution: commentedincludes #2013717