Translated string are loaded into Views UI and then saved as default language strings.

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

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.

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.

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

@dawehner that's implemented in ConfigFormBaseTrait

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

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

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? :)

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

According to #2550249: [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.

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

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?

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

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

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.

Forgot the test-only patch.

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

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.

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?

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.

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.

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

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.

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

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

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

Amazing, thanks all!

Nice!

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

Thanks a lot! Woot :)