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

  1. Enable the Interface Translation and Language modules.
  2. Navigate to admin/reports/translations to see the empty report.
  3. Add a new language by navigating to admin/config/regional/language.
  4. 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.

#1757550: [Meta] Convert core theme functions to Twig templates
#1938916: Convert locale theme tables to table #type

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

joelpittet’s picture

Assigned: Unassigned » joelpittet
joelpittet’s picture

Status: Active » Needs review
FileSize
8.22 KB
joelpittet’s picture

missed a tab to space

Status: Needs review » Needs work

The last submitted patch, drupal-twig-locale-1898428-4.patch, failed testing.

joelpittet’s picture

Fixes 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?

    if ($details) {
      $details .= t('Missing translations for:');
    }
    $details .= theme('item_list', array('items' => $releases));

Also fixed some bad HTML p's surrounding divs <p><div></div></p>

joelpittet’s picture

Status: Needs work » Needs review
star-szr’s picture

Status: Needs review » Needs work

@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.

+++ b/core/modules/locale/templates/locale-translate-edit-form-strings.html.twigundefined
@@ -0,0 +1,17 @@
+ * - table: form table of translatable strings

The "Available variables" comments in the Twig template files need to end in periods per http://drupal.org/node/1354#drupal.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -844,18 +844,21 @@ function theme_locale_translate_edit_form_strings($variables) {
+  // Set the template variables

@@ -899,15 +902,13 @@ function theme_locale_translation_update_info($variables) {
+  // Set the template variables

@@ -920,11 +921,11 @@ function theme_locale_translation_update_info($variables) {
+  // Set the template variables

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.64 KB
3.11 KB

Here 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)

joelpittet’s picture

Cleaned this up a bit more

jenlampton’s picture

Status: Needs review » Needs work

A few questions & suggestions :)

+  unset($variables['form']);

Do we really need to unset this? Let's not unset anything unnecessarily.

$details .= theme('item_list', array('items' => $releases));

Can we turn this into a render array somehow? (separate variable maybe? $variables['detail_items'] ?)

+  $variables['last_check'] = $last ? t('Last checked: @time ago', array('@time' => format_interval(REQUEST_TIME - $last))) : t('Last checked: never');

I think most of this text can move safely into the template. Let's do something like...

$variables['last_check'] = $last;
$variables['time'] = format_interval(REQUEST_TIME - $last);

and inTwig

...
{% if last_check %}
  {{ 'Last checked: @time ago'|t('@time', time) }}
{% else %]
  {{ 'Last checked: never'|t }}
{% endif %}
...
+  unset($variables['last']);

Do we really need to unset this? Let's not unset anything unnecessarily.

joelpittet’s picture

Thanks @jenlampton for reviewing, I think I got all those things covered in this one and passes local tests.

idflood’s picture

Status: Needs work » Needs review
tlattimore’s picture

Status: Needs review » Needs work

Only did a brief review of this so far but caught this:

+++ b/core/modules/locale/templates/locale-translation-last-check.html.twigundefined
@@ -0,0 +1,25 @@
+ * - last_checked: Weather or not the time has been checked before.
+ * - time: The formatted time ago when the site last checked for available updates.

Shouldn't this be 'whether' not 'weather'?

floydm’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

Patch from #12 rerolled with "Weather" changed to "Whether."

gillbates’s picture

Hi! I generated an interdiff for the last patch.

star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Issue summary: View changes

Linkify #type table issue + add to related

star-szr’s picture

Status: Needs review » Needs work

Overall this looks good, found some documentation that can be tweaked, #1 and #2 below are functional changes though. Thanks everyone!

  1. +++ b/core/modules/locale/locale.pages.incundefined
    @@ -863,10 +866,10 @@ function theme_locale_translate_edit_form_strings($variables) {
    +  $description = '';
    
    @@ -877,14 +880,17 @@ function theme_locale_translation_update_info($variables) {
    +    $description = t('Updates for: @modules', array('@modules' => implode(', ', $modules)));
    
    @@ -893,21 +899,23 @@ function theme_locale_translation_update_info($variables) {
    +  $variables['description'] = $description;
    

    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.

  2. +++ b/core/modules/locale/locale.pages.incundefined
    @@ -813,17 +814,17 @@ function _locale_translation_status_debug_info($source) {
    -function theme_locale_translate_edit_form_strings($variables) {
    ...
    +function template_preprocess_locale_translate_edit_form_strings(&$variables) {
    

    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).

  3. +++ b/core/modules/locale/locale.pages.incundefined
    @@ -813,17 +814,17 @@ function _locale_translation_status_debug_info($source) {
    + * Prepares variables for translation edit form.
    

    All template preprocess function summary lines should end in "templates" per #1913208: [policy] Standardize template preprocess function documentation.

  4. +++ b/core/modules/locale/locale.pages.incundefined
    @@ -893,21 +899,23 @@ function theme_locale_translation_update_info($variables) {
    +    // Prefix the missing updates list if there is an available updates lists before it.
    
    +++ b/core/modules/locale/templates/locale-translation-last-check.html.twigundefined
    @@ -0,0 +1,25 @@
    + * - time: The formatted time ago when the site last checked for available updates.
    

    These lines should be wrapped at 80 characters per http://drupal.org/node/1354#drupal.

  5. +++ b/core/modules/locale/templates/locale-translation-update-info.html.twigundefined
    @@ -0,0 +1,24 @@
    + * Default theme implementation for the translation status information per language.
    

    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:

    Default theme implementation for displaying translation status information.
    
    Displays translation status information per language.
    
joelpittet’s picture

Assigned: joelpittet » Unassigned
Issue tags: +Novice
star-szr’s picture

At least #3 through #5 are novice-friendly :)

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Status: Needs work » Needs review
FileSize
6.9 KB
7.9 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-locale-1898428-22.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review
FileSize
7.91 KB
878 bytes

Twig is not Ruby.

shanethehat’s picture

Assigned: shanethehat » Unassigned
star-szr’s picture

Status: Needs review » Needs work
Issue tags: -Novice

Thanks @shanethehat, you definitely covered all the bases in #18. There's only a couple tweaks I can see now:

+++ b/core/modules/locale/locale.pages.incundefined
@@ -758,8 +757,8 @@ function template_preprocess_locale_translation_update_info(&$variables) {
-    $description = t('Updates for: @modules', array('@modules' => implode(', ', $modules)));

+++ b/core/modules/locale/templates/locale-translation-update-info.html.twigundefined
@@ -15,8 +17,10 @@
+    <span class="text">{{ 'Updates for'|t }}: {{ modules|join(', ') }}</span>

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.

{{ 'Updates for: @modules'|t({'@modules': modules|join(', ')}) }}

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.

shanethehat’s picture

Assigned: Unassigned » shanethehat
Issue tags: +Novice
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
1.29 KB
7.94 KB
shanethehat’s picture

Issue summary: View changes

Update conversion table

star-szr’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

I 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.

  1. +++ b/core/modules/locale/locale.pages.incundefined
    @@ -734,7 +735,9 @@ function theme_locale_translate_edit_form_strings($variables) {
    + * Prepares variables for translation status information per language.
    

    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.'

    +++ b/core/modules/locale/locale.pages.incundefined
    @@ -772,21 +775,23 @@ function theme_locale_translation_update_info($variables) {
    + * Prepares variables for the last time we checked for update data.
    

    This should end in 'templates' as well.

  2. +++ b/core/modules/locale/templates/locale-translation-last-check.html.twigundefined
    @@ -0,0 +1,26 @@
    + * - last_checked: Whether or not the time has been checked before.
    

    Whether or not locale updates have been checked before.

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
2.33 KB
8.28 KB
star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

This looks good to me, great work @shanethehat!

star-szr’s picture

Issue summary: View changes

Add testing steps

c4rl’s picture

Title: Convert locale module to Twig » locale.module - Convert theme_ functions to Twig
Issue tags: +Novice

Per #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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling
jstoller’s picture

Status: Needs work » Needs review
johannez’s picture

Tested patch from #31 against latest core. No need to re-roll.

artis’s picture

Assigned: Unassigned » artis

starting to profile this issue

steinmb’s picture

Profiling data

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fd788bae9c&...

admin/reports/translations

=== 8.x..8.x compared (519fd788bae9c..519fd80de9ca0):

ct  : 159,589|159,589|0|0.0%
wt  : 611,208|611,127|-81|-0.0%
cpu : 580,833|581,160|327|0.1%
mu  : 13,049,312|13,049,312|0|0.0%
pmu : 13,160,952|13,160,952|0|0.0%

<a href="http://d8.dev/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=admin%2Freports%2Ftranslations&run1=519fd788bae9c&run2=519fd80de9ca0&extra=8.x..8.x">Profiler output</a>

=== 8.x..1898428-31 compared (519fd788bae9c..519fd8d01971a):

ct  : 159,589|159,772|183|0.1%
wt  : 611,208|608,970|-2,238|-0.4%
cpu : 580,833|577,934|-2,899|-0.5%
mu  : 13,049,312|13,131,640|82,328|0.6%
pmu : 13,160,952|13,243,816|82,864|0.6%

<a href="http://d8.dev/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=admin%2Freports%2Ftranslations&run1=519fd788bae9c&run2=519fd8d01971a&extra=8.x..1898428-31">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
artis’s picture

Assigned: artis » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Scenario: 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.

=== 8.x..8.x compared (519fe7ae1f6cc..519fe8bb569af):

ct  : 34,211|34,211|0|0.0%
wt  : 225,800|227,030|1,230|0.5%
cpu : 206,222|205,704|-518|-0.3%
mu  : 9,619,928|9,619,928|0|0.0%
pmu : 9,797,224|9,797,224|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe7ae1f6cc&...

=== 8.x..drupal-twig-locale-1898428-31.patch compared (519fe7ae1f6cc..519fe8e1e5eca):

ct  : 34,211|34,211|0|0.0%
wt  : 225,800|226,074|274|0.1%
cpu : 206,222|205,962|-260|-0.1%
mu  : 9,619,928|9,619,944|16|0.0%
pmu : 9,797,224|9,797,160|-64|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fe7ae1f6cc&...

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review

Another set of results from #38's scenario.

=== 8.x..8.x compared (519fedcae68c3..519ff028be5c1):

ct  : 162,487|162,487|0|0.0%
wt  : 618,992|617,077|-1,915|-0.3%
cpu : 563,946|567,352|3,406|0.6%
mu  : 12,839,600|12,839,600|0|0.0%
pmu : 12,979,368|12,979,368|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fedcae68c3&...

=== 8.x..1898428-twig-locale-theme compared (519fedcae68c3..519ff0715b88b):

ct  : 162,487|162,670|183|0.1%
wt  : 618,992|620,240|1,248|0.2%
cpu : 563,946|565,424|1,478|0.3%
mu  : 12,839,600|12,922,072|82,472|0.6%
pmu : 12,979,368|13,062,616|83,248|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519fedcae68c3&...

Still RTBC:)

steinmb’s picture

Status: Needs review » Reviewed & tested by the community

Let's get this in then. I also posted result data in the related issue [##1938916] and we should also get that on RTBC.

steinmb’s picture

Issue summary: View changes

Remove sandbox link

joelpittet’s picture

Issue tags: -Novice

untagging novice

star-szr’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
791 bytes
8.47 KB

Adding missing docs to locale-translation-update-info.html.twig. Improvements welcome :)

star-szr’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

#43 Seems clear enough to me. Back to RTBC.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Twig

The last submitted patch, 1898428-43.patch, failed testing.

steinmb’s picture

Status: Needs work » Needs review
Issue tags: +Twig

#43: 1898428-43.patch queued for re-testing.

steinmb’s picture

Status: Needs review » Reviewed & tested by the community

OK, #43 came back green. Must have been something wrong with testbot. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

curl https://drupal.org/files/1898428-43.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8670  100  8670    0     0   8072      0  0:00:01  0:00:01 --:--:--  9807
error: patch failed: core/modules/locale/locale.pages.inc:546
error: core/modules/locale/locale.pages.inc: patch does not apply
gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Assigned: gnuget » Unassigned
Status: Needs work » Needs review
FileSize
8.47 KB

Done.

The only change was:

#43

   $last_checked = state()->get('locale.translation_last_checked');

by

   $last_checked = Drupal::state()->get('locale.translation_last_checked');

Patch attached.

joelpittet’s picture

Thanks @gnuget! On to profiling.

joelpittet’s picture

Issue summary: View changes

Update commit message

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

This already got profiled in #40 :)

Reroll looks great, thanks @gnuget. Added you to the commit message up top.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 42c1a7c and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Add @gnuget to commit message