There is a problem with the Views UI when used with multilingual Drupal sites. If a view is accessed in another language then the default language the Views UI will load the translated strings if available. So far so good but when hitting the Save button those translated strings will overwrite the original strings. Thus the view in default langue now contains translated strings from the translated language.

Steps to reproduce:

1. Set up a view in a view in a multilingual Drupal, in my example I will use German (de) as default and English (en) as additional language.
2. Create a view in default language and translate a few strings into an additional language and save. E.g. the title of the View.
3. Open the View UI in the additional language, in our case English. e.g. http://[drupal_root]/en/admin/structure/views/view/[view_name]/
4. Note that views UI opens the view with the Title translated and save the view.
5. Open the view in it's default language (e.g. http://[drupal_root]/de/admin/structure/views/view/[view_name]/) and note that the original Title has been overridden.

The question arises how Views UI should handle multilingual content. Should it remember that strings are translated and save those correctly or should it always load the original strings from the beginning?

A similar situation would be the translation of the Title Label of a Content Type. In the content type UI Drupal will always load the Title Label in the content types original language no matter what language you access the UI in. Thus anything related to translation are keep separately over at translations only.

Suggested resolution:
1. Remove the translation of strings in the Views UI.

Comments

hauruck created an issue. See original summary.

hauruck’s picture

Issue summary: View changes
hauruck’s picture

Title: Translated string are loaded into display and then saved as original strings. » Translated string are loaded into Views UI and then saved as original strings.
hauruck’s picture

Title: Translated string are loaded into Views UI and then saved as original strings. » Translated string are loaded into Views UI and then saved as default language strings.
webflo’s picture

Version: 8.0.0-rc2 » 8.0.x-dev
Issue tags: +D8MI, +VDC
webflo’s picture

Issue tags: +language-config
Gábor Hojtsy’s picture

Does this apply to other things than views? Menus, content types, field settings? I would suspect so.

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +sprint
hauruck’s picture

Issue summary: View changes

Content type, Field Settings & Menus: Strings will always load into the UI in default language no matter wheat language the UI is loaded in, thus not having problems with overwriting.

hauruck’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Admin pages are supposed to load the entity in its original language without translations, they should let you edit that and save that. It is not supposed to load translated values and it is not supposed to let you edit them on the original UI (unless the original config itself is translated of course). The behavior should be the same across fields, content types, input formats, menus, views, whatever, it should not be different.

xjm’s picture

Priority: Major » Critical

The core committers and Views maintainers (alexpott, xjm, effulgentsia, tim.plunkett, and dawehner) agreed that this actually is potentially a critical issue, because it causes data loss. Promoting to critical for that reason. We should test and confirm the bug still exists as described.

dawehner’s picture

So I'm wondering what is the mechanism that config entities aren't translated on admin forms by default?

catch’s picture

@dawehner that's implemented in ConfigFormBaseTrait

Gábor Hojtsy’s picture

Also, config entities are upcast without translations on admin routes by default, see AdminPathConfigEntityConverter, which should ensure you get the original values for editing.

tim.plunkett’s picture

Issue tags: +Needs tests

I'm guessing \Drupal\views_ui\ParamConverter\ViewUIConverter and \Drupal\Core\ParamConverter\AdminPathConfigEntityConverter are incompatible with each other.

Gábor Hojtsy’s picture

Yeah looks like the view converter is higher priority.

  paramconverter.configentity_admin:
    class: Drupal\Core\ParamConverter\AdminPathConfigEntityConverter
    tags:
      # Use a higher priority than EntityConverter, see the class for details.
      - { name: paramconverter, priority: 5 }
    arguments: ['@entity.manager', '@config.factory', '@router.admin_context']
    lazy: true

  paramconverter.views_ui:
    class: Drupal\views_ui\ParamConverter\ViewUIConverter
    arguments: ['@entity.manager', '@user.shared_tempstore']
    tags:
      - { name: paramconverter, priority: 10 }
    lazy: true

From the docs on AdminPathConfigEntityConverter:

 * Due to this converter having a higher weight than the default
 * EntityConverter, every time this applies, it takes over the conversion duty
 * from EntityConverter. As we only allow a single converter per route
 * argument, EntityConverter is ignored when this converter applies.

So indeed Views takes over the admin converter. Should it sit between the two? :)

Gábor Hojtsy’s picture

Discussed with @dawehner, he says we should look at inheriting the views converter from the admin converter. Looking at this, it does not seem trivial, especially given it comes with API changes to how the views converter is instantiated (if we consider that an API that is).

Berdir’s picture

Status: Active » Needs review
FileSize
2.72 KB

According to #2550249: [policy, then meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation, we don't unless it's a base class, which I think this isn't.

But, it fails pretty hard if you do, since this is instantiated on every request. I added a workaround for that by making the new arguments optional and get them from the container if they're missing. Then the site continues to work after applying the patch.

We could add a @todo to remove it again and make them required later on. (not sure if later is 8.1.x or 9.x)

Still needs tests.

Gábor Hojtsy’s picture

I don't think this patch solves the problem yet? It does not change the applies() method or the conversion itself?

maxocub’s picture

FileSize
2.24 KB

I'm trying to write a test but I can't reproduce the bug with a test. I can easily reproduce it manually, but not with a test. I first save a translation for the label of a view, then I visit the edit page in the second language, but the label is not displayed translated. Does someone knows why? Did I forget something in the test?

Berdir’s picture

@maxocub: You seem to assert for the wrong behavior now in your test? You should write it so that it should pass when being correct. There might be a problem in how the URL is built, I'll check. You should also upload a patch as .patch, not .txt, also if it doesn't work yet.

@Gabor: I haven't tried it yet, but IMHO, it should just work. All the relevant logic is in the parent, the actual code is just about getting it from tempstore instead. But if we ensure that we get the default translation, that should continue to work in the same way.

maxocub’s picture

@Berdir: Yes, I know I'm not asserting for the right thing and that a .txt will not trigger the testbot, this is just a work in progress, I uploaded it just to show where I was going.
I'm trying to load the edit page in the second language to be able to save the view and check if the translated string overrides the original value.
Right now, the label is not translated on the edit page (That's what I'm trying to show with the last assert) so I can't test if it overrides the orignal one.
You might be right with how the URL is built, but I can't find why.

Berdir’s picture

Manually tested now, my patch does seem to work.

The test also seems to work for me, more or less. The asserts were a bit confusing because it used assertLink() but the page title isn't a link.

I changed it slightly to use assertTitle() with the full, explicit title, now it fails/passes as expected. Interdiff is against the two patches combined.

Berdir’s picture

tim.plunkett’s picture

+++ b/core/modules/views_ui/src/ParamConverter/ViewUIConverter.php
@@ -47,8 +49,17 @@ class ViewUIConverter extends EntityConverter implements ParamConverterInterface
+    // The config factory and admin context are new arguments due to changing
+    // the parent. Avoid an error on updated sites by falling back to getting
+    // them from the container.
+    if (!$config_factory) {
+      $config_factory = \Drupal::configFactory();
+    }
+    if (!$admin_context) {
+      $admin_context = \Drupal::service('router.admin_context');
+    }

Is this really something we do now? :(

Berdir’s picture

I think it is worth doing. It doesn't hurt unit testing (which we don't have anyway for that class) in any way and avoids ugly exceptions when anyone access the site while updating. The class is instantiated on every request, not just in the views backend, then I could totally live with errors.

Should help to get it into a patch release.

Status: Needs review » Needs work

The last submitted patch, 25: views-ui-converter-2602268-24-test-only.patch, failed testing.

tim.plunkett’s picture

Can we get an @todo to clean it up eventually? Or better yet, a separate 8.1.x patch without that, assuming it goes into 8.0.x like this?

catch’s picture

Status: Needs work » Needs review

An 8.1.x issue to clean this up would be good. But definitely better to have a temporary ugly hack than an ugly fatal error on every request after a patch update.

Only CNW 'cos the bot liked the test-only patch. Moving back to CNR but for me I can't see anything wrong here, will let someone else RTBC it if they agree.

catch’s picture

One issue with an 8.1.x issue is that if someone upgrades direct from 8.0.3 or whatever they'd still get the error, but that should probably be explicitly unsupported for reasons like this.

Gábor Hojtsy’s picture

@Berdir: thanks for the explanation, I did not check how do they interoperate properly.

@catch: so looks like we should have a @todo (if the policy does not yet comply with the assumption that it should not be supported to update from earlier patch releases to a minor) or a 8.1 patch if the policy is already that and we are fine with a fatal there.

I think either a @todo or a separate 8.1 patch is needed for this to be RTBC? I assume it is not OK to fix this in 8.0.x and not in 8.1.x at once.

catch’s picture

I'd go for a new issue for 8.1.x changes, whether it has an associated @todo or not I don't mind too much - there's always git blame pointing to this issue pointing to that issue.

Also if the 8.1.x patch didn't happen but got into 8.2.x instead, that'd solve the 8.1.0 upgrade path question I had.

Berdir’s picture

Status: Needs review » Needs work

Ok, then lets do exactly that? A @todo and follow-up for *8.2.x* to remove that again?

dawehner’s picture

+++ b/core/modules/views_ui/src/Tests/TranslatedStringsTest.php
@@ -0,0 +1,89 @@
+ */
+class TranslatedStringsTest extends WebTestBase {

Let's provide a better testclass name, this one isn't really clear. What about using TranslatedViewTest ?

Berdir’s picture

Changed the class name, added the todo and opened #2674328: Remove BC code in ViewUIConverter

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Amazing, thanks all!

dawehner’s picture

Nice!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 6109cb9 on 8.1.x
    Issue #2602268 by Berdir: Translated string are loaded into Views UI and...

  • catch committed 5241d77 on 8.0.x
    Issue #2602268 by Berdir: Translated string are loaded into Views UI and...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks a lot! Woot :)

Status: Fixed » Closed (fixed)

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