Views default options such as pager options are not translated

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

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

Translated without patch applied

Not translated without patch applied

We translated next › to siguente ›:

We are going to investigate why and when this happening.

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

Classy

Seven

Seven

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.

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!

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

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

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

Fixes based on feedback in #13

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

+++ 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

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

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.

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?

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.

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

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

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.

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?

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.

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.

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

So what are we actually fixing here?

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

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

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

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?

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.

Added the test from #15 back to the patch.

Updated issue summary and title.

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

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;
      }
    }
  }

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

Committed and pushed to 8.0.x. Thanks!

The issue is not resolved (July 2018) and the patch is no longer working.
There is no such file there anymore:

core/modules/node/config/install/views.view.content.yml

+1 yes its not working anymore since jan 2020

It is not working for me either .