Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Task
Create a Twig template to replace the theme function theme_simpletest_result_summary()
.
Remaining
Patch needs reviewManual testing
Theme function name/template path | Conversion status |
---|---|
theme_simpletest_result_summary | converted |
Testing steps
Compare markup and test functionality for the following:
- Enable Testing module (simpletest.module).
- Navigate to admin/config/development/testing.
- Select two or three individual tests to run (expand to choose individual tests). -
this table is simpletest-test-table.html.twig(the table theme function was converted to #type table instead) - Wait for the test results to come back. - this table uses simpletest-result-summary.html.twig.
Run another test in the command line.
Example test run.
php ./core/scripts/run-tests.sh --url http://d8.dev --verbose --color --class "Drupal\system\Tests\Theme\TwigFilterTest"
Related
#1757550: [Meta] Convert core theme functions to Twig templates
Comment | File | Size | Author |
---|---|---|---|
#101 | interdiff.txt | 1.96 KB | star-szr |
#101 | 1898452-101.patch | 5.9 KB | star-szr |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
disasm CreditAttribution: disasm commentedAttached is a super hacky patch. For now, just setting the themed table as a variable table and outputting that variable by itself in the theme. If this does go green by some miracle, need to clean up template_preprocess_simpletest_test_table and it's twig template.
Comment #4
joelpittetA little less rough;)
Comment #5
joelpittetComment #7
joelpittet#4: drupal-simpletest_twig-1898452-4.patch queued for re-testing.
Comment #9
joelpittet#4: drupal-simpletest_twig-1898452-4.patch queued for re-testing.
Comment #11
star-szrI'm thinking this is probably why testbot can't login to the test site.
Comment #12
joelpittetOh that's not a good thing to have in a commit:S
Same as #4 but without the Rebase change from my dev env.
Comment #14
joelpittetI have a feeling this may still fail testbot but it's slightly closer than it was. I converted the
'#theme' => 'table'
to
'#type' => 'table'
Which does good for the first table but the results after the tests are messed up and I have a feeling it has to do with colspan issue noted in http://drupal.org/node/1876712#comment-7156742.
#1876712: [meta] Convert all tables in core to new #type 'table'
Comment #16
joelpittetthere was a nasty typo and a issue with variables vs render element
Comment #17
star-szrTagging.
Comment #17.0
star-szrAdd conversion summary table
Comment #17.1
star-szrAdd testing steps
Comment #18
star-szrI just tested this patch, works perfectly!
The preprocess function docblocks should be updated to meet #1913208: [policy] Standardize template preprocess function documentation:
Prepares variables for test list templates. / Prepares variables for test result summary templates.
Default template: …
@param array $variables
And remove @ingroup themeable.
Once that's done, I think this is ready to go.
Comment #19
star-szrOr maybe "test list table templates". Just suggestions :)
Comment #20
star-szrCNW for #18.
Comment #21
joelpittetComment #22
joelpittetDid a bit more, and fixed the typo... Could do table twig template or possibly convert to type table.
Comment #24
star-szrThe changes are looking good (other than the syntax bit), will leave this at needs review for now, @joelpittet said he would take another look at the table again to see what's possible.
This should end in "templates".
Comment #25
joelpittetWell, got crazy there... those need to be rendered with theme( the way they are because they are used for JS.
But that style should really be done with CSS... instead of images.
Reverted and added doc cleanup.
Comment #26
joelpittetComment #27
joelpittethmm one of those should be an interdiff. The save button got away on me before I could remove the non-rebased one.
Ok so, I replaced the theme_table with a twig table, in hopes in the future some brave soul would find it easier to swap the table out with some dl/ol/ul markup. Zebra was being added (but not used) and affected the layout so I added a css class to keep the margin down as in the original.
Comment #28
joelpittetCan't think of a good way to remove these theme() without losing the localization.
Comment #29
joelpittetOk we are going to leave that theme('image') and use #27 as discussed with Cottser. This should be probably reworked for the CSS/JS but outside of this conversions scope. The theme('image') string is passed along to JS directly so converting to array('#theme' => 'image') won't do the trick here unless we wrap it in render() which is just busy work for people later.
Hope someone could confirm the output is right through manual testing?
Comment #30
thedavidmeister CreditAttribution: thedavidmeister commentedI'll do the manual testing.
Comment #30.0
thedavidmeister CreditAttribution: thedavidmeister commentedRemove empty
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commentedThis is for simpletest-test-table.html.twig for the first couple of test groups, I removed the whitespace as it was making the diff hard to read:
Original:
New:
Looking at the diff I can see two things:
- the original table had
class="sticky-enabled responsive-enabled"
- the original table had zebra striping
class="simpletest-group odd"
, I know this is being killed off but are we doing that during or post Twig conversion?Comment #32
thedavidmeister CreditAttribution: thedavidmeister commented+<div class="simpletest-{{ status }}">{{ summary }}</div>
This class like it would be better as a standard Attribute() class rather than a string that is half generated in the preprocess and half in the template.
Comment #33
thedavidmeister CreditAttribution: thedavidmeister commentedScrap what I said in #32, what is even the point of simpletest-result-summary.html.twig? all it does is renders this tiny little bit of the page:
<div class="simpletest-pass">42 passes, 0 fails, 0 exceptions, and 5 debug messages</div>
You can't edit the table rows through the theme system, not even the image that displays. All you can do is change part of the class of the summary, BUT you can't change the "simpletest-pass" class being added to all the table rows about a thousand times on the page - the whole thing is generated in one big page callback and then about 3 divs are run through the theme system - so all you can do in the template is make one class for a small part of the page inconsistent with all the classes on the rest of the page. Completely useless..
My vote is to get rid of theme('simpletest_result_summary') completely - just turn it into a renderable array in the page callback like everything else on the results page is.
Then the next question is, if the results page is not run through the theme system why is the test summary page run through the theme system? that's just inconsistent and you can only theme the page that you would have less reason to want to theme out of the two (can touch the list of checkboxes but not the thing with all the green/yellow/red colouring with images?).
So I'm also going to propose that we choose to either convert the entire page callback of the results page into a Twig template OR move the simpletest-test-table template into a page callback and out of the theme system (making this issue into "remove SimpleTest from the theme system").
Comment #34
thedavidmeister CreditAttribution: thedavidmeister commentedComment #34.0
thedavidmeister CreditAttribution: thedavidmeister commentedupdated manual testing steps
Comment #35
thedavidmeister CreditAttribution: thedavidmeister commentedI'm just removing theme_simpletest_result_summary() in this patch to see what the testbots think.
Comment #36
thedavidmeister CreditAttribution: thedavidmeister commentedComment #37
thedavidmeister CreditAttribution: thedavidmeister commentedwith #27 as well.
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commentedback to needs work because of markup differences from #31
Comment #39
gnugetComment #40
gnugetHi!
I attach a reroll of #37 because of #1978982: Convert simpletest_result_form to a Controller
now taking in account to #1948374: #type 'table' allow attributes on table cells is fixed and now is possible use attributes (and therefore colspan ) at the cells, instead to work in the diferences on #31 in my next patch i will to try to provide a version using theme_table instead to use a custom twig table.
Comment #40.0
gnugetRemove sandbox link
Comment #41
gnugetI will switch temporary the status to "needs review" for make sure to my reroll does not broke anything, i will switch back to "needs work" once the testbot check the patch
Comment #42
gnugetComment #43
gnugetThis patch is based in #37 and in #25 where was remove the
theme_simpletest_result_summary
and the twig table was not addedComment #44
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #45
intergalactic overlords CreditAttribution: intergalactic overlords commented#43: 1898452-43-twig-simpletest.patch queued for re-testing.
Comment #47
zaphoyd CreditAttribution: zaphoyd commentedComment #48
zaphoyd CreditAttribution: zaphoyd commentedComment #50
joelpittet#43: 1898452-43-twig-simpletest.patch queued for re-testing.
Comment #51
ezeedub CreditAttribution: ezeedub commented#48: twig-simpletest.patch queued for re-testing.
Comment #52
joelpittetThis one needs another re-roll I think from #40 maybe. Also, kinda disappointed the table got removed, but I'll get over it. Was just hoping that would help themers move that tree structure away from a table in the future and that would give them a better crack at it.
Anyways, there is lots of new stuff in #48 so @zaphoyd could you explain what you did there? It's got a bunch of new stuff than #40 had if you diff the diffs.
Comment #53
gnuget#52 Accord with http://drupal.org/node/1898034#comment-7421300 Cottser is agree with you.
I think to is good idea to the table don't be removed , and if that is the case then the patch in #40 is the path to follow (that patch still contain the table)
Comment #54
thedavidmeister CreditAttribution: thedavidmeister commentedWell what I was saying in #33:
The table you can theme is not the one with all the images and colours in it...
Comment #55
zaphoyd CreditAttribution: zaphoyd commentedIn theory this should be a better reroll of #40 than the last one. Hopefully this works!
Comment #57
joelpittet#55: 1898452-55-twig-simpletest.patch queued for re-testing.
Comment #58
joelpittetI'm a bit torn, this theme doesn't need to be overridden IMO by themers. But it would be nice to have a twig template for internal use and hopefully let the core theme developers play with raw markup instead of the convoluted/confusing mess that is #type=>table.
I do like the consistency that type table brings, but I think the cost of making the code unintelligible is maybe greater.
Comment #59
joelpittet@Cottser is going to give a try at the type table conversion and maybe that will remove all simpletest theme functions.
#1938926: Convert simpletest theme tables to table #type
Comment #60
star-szrJust tweaking tags.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commentedComment #62
thedavidmeister CreditAttribution: thedavidmeister commentedManual testing is a novice task.
Comment #63
azinoman CreditAttribution: azinoman commentedRerolled!
Comment #65
jenlamptonrerolling
Comment #66
jenlamptonrerolled patch. and adding manual testing while I'm at it...
Before:
After:
It looks like we're loosing a lot of the classes here. I don't think they were all intentionally dropped. Also, I'm not sure if we're allowed to drop the even/odd until after #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
Comment #67
star-szrThere is also #1938926: Convert simpletest theme tables to table #type which last I checked the markup matches up but it's sloooow. See #1938926-18: Convert simpletest theme tables to table #type.
Comment #68
jenlamptonI confirmed with the core CSS crew that it's okay to drop the even/odd classes, so let's leave those be.
The attached patch adds the responsiveness back to the table.
Comment #69
jenlampton@Cottser, sorry about the cross-post.
I like the approach in #1938926: Convert simpletest theme tables to table #type better than having a huge table in a template like we do here, but since it's so slow maybe we should get this in first?
Comment #70
star-szrI think this patch might need some profiling anyway. And a reroll.
Comment #71
joelpittetRerolled and Cleaned up the formatting of the twig table indentation and filled out the docblock.
Comment #72
joelpittetRe-rolled again.
Comment #73
joelpittetDoc cleanup and whitespace cleanup.
Comment #73.0
joelpittetupdated result summary status
Comment #74
joelpittet73: 1898452-73-twig-simpletest.patch queued for re-testing.
Comment #75
robmc CreditAttribution: robmc commentedRe-roll
Comment #76
star-szrComment #77
star-szrComment #78
joelpittetAdding manual testing back to this but removing profiling.
This has been redone due to the drastic changes from #1938926: Convert simpletest theme tables to table #type
Hope the changes sit well with all.
Comment #79
joelpittetJust realized I missed a colon.
Comment #82
joelpittetHmm, should have checked a wider scope before deleting that private function. This abstracts things out a bit more so there isn't duplicate code. Should pass now. *crosses fingers*
Comment #83
pjezek CreditAttribution: pjezek commentedwill profile this issue. Just found out that the *simpletest-test-table.html.twig* is no longer contained in the lastest patch. I assume the *how to test* is just outdated?
edit:
- the simpletest-test-table.html.twig looks not to be needed.
- can't really profile as the request is a post?
Comment #84
joelpittetThis needs an Needs issue summary update re #83.
Comment #85
sunThis closely related issue slightly conflicts with the proposed changes here.
In addition, I'm not sure whether it is really worth to have a full theme template with preprocess and all for that trivial test summary line... :-/
I assume we do not have a solution for a sensible middle-ground with Twig yet? Does everything have to be a template...?
Comment #86
joelpittet@sun Yes, the goal is anything with markup should be in a template. There are exceptions but trying to be as hard on that point as I can be.
$output .= '<p>No markup in PHP strings please!.</p>'
We can render a template with the twig service without using the theme system... don't know if I want to play with that here though because we haven't done it yet.
Comment #87
joelpittetRe-rolled.
Comment #88
joelpittetComment #89
steinmb CreditAttribution: steinmb commentedComment #90
steinmb CreditAttribution: steinmb commentedPSR-4 fix
Comment #92
steinmb CreditAttribution: steinmb commentedHow about this?
Comment #93
steinmb CreditAttribution: steinmb commentedbefore:
After:
Drupal test run
Comment #94
star-szrExtra space between @param and array in these spots.
This should probably just be "The pluralized test summary items." https://drupal.org/node/1354#return
s/concatinated/concatenated/
These are missing minus signs before each available variable (to make it a list per https://drupal.org/node/1354#lists).
Comment #95
star-szrAnd thanks for the recent work on this @steinmb @joelpittet :) it is getting late so I don't mean to be curt.
Comment #96
amitgoyal CreditAttribution: amitgoyal commentedPlease review attached patch for fixes in #94.
Comment #97
star-szrThanks @amitgoyal!
Spotted one more :)
s/debuged/debugged/
Comment #98
amitgoyal CreditAttribution: amitgoyal commentedThanks for your feedback @Cottser. Please review attached patch for fixes in #97.
Comment #99
star-szrThere were two instances to be changed (forgot the /g flag ;)). The string in the Twig template should be updated as well. Thanks!
Comment #100
amitgoyal CreditAttribution: amitgoyal commentedThe string in the Twig template has also been updated as per #99. :)
Please review.
Comment #101
star-szrThe docs still weren't sitting well with me, sorry to jump in but I think this version is easier to understand and scan (see interdiff). This is RTBC from my perspective now, and only docs have changed since the previous RTBC so I'm going for it. Did another manual test myself and it checks out.
Thanks @amitgoyal for all the recent updates!
Comment #102
alexpottCommitted a6eb26d and pushed to 8.x. Thanks!