Problem/Motivation

Views default options are not translated at runtime, they contain English strings verbatim. This causes problems in two ways:

1. When the view does not include the default options (such as some pager options in views.view.content.yml), the runtime options are used and those are never translated despite the view possibly being displayed in other languages. While the options saved into the view are translatable via config translation (and/or locale in case of shipped config), settings that are not exported to views will fall back on the runtime collected defaults. These need to use t() to be translated properly at time of display.

2. When creating a new view in a foreign language (such as a German only site), the default options are expected to appear and be saved in that language. That also requires the default options to be t()-ed.

Proposed resolution

Make default options in views t()-ed when they are translatable human readable text. Add tests for the pager case that prompted this issue.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

The defaultOptions() expected return value slightly changes. Will not be always English anymore.

Files: 
CommentFileSizeAuthor
#42 2424445-42-complete.patch16.34 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,706 pass(es). View
#42 2424445-42-test.patch6.23 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,701 pass(es), 13 fail(s), and 0 exception(s). View
#42 interdiff.txt604 bytesgeertvd
#39 2424445-39-complete.patch16.34 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,704 pass(es), 5 fail(s), and 0 exception(s). View
#39 2424445-39-test.patch6.23 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,691 pass(es), 14 fail(s), and 0 exception(s). View
#39 interdiff.txt3.31 KBgeertvd
#38 2424445-38.patch13.81 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,711 pass(es). View
#24 drupal-wrong-translation-in-pager-templates-2424445-24-complete.patch4.26 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,455 pass(es). View
#24 drupal-wrong-translation-in-pager-templates-2424445-24-test.patch3.7 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,312 pass(es), 9 fail(s), and 0 exception(s). View
#11 drupal-wrong-translation-in-pager-templates-2424445-11-complete.patch7.71 KBgeertvd
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,211 pass(es). View
#11 drupal-wrong-translation-in-pager-templates-2424445-11-test.patch3.4 KBgeertvd
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,268 pass(es), 4 fail(s), and 0 exception(s). View
#7 drupal-wrong-translation-in-pager-templates-2424445-7.patch4.31 KBdobrzyns
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,704 pass(es). View
#6 pagers-patched-stark.jpg548.7 KBdobrzyns
#6 pagers-patched-bartik.jpg340.57 KBdobrzyns
#6 pagers-patched-classy.jpg476.75 KBdobrzyns
#6 pagers-patched-seven.jpg526.31 KBdobrzyns
#6 pager-seven.jpg387.27 KBdobrzyns
#6 pager-classy.jpg371.82 KBdobrzyns
#6 pager-bartik.jpg304.37 KBdobrzyns
#6 pager-stark.jpg500.76 KBdobrzyns
#5 translation-settings.jpg478.67 KBdobrzyns
#5 pager-not-translated.jpg374.47 KBdobrzyns
#5 pager-translated.jpg480.21 KBdobrzyns
#3 2424445-3.patch4.31 KBrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,706 pass(es). View
Selection_013.png7.77 KBrpayanm
Selection_014.png34.42 KBrpayanm

Comments

rpayanm’s picture

Issue summary: View changes
rpayanm’s picture

Title: Problem with t function in twig templates » Problem with translation

Seem that not is a problem with t() is for items.next.text's values isn't translatable I think.

Note: Edit the title when finds the error.

rpayanm’s picture

Title: Problem with translation » Wrong translation in pager.html.twig
Status: Active » Needs review
FileSize
4.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,706 pass(es). View

I saw where is the problem, review this patch, please.

jmauro8ac’s picture

Issue tags: +LatinAmerica2015

I'm at drupalcon latinamerica with dobrzyns and we are going to manually test this patch

dobrzyns’s picture

Issue summary: View changes
FileSize
480.21 KB
374.47 KB
478.67 KB

Without the patch applied, the pager string is sometimes getting translated and sometimes not getting translated.

Translated without patch applied
Translated

Not translated without patch applied
Not Translated

We translated next › to siguente ›:
Translation Settings

We are going to investigate why and when this happening.

dobrzyns’s picture

Without the patch, we have identified that translation is working with all core themes on a non-views pager but does not work on views pager. We added some text to core/modules/system/templates/pager.html.twig to verify when the template is being used. All core themes (views and non-views pagers) use the core/modules/system/templates/pager.html.twig template.

The manually applied patch fixes the translation on views pager without breaking it on the non-views pager, as shown in the screenshots below.

Bartik
Pagers Patched Bartik

Classy
Pagers Patched Classy

Seven
Pagers Patched Seven

Seven
Pagers Patched Stark

dobrzyns’s picture

FileSize
4.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,704 pass(es). View

The patch filename does not following the naming standards (https://www.drupal.org/node/707484). I have renamed the file to follow the naming standards; there are no changes to the code.

rpayanm’s picture

@dobrzyns thank you for manual test.

Do you hit "Clear cache"?. Remember after of apply the patch you must "Clear cache".

Too try with a pager outside of admin section, example: a frontpage with more than 10 articles.

RachFrieee’s picture

Issue summary: View changes

I was at the DrupalCon Latin America Sprints and I picked an issue, read the summary, saw duplicate images that I noticed were inserted multiple times and this is a problem that I could fix. It was confusing to me. This is my first comment on any core issue. :) yay!

Cottser’s picture

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

Patch makes sense, but it would be great if we could add tests here. Thanks all, great find and great investigation :)

geertvd’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,268 pass(es), 4 fail(s), and 0 exception(s). View
7.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,211 pass(es). View

Cottser’s picture

Awesome, thanks @geertvd! Here's a quick review, overall that looks good to me :)

  1. +++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
    @@ -28,7 +29,14 @@ class PagerTest extends PluginTestBase {
    +  public static $modules = array('node', 'views_ui', 'language', 'locale');
    

    We should only enable the 'language' and 'locale' modules for testPagerMultilingual(), otherwise it is probably slowing down all the other test methods in this test class.

  2. +++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
    @@ -323,4 +331,58 @@ function testPagerApi() {
    +    // labels that need translations.
    

    Capitalize "labels" here per https://www.drupal.org/node/1354#general.

  3. +++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
    @@ -323,4 +331,58 @@ function testPagerApi() {
    +    // We create 11 nodes, because the default pager plugin had 5 items per page.
    

    Minor: The period puts this line over 80 characters, please wrap (same reference as the above).

Cottser’s picture

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

Status and tags. This doesn't seem to want to take so adding a comment.

geertvd’s picture

FileSize
1.65 KB
3.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,263 pass(es), 4 fail(s), and 0 exception(s). View
7.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,233 pass(es). View

Fixes based on feedback in #13

geertvd’s picture

Status: Needs work » Needs review

pjbaert’s picture

Status: Needs review » Reviewed & tested by the community

It looks like the remarks of #13 are implemented correctly.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/templates/pager.html.twig
@@ -40,7 +40,7 @@
-            <span aria-hidden="true">{{ items.first.text|default('« first'|t) }}</span>
+            <span aria-hidden="true">{{ items.first.text|default('« first')|t }}</span>

@@ -49,7 +49,7 @@
-            <span aria-hidden="true">{{ items.previous.text|default('‹ previous'|t) }}</span>
+            <span aria-hidden="true">{{ items.previous.text|default('‹ previous')|t }}</span>

@@ -82,7 +82,7 @@
-            <span aria-hidden="true">{{ items.next.text|default('next ›'|t) }}</span>
+            <span aria-hidden="true">{{ items.next.text|default('next ›')|t }}</span>

@@ -91,7 +91,7 @@
-            <span aria-hidden="true">{{ items.last.text|default('last »'|t) }}</span>
+            <span aria-hidden="true">{{ items.last.text|default('last »')|t }}</span>

+++ b/core/themes/classy/templates/system/pager.html.twig
@@ -40,7 +40,7 @@
-            <span aria-hidden="true">{{ items.first.text|default('« first'|t) }}</span>
+            <span aria-hidden="true">{{ items.first.text|default('« first')|t }}</span>

@@ -49,7 +49,7 @@
-            <span aria-hidden="true">{{ items.previous.text|default('‹ previous'|t) }}</span>
+            <span aria-hidden="true">{{ items.previous.text|default('‹ previous')|t }}</span>

@@ -82,7 +82,7 @@
-            <span aria-hidden="true">{{ items.next.text|default('next ›'|t) }}</span>
+            <span aria-hidden="true">{{ items.next.text|default('next ›')|t }}</span>

@@ -91,7 +91,7 @@
-            <span aria-hidden="true">{{ items.last.text|default('last »'|t) }}</span>
+            <span aria-hidden="true">{{ items.last.text|default('last »')|t }}</span>

But isn't the use of default() here wrong. The whole point of default is that it takes two arguments?

Looking at the docblock for default

// The '_default' filter is used internally to avoid using the ternary operator
// which costs a lot for big contexts (before PHP 5.4).

Considering we are using PHP5.4 maybe <span aria-hidden="true">{{ items.last.text ?: 'last »'|t ) }}</span> is acceptable.

There are also two instances of this pattern not fixed in views-mini-pager.html.twig

alexpott’s picture

Also won't items.last.text|default('last »')|t run items.last.text through t() - which is definitely not the desired outcome.

alexpott’s picture

But isn't the use of default() here wrong. The whole point of default is that it takes two arguments?

This is wrong - see http://twig.sensiolabs.org/doc/filters/default.html but the point in #20 i think still stands - we should not be running items.last.text through the t filter.

geertvd’s picture

I think the templates can actually stay as they were.

If we take the content overview for example:

The pager configuration in view.view.content.yml looks like this:

      pager:
        type: full
        options:
          items_per_page: 50

So the "‹ previous", 'next ›', '« first' and 'last »' string are not included there.
Since it's not included in the .yml file this is not being picked up by config_translation.

I wouldn't expect this to be a problem since now the items.last.text, items.first.text, etc. variables should be empty but they are not because they are being set in Drupal\views\Plugin\views\pager\Full

$options['tags']['contains']['first'] = array('default' => '« first');
$options['tags']['contains']['last'] = array('default' => 'last »');

and then later passed to the render function

public function render($input) {
    // The 0, 1, 3, 4 indexes are correct. See the template_preprocess_pager()
    // documentation.
    $tags = array(
      0 => $this->options['tags']['first'],
      1 => $this->options['tags']['previous'],
      3 => $this->options['tags']['next'],
      4 => $this->options['tags']['last'],
    );
    return array(
      '#theme' => $this->themeFunctions(),
      '#tags' => $tags,
      '#element' => $this->options['id'],
      '#parameters' => $input,
      '#quantity' => $this->options['quantity'],
    );
  }

So in the end items.last.text will contain "last »" which is not picked up by config_translation since it's not in the .yml file and it's not picked up by interface translation since doesn't pass through the t filter.

So how should we fix this?
We can just add the strings to the .yml files like this:

pager:
        type: full
        options:
          items_per_page: 50
          tags:
            previous: '‹ previous'
            next: 'next ›'
            first: '« first'
            last: 'last »'

Then we need to add this for all views which are using pager though which seems a bit excessive.

Should we not take care that when the strings are not set in the .yml file they are also not passed to the template and then the default filter can take care of it?

geertvd’s picture

I just checked all .yml files that use pager, seems like view.view.content.yml was actually the only one not adding the string in the .yml file (ignoring test views).
I'm working on a patch and updating my test now.

geertvd’s picture

FileSize
3.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,312 pass(es), 9 fail(s), and 0 exception(s). View
4.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,455 pass(es). View

No interdiff since this is almost a complete rewrite of the original patch

geertvd’s picture

Status: Needs work » Needs review

Cottser’s picture

@geertvd - thanks for keeping on this! Would the new patch also pass the test in #15?

geertvd’s picture

I don't think the test in #15 is relevant anymore, I would have to change the test view and wouldn't be ably to give a failing test once that is done.

Cottser’s picture

The reason I asked is because I think there is still an issue where the default/fallback is not translatable. But based on the most recent comments it sounds like that should maybe be spun off into a separate issue. Thoughts?

geertvd’s picture

Yea that's what I'm mentioning here also

Should we not take care that when the strings are not set in the .yml file they are also not passed to the template and then the default filter can take care of it?

I also agree this should be a separate issue because I think the same thing happens in exposed_form configuration and maybe in some other places.

Cottser’s picture

tl;dr: I don't think we need to create any issue around using the t filter inside of the default filter. This is entirely about translation and the incoming variables to the Twig template, there is nothing we can do in Twig/the theme layer to make this better.

The Twig template itself and the use of |t inside the default filter seems to be working fine, so something else is happening here, completely unrelated to Twig and the theme system. For example:

  1. Add this to the top of Bartik's page.html.twig:
    {{ fake_var|default('Hello'|t) }}
    
  2. Enable the Interface Translation module.
  3. Add a new language at /admin/config/regional/language (I used French).
  4. Visit /fr.
  5. Navigate to /admin/config/regional/translate and search for "Hello".
  6. Enter a translation of "Allo" for "Hello".
  7. Navigate to /fr again - you should see "Allo" at the top of the page.

After completing the steps above, you can navigate to /admin/content, and {{ kint() }} from Classy's pager.html.twig (Kint is a submodule of Devel). You'll see that items.next.text, items.last.text, etc. all have data in them (but if you compare to the homepage, the text is not translated), so the default filter is not kicking in for either the homepage or /admin/content.

The reason why it works on the homepage is because the text gets from the YAML to the translation system, not because |default('foo'|t) doesn't work.

Cottser’s picture

Title: Wrong translation in pager.html.twig » Content view missing translatable strings for pager elements
Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC

Since this kinda touches views, tagging. Latest patch looks good to me, I'm not sure if there is a "deeper" fix.

alexpott’s picture

So what are we actually fixing here?

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/node/config/install/views.view.content.yml
@@ -37,6 +37,11 @@ display:
+          tags:
+            previous: '‹ previous'
+            next: 'next ›'
+            first: '« first'
+            last: 'last »'

It seems like adding the defaults fixes the issue. Aren't other views broken?

geertvd’s picture

I checked if any other views are missing those pager defaults, I'm only finding testviews. Should we add the defaults there also?

Gábor Hojtsy’s picture

#2446431: Views default options are not translated when creating a foreign language view makes the default options translated too, so even without fixing the view, the test would pass. Maybe we wan bring this test over to there or to be more fair with commit credits, move the fix from there to here?

geertvd’s picture

I'm fine with either really, probably more fair to move it here since quite some people have been working on this.

I'm thinking test coverage in #24 and in #15 are now both relevant (one uses interface translation the other config_translation) so should we just add both of them to the patch?

Gábor Hojtsy’s picture

FileSize
13.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,711 pass(es). View

Merging in #2446431: Views default options are not translated when creating a foreign language view here then, marked that as duplicate. The current test does not really test that. Testing views without the default value with interface translation would do it, so indeed should have the prior interface translation based test added back too.

geertvd’s picture

FileSize
3.31 KB
6.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,691 pass(es), 14 fail(s), and 0 exception(s). View
16.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,704 pass(es), 5 fail(s), and 0 exception(s). View

Added the test from #15 back to the patch.

The last submitted patch, 39: 2424445-39-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 39: 2424445-39-complete.patch, failed testing.

geertvd’s picture

Status: Needs work » Needs review
FileSize
604 bytes
6.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,701 pass(es), 13 fail(s), and 0 exception(s). View
16.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,706 pass(es). View

The last submitted patch, 42: 2424445-42-test.patch, failed testing.

Gábor Hojtsy’s picture

Title: Content view missing translatable strings for pager elements » Views default options such as pager options are not translated
Issue summary: View changes
Issue tags: -language-content +language-config, +sprint

Updated issue summary and title.

Gábor Hojtsy’s picture

BTW patch looks good to me, but I have significant contributions in terms of the views changes, so cannot RTBC.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I did some little research and we indeed localized those default values in Drupal 6 and 7 already:

  function _set_option_defaults(&$storage, $options, $level = 0) {
    foreach ($options as $option => $definition) {
      // ...
      elseif (!empty($definition['translatable']) && !empty($definition['default'])) {
        $storage[$option] = t($definition['default']);
      }
      else {
        $storage[$option] = isset($definition['default']) ? $definition['default'] : NULL;
      }
    }
  }

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks like all of Alex's feedback has been addressed. Great work, folks!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed e3003f6 on 8.0.x
    Issue #2424445 by geertvd, dobrzyns, rpayanm, Gábor Hojtsy, Cottser,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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