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 #1898428 by joelpittet, shanethehat, Floydm, gillbates, steinmb, artis, Cottser, gnuget: locale.module - Convert theme_ functions to Twig.
Task
Use Twig instead of PHPTemplate
Remaining
Theme function name/template path | Conversion status |
---|---|
theme_locale_translate_edit_form_strings | Will be removed in #type table conversion issue: #1938916: Convert locale theme tables to table #type |
theme_locale_translation_last_check | converted |
theme_locale_translation_update_info | converted |
Testing steps
- Enable the Interface Translation and Language modules.
- Navigate to admin/reports/translations to see the empty report.
- Add a new language by navigating to admin/config/regional/language.
- The available updates should be rendered by locale-translation-update-info.html.twig, and the 'last checked' information should be rendered by locale-translation-last-check.html.twig.
To test further, enable the Testing module, run the "Update translations user interface" automated test in the Locale group and examine the verbose output.
Related
#1757550: [Meta] Convert core theme functions to Twig templates
#1938916: Convert locale theme tables to table #type
Comment | File | Size | Author |
---|---|---|---|
#50 | 1898428-50.patch | 8.47 KB | gnuget |
#43 | 1898428-43.patch | 8.47 KB | star-szr |
#43 | interdiff.txt | 791 bytes | star-szr |
#31 | drupal-twig-locale-1898428-31.patch | 8.28 KB | shanethehat |
#31 | interdiff.txt | 2.33 KB | shanethehat |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
joelpittetComment #3
joelpittetComment #4
joelpittetmissed a tab to space
Comment #6
joelpittetFixes to the pager by converting theme() to renderable array from http://drupal.org/node/1920746#render
As well as the other ones I could... I left a few theme() and drupal_render's in there because I wasn't sure how to properly convert them... maybe #prefix for the renderable array?
Also fixed some bad HTML p's surrounding divs
<p><div></div></p>
Comment #7
joelpittetComment #8
star-szr@joelpittet - Thanks for all your work! Looks good so far, interdiffs are helpful for reviewers as well :)
In that case, it looks like the t() should probably be moved to the template file with an if block - see http://drupal.org/node/1920746#utility for an example (although the example doesn't use an if). In other cases, we've been creating new variables where the theme() function was previously concatenating. See #1898070-9: file.module - Convert theme_ functions to Twig for an example of that.
The "Available variables" comments in the Twig template files need to end in periods per http://drupal.org/node/1354#drupal.
I think these comments should be removed. We know we're in a preprocess function already and because we're converting to Twig we also know that the output is coming from a template.
Also, the three Twig template files need to end in a newline per http://drupal.org/coding-standards#indenting, if you review the patch in Dreditor you'll see what I mean.
Comment #9
joelpittetHere is the removal of the template variable comments, newline adds, twig docblock period and, interdiff ;)
I will leave any further refactoring of the theme() and t() functions into the twig up to someone with a better grasp of how best to do that in this circumstance. (or tell me and I will re-roll)
Comment #10
joelpittetCleaned this up a bit more
Comment #11
jenlamptonA few questions & suggestions :)
Do we really need to unset this? Let's not unset anything unnecessarily.
Can we turn this into a render array somehow? (separate variable maybe? $variables['detail_items'] ?)
I think most of this text can move safely into the template. Let's do something like...
and inTwig
Do we really need to unset this? Let's not unset anything unnecessarily.
Comment #12
joelpittetThanks @jenlampton for reviewing, I think I got all those things covered in this one and passes local tests.
Comment #13
idflood CreditAttribution: idflood commentedComment #14
tlattimore CreditAttribution: tlattimore commentedOnly did a brief review of this so far but caught this:
Shouldn't this be 'whether' not 'weather'?
Comment #15
floydm CreditAttribution: floydm commentedPatch from #12 rerolled with "Weather" changed to "Whether."
Comment #16
gillbates CreditAttribution: gillbates commentedHi! I generated an interdiff for the last patch.
Comment #16.0
star-szrAdd conversion summary table
Comment #17
star-szrTagging.
Comment #17.0
star-szrLinkify #type table issue + add to related
Comment #18
star-szrOverall this looks good, found some documentation that can be tweaked, #1 and #2 below are functional changes though. Thanks everyone!
I think printing the description could be done in the Twig template with the join filter. aggregator-item.html.twig in #1896060: aggregator.module - Convert PHPTemplate templates to Twig has an example of the join filter.
Let's leave this conversion to #1898428: locale.module - Convert theme_ functions to Twig and remove it from this patch entirely (leave theme function alone, remove 'template' line added to locale_theme(), remove locale-translate-edit-form-strings.html.twig from the patch).
All template preprocess function summary lines should end in "templates" per #1913208: [policy] Standardize template preprocess function documentation.
These lines should be wrapped at 80 characters per http://drupal.org/node/1354#drupal.
The summary line for this one is a bit too long, if needed you can add a line below to add more to the description. Could be something like this:
Comment #19
joelpittetComment #20
star-szrAt least #3 through #5 are novice-friendly :)
Comment #21
shanethehat CreditAttribution: shanethehat commentedComment #22
shanethehat CreditAttribution: shanethehat commentedI think I've covered all the points. For #2, I reverted the function to its state before the patch started. For #1, I removed the references to the description property and let twig take care of joining the $modules list, but kept the creation of the missing updates text and its call to format_plural in local.page.inc.
Comment #24
shanethehat CreditAttribution: shanethehat commentedTwig is not Ruby.
Comment #25
shanethehat CreditAttribution: shanethehat commentedComment #26
star-szrThanks @shanethehat, you definitely covered all the bases in #18. There's only a couple tweaks I can see now:
For translation purposes we'll need the t() string passed to the filter in the template file to match the t() string that was in the preprocess function. Something like the following should work (not sure if my syntax is 100%), see http://drupal.org/node/1920746 for an example as well.
Also, let's document the 'modules' filter in the .html.twig template docblock to indicate that it's a list of modules with available translation updates.
Comment #27
shanethehat CreditAttribution: shanethehat commentedComment #28
shanethehat CreditAttribution: shanethehat commentedComment #28.0
shanethehat CreditAttribution: shanethehat commentedUpdate conversion table
Comment #29
star-szrI performed manual testing, testing steps added to the summary.
Just a few minor docs tweaks and then I think this one is ready to go.
Per #1913208: [policy] Standardize template preprocess function documentation this should end in 'templates', it might make sense to break up the description. Suggested summary line:
'Prepares variables for translation status information templates.'
This should end in 'templates' as well.
Whether or not locale updates have been checked before.
Comment #30
shanethehat CreditAttribution: shanethehat commentedComment #31
shanethehat CreditAttribution: shanethehat commentedComment #32
star-szrThis looks good to me, great work @shanethehat!
Comment #32.0
star-szrAdd testing steps
Comment #33
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 #34
alexpottComment #35
jstoller#31: drupal-twig-locale-1898428-31.patch queued for re-testing.
Comment #36
johannez CreditAttribution: johannez commentedTested patch from #31 against latest core. No need to re-roll.
Comment #37
artis CreditAttribution: artis commentedstarting to profile this issue
Comment #38
steinmb CreditAttribution: steinmb commentedProfiling data
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fd788bae9c&...
admin/reports/translations
Comment #39
artis CreditAttribution: artis commentedScenario: Enable Translation and Language modules; Permissions: Translations, Language, Admin pages, view reports, configure site; Add a new language; set homepage to admin/reports/translatations; Both twig files will be displayed on this page.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe7ae1f6cc&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe7ae1f6cc&...
Comment #40
joelpittetAnother set of results from #38's scenario.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fedcae68c3&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fedcae68c3&...
Still RTBC:)
Comment #41
steinmb CreditAttribution: steinmb commentedLet's get this in then. I also posted result data in the related issue [##1938916] and we should also get that on RTBC.
Comment #41.0
steinmb CreditAttribution: steinmb commentedRemove sandbox link
Comment #42
joelpittetuntagging novice
Comment #43
star-szrAdding missing docs to locale-translation-update-info.html.twig. Improvements welcome :)
Comment #43.0
star-szrUpdated issue summary.
Comment #44
joelpittet#43 Seems clear enough to me. Back to RTBC.
Comment #46
steinmb CreditAttribution: steinmb commented#43: 1898428-43.patch queued for re-testing.
Comment #47
steinmb CreditAttribution: steinmb commentedOK, #43 came back green. Must have been something wrong with testbot. Back to RTBC.
Comment #48
alexpottNeeds a reroll
Comment #49
gnugetComment #50
gnugetDone.
The only change was:
#43
by
Patch attached.
Comment #51
joelpittetThanks @gnuget! On to profiling.
Comment #51.0
joelpittetUpdate commit message
Comment #52
star-szrThis already got profiled in #40 :)
Reroll looks great, thanks @gnuget. Added you to the commit message up top.
Comment #53
alexpottCommitted 42c1a7c and pushed to 8.x. Thanks!
Comment #54.0
(not verified) CreditAttribution: commentedAdd @gnuget to commit message