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.
Issue #2151119 by joelpittet, Cottser, idflood, InternetDevels, c4rl, IshaDakota, pplantinga, gnuget, longwave, jeanfei, sbudker1: Convert theme_system_themes_page() to Twig
Task
Convert theme_system_themes_page() to a Twig template.
Remaining tasks
Patch- Patch needs another review
Manual testingProfiling
Steps to test
Navigate to /admin/appearance.
Comment | File | Size | Author |
---|---|---|---|
#26 | 2151119-twig-theme_system_themes_page-22-reroll.patch | 9.32 KB | joelpittet |
#24 | theme_system_themes_page-patched-normalized.txt | 25.84 KB | joelpittet |
#22 | interdiff-2151119-20-22.txt | 818 bytes | jamesrutherford |
#22 | twig-theme_system_themes_page-2151119-22.patch | 9.36 KB | jamesrutherford |
#19 | interdiff.txt | 7.37 KB | star-szr |
Comments
Comment #1
star-szrAdding a commit message to the issue summary so the folks who already worked on #1987410: [meta] system.module - Convert theme_ functions to Twig and #1898454: system.module - Convert PHPTemplate templates to Twig (comment #41 and above aka @c4rl) get credit.
Comment #2
joelpittetSplit from the system module twig conversion meta.
Comment #4
joelpittettypo fix.
Comment #5
star-szrThanks @joelpittet!
Comment #6
ipo4ka704 CreditAttribution: ipo4ka704 commented4: 2151119-4-twig-theme_system_themes_page.patch queued for re-testing.
Comment #8
joelpittetRe-roll and leave the form alter were it was.
Comment #9
star-szrTagging for reroll.
Comment #10
idflood CreditAttribution: idflood commentedReroll of path in #8.
Comment #11
idflood CreditAttribution: idflood commentedHere is the profiling of the patch in #10.
I've also quickly checked visually and the markup of admin/appearance . Seems to be fine but I did not check for the case where a theme doesn't have a screenshot.
Comment #12
InternetDevels CreditAttribution: InternetDevels commentedFixed comments standards in the previous patch.
Comment #14
star-szrThanks @InternetDevels, we weren't changing/adding that line so we could just as well leave it as is. We lost the .html.twig template from the patch though, so that needs to be added back :)
Comment #15
InternetDevels CreditAttribution: InternetDevels commentedThanks @Cottser, for the answer.
Oops, we really missed it. Added it back ;)
Comment #16
star-szrComment #17
idflood CreditAttribution: idflood commentedDid some manual testing and everything works as expected.
The patch looks good and the markup also doesn't have any significant difference.
Comment #18
star-szrThanks @idflood! I'm reviewing this and it's not quite ready yet, patch coming a bit later.
Comment #19
star-szrSummary of changes:
I compared markup again and it looks good, including the no screenshot case.
Comment #20
joelpittetThe double negatives are hard to read in code. Just swap the blocks and remove 'not'.
Otherwise I like these changes alot:)
Comment #21
star-szrHehe, agreed! It felt a bit wrong.
Tagging as a good task for a new contributor to make the change suggested in #20 and post a new patch and interdiff.
Comment #22
jamesrutherford CreditAttribution: jamesrutherford commentedThanks for identifying an easy new contributor task! Here is the new patch and interdiff. Setting to needs review.
Comment #23
star-szrChanges look good, thanks @jamesrutherford!
Comment #24
joelpittetBack to RTBC, did another markup review and it's looking good. Attached are the before and after markup and the normalized versions for the screenshot and easier review.
Comment #25
webchickShoot. Really sorry to say, but this one no longer applies. Can we get a quick re-roll?
Comment #26
joelpittetQuick re-roll as requested:)
Comment #27
webchickActually, sorry.. #22 applied just fine with patch -p1.
Committed and pushed to 8.x. Thanks!