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.
Comment | File | Size | Author |
---|---|---|---|
#42 | 2424445-42-complete.patch | 16.34 KB | geertvd |
#42 | 2424445-42-test.patch | 6.23 KB | geertvd |
#42 | interdiff.txt | 604 bytes | geertvd |
#39 | 2424445-39-complete.patch | 16.34 KB | geertvd |
#39 | 2424445-39-test.patch | 6.23 KB | geertvd |
Comments
Comment #1
rpayanmComment #2
rpayanmSeem 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.
Comment #3
rpayanmI saw where is the problem, review this patch, please.
Comment #4
jmauro8ac CreditAttribution: jmauro8ac commentedI'm at drupalcon latinamerica with dobrzyns and we are going to manually test this patch
Comment #5
dobrzyns CreditAttribution: dobrzyns commentedWithout 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.
Comment #6
dobrzyns CreditAttribution: dobrzyns commentedWithout 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
Comment #7
dobrzyns CreditAttribution: dobrzyns commentedThe 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.
Comment #8
rpayanm@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.
Comment #9
RachFrieee CreditAttribution: RachFrieee commentedI 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!
Comment #10
star-szrPatch makes sense, but it would be great if we could add tests here. Thanks all, great find and great investigation :)
Comment #11
geertvd CreditAttribution: geertvd commentedComment #13
star-szrAwesome, thanks @geertvd! Here's a quick review, overall that looks good to me :)
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.
Capitalize "labels" here per https://www.drupal.org/node/1354#general.
Minor: The period puts this line over 80 characters, please wrap (same reference as the above).
Comment #14
star-szrStatus and tags. This doesn't seem to want to take so adding a comment.
Comment #15
geertvd CreditAttribution: geertvd commentedFixes based on feedback in #13
Comment #16
geertvd CreditAttribution: geertvd commentedComment #18
pjbaertIt looks like the remarks of #13 are implemented correctly.
Comment #19
alexpottBut 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
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
Comment #20
alexpottAlso won't
items.last.text|default('last »')|t
runitems.last.text
through t() - which is definitely not the desired outcome.Comment #21
alexpottThis 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 thet
filter.Comment #22
geertvd CreditAttribution: geertvd commentedI 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:
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\Fulland then later passed to the render function
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:
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?
Comment #23
geertvd CreditAttribution: geertvd commentedI 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.
Comment #24
geertvd CreditAttribution: geertvd commentedNo interdiff since this is almost a complete rewrite of the original patch
Comment #25
geertvd CreditAttribution: geertvd commentedComment #27
star-szr@geertvd - thanks for keeping on this! Would the new patch also pass the test in #15?
Comment #28
geertvd CreditAttribution: geertvd commentedI 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.
Comment #29
star-szrThe 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?
Comment #30
geertvd CreditAttribution: geertvd commentedYea that's what I'm mentioning here also
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.
Comment #31
star-szrtl;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: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.Comment #32
star-szrSince this kinda touches views, tagging. Latest patch looks good to me, I'm not sure if there is a "deeper" fix.
Comment #33
alexpottSo what are we actually fixing here?
Comment #34
alexpottIt seems like adding the defaults fixes the issue. Aren't other views broken?
Comment #35
geertvd CreditAttribution: geertvd commentedI checked if any other views are missing those pager defaults, I'm only finding testviews. Should we add the defaults there also?
Comment #36
Gábor Hojtsy#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?
Comment #37
geertvd CreditAttribution: geertvd commentedI'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?
Comment #38
Gábor HojtsyMerging 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.
Comment #39
geertvd CreditAttribution: geertvd commentedAdded the test from #15 back to the patch.
Comment #42
geertvd CreditAttribution: geertvd commentedComment #44
Gábor HojtsyUpdated issue summary and title.
Comment #45
Gábor HojtsyBTW patch looks good to me, but I have significant contributions in terms of the views changes, so cannot RTBC.
Comment #46
dawehnerI did some little research and we indeed localized those default values in Drupal 6 and 7 already:
Comment #47
webchickLooks like all of Alex's feedback has been addressed. Great work, folks!
Committed and pushed to 8.0.x. Thanks!
Comment #49
Gábor HojtsyComment #51
Max_Z. CreditAttribution: Max_Z. as a volunteer commentedThe 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
Comment #52
boby_ui CreditAttribution: boby_ui as a volunteer commented+1 yes its not working anymore since jan 2020
Comment #53
cheng75 CreditAttribution: cheng75 as a volunteer and at Google Code-In commentedIt is not working for me either .