Problems with multilingual entities

Test would be nice.

A bit improved version of PR 150.

I'll work on tests.

I was looking through the code again and the patch seems very manual - I can't find examples of another Controller delivering an Entity Form and handling Translation this way. Has this issue been tested with any entity type other than Media Entities?

I ask this because forms like ContentEntityForm seem to do similar logic, which makes sense as the Entity Form in question here should be responsible for delivering the correct translation of an Entity - not Entity Browser. See ContentEntityForm::init() for what I'm referencing.

What about passing language via URL argument?

Patch no longer applies. Attaching rerolled version.

@samuel.mortenson Attaching a new, refactored version that takes your comments into account. Thanks to $form_state->set('langcode', $langcode); ContentEntityForm::init() takes over translation handling. It doesn't copy values from source entity when adding translation though, so the following part has been added to improve UX:

if (!$entity->hasTranslation($langcode)) {
  $entity = $entity->addTranslation($langcode, $entity->toArray());
}

Just tested adding this patch to my site under development and it did indeed fix the issue. Thanks.

@blazey Thanks! This patch is starting to look better.

So ContentEntityForm::init() already adds a translation, but as you mentioned does not copy values from the existing Entity. Can you explain the why using our own $entity->addTranslation call is necessary here? What behavior is different with/without that call? I'm also wondering if there's an example of a controller doing this in core, since I would expect ContentEntityForm to handle all of this.

@samuel.mortenson Thanks for review!

Without the call to $entity->addTranslation media translation form is displayed blank. Initial field values are not copied from the translation source. IMHO it's very inconvenient to select the same file every time you add a translation. There are definitely cases when you have to do that anyway (like when it's a pdf in different language) but often you just want to translate alt and title of the image and it's just easier (and more intuitive) to have the values (including the original file) prefilled.

I have no idea why ContentEntityForm creates the translation but doesn't pass field values. It would be enough to change line 171 here http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Conte... from

(...) : $this->entity->addTranslation($langcode);

to

(...) : $this->entity->addTranslation($langcode, $this->entity->toArray());

Maybe it deserves a core issue? Anyway, for the time being it might be worth adding the translation ourselves as the UX gain is huge.

@blazey You mention media translation specifically - as Entity Browser can be used with any Entity Type, would copying values to the new translation make sense for Nodes as well?

@samuel.mortenson To me it would make sense as I would like to know what I'm translating. The alternative is getting an empty form and having to look up the original entity in another browser tab.

@samuel.mortenson,

would copying values to the new translation make sense for Nodes as well?

Yes it would. Currently ContentTranslationController does this for regular node forms here: https://github.com/drupal/drupal/blob/8.3.0-alpha1/core/modules/content_...

(Attached image is a screenshot of a customized "Media file selector" (media_generic) widget we used on one of our old D7 projects. Not the most intuitive UI :D)

@slashrsm,

What about passing language via URL argument?

I agree that it would be nicer to use parent form language instead of the current content language.

So here is my attempt.
Additionally, I made that the edit form reflects the language select element value dynamically. If the language select element exists on the parent form.

What benefit is there to passing the langcode with a query parameter, instead of trusting the core functionality of prefixing routes with the langcode (i.e. /fr/*)? I've never seen modules explicitly pass the langcode like this.

Well, we can use system language prefixes/domains. But we have to be careful since language detection can be setup individually for different language type.
Also, using query parameter, it was easy to change langcode dynamically basing on the current language select value (in #20).

@Leksat,
Your patch wasn't working on drupal 8.3.x but i have made a fix see my patch.

If add new translatable entity by entity browser, new entity will be add with default site language. I added fix for it.

@Faibyshev: I think your way of getting the current language is not always the right?
Because of the types of language detections i think this is the right way: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Language%... and LanguageInterface::TYPE_CONTENT

@Lennard Westerveld, Thanks for the advice, you are right. I have made changes. Thank you.

I've tested on drupal 8.3.x the patch at #23 and it works for me.

4. Create new content and upload image via Entity browser field, specify alt and title attributes for image
5. Add translation for created content
6. Try to overwrite entity browser's image attributes to new language (create translation for referenced media entity)
As result, it create new translation for referenced media entity.

Reroll of #27 on 8.x-1.x branch.

BTW, #27 still applies nicely to 8.x-1.4.

Does this work with the View widget as well? or should I open a different issue? Currently on a translated node if you select a translated media entity, the original node changes to point to the selected translation as well. Expected behaviour was that the media on the original node would not change.

I'm not sure if in fits the scope of this issue, but we needed something similar but from a media entity perspective: creating media entity translations when they're added to content in a different language (e.g. referencing "English" media entity in a "Spanish" content would create a Spanish translation of that media entity).

Please ignore the patch if it doesn't fit in here.

Here's the updated patch that covers LanguageInterface::LANGCODE_NOT_SPECIFIED.

Putting back into needs review for #29.

+++ b/js/entity_browser.common.js
@@ -143,4 +143,23 @@
+  // AJAX beforeSubmit callback.
+  var ajaxBeforeSubmitOriginal = Drupal.Ajax.prototype.beforeSubmit;
+  Drupal.Ajax.prototype.beforeSubmit = function(form_values, element, options) {
+    if (this.$form) {
+
+      // If there is a langcode select element on the form, use its value in the
+      // AJAX call.
+      var langcodeSelectElementName = $(this.element).data('entity-browser-langcode-select-element-name');
+      if (langcodeSelectElementName) {
+        var $langcode = this.$form.find('select[name="' + langcodeSelectElementName + '"]');
+        if ($langcode.length) {
+          options.url = options.url.replace(/([?&]langcode=)[^&]+/, '$1' + $langcode.val() + '$2');
+        }
+      }
+    }
+
+    ajaxBeforeSubmitOriginal.call(this, form_values, element, options);
+  };

Is there another (cleaner?) place we can do this? Overriding the prototype in this way seems like a workaround.

I also wonder if the logic this patch is adding makes sense for everyone. From reading it - if I am viewing a translation of a Node and edit one of the referenced entities using Entity Browser, I am automatically creating a translation of that entity. Will every user want this?

This patch also still needs tests, so setting back to needs work.

Reroll of #27 on 8.x-1.x branch.

Putting back into needs review for #36

@badrange This issue still needs tests, and my feedback from #35 has not been addressed.

We are using this patch on every multilingual site. Whithout it, the handling of translated entities is completely broken.

This seems to be a major issue

This lead also on lost user data in a multilanguage website, and should be marked as bug not task as it is now.

Imagine this scenario, multilanguage website in italian and english:
- user create the english version of a page "Type A" as alt in the image write "This is the title and alt of the image"
- user translate the "Type A" in italian and set as alt for the image "Questo è il testo per alt e titolo dell'immagine"

You lost the english version, as the italian overrided it.

The patch in #36 solves the problem for me too, but it sure still needs tests.

Hi All,

Is there a patch for entity_browser 8.2.1 version? Patch #13 used to work with 2.0.0-alpha2.

thanks,

This should be focus on 8.2.x, it is not really fixed!

I now get

 Updating drupal/entity_browser (2.0.0 => 2.2.0): Downloading (100%)
  - Applying patches for drupal/entity_browser

    patches/entity_browser/2721665-13-2.patch (In-place media translation (Entity Browser 2.O))

   Could not apply patch! Skipping. The error was: Cannot apply patch patches/entity_browser/2721665-13-2.patch

Does this mean this patch is in 2.2.0?

@stewest

Does this mean this patch is in 2.2.0?

Sadly not, it means you got very lucky that such an old patch made for the 8.1 version survived for so long.

This really needs to get some TLC to get into the next release. I rerolled #36 against the latest 8.2.x-dev version to get this back on the radar.
Also upped the version from 8.x-1.x-dev to 8.x-2.x-dev.
Looking at #41 (and due to the lack of tests) I've left it at Needs work

D8.6 test failures are caused by https://www.drupal.org/project/entity_browser/issues/3054210, which makes running tests for this module on D8.6 fail as mentioned by oknate here: https://www.drupal.org/project/entity_browser/issues/3054210#comment-131...

Note that the tests fail because of using a function that exists only from D8.7 upwards, so it doesn't mean the test is failing, just that the test couldn't run. The mentioned function is only used in the tests, not in the actual module, so using this patch on D8.6 is still OK.

Note this is under testing at Smartsheet. We will report back.

Updating patch to avoid a PHP warning :

Warning: Declaration of Drupal\entity_browser\Plugin\Field\FieldWidget\FileBrowserWidget::displayCurrentSelection($details_id, $field_parents, $entities, $langcode = NULL) should be compatible with Drupal\entity_browser\Plugin\Field\FieldWidget\EntityReferenceBrowserWidget::displayCurrentSelection($details_id, array $field_parents, array $entities, $langcode = NULL)...

Reroll of patch in #50 against latest 8.2-2.x

Tags cleanup.

I will be trying to get at least the scenario from #41 in as a start point for (more) testing during the upcoming weeks.

We did a lot of testing and have this patch in place on a lot of production sites.
Test are not failing and not using this patch might lead to a loss af data.
We should go a step ahead.

Reroll the patch with the 8.x-2.x

Confirming RTBC

Does anyone know of an issue regarding this for core's media_library?

Works perfectly! Thank you!

#55 Patch does not work with layout builder in Drupal 9.3.0

Using LanguageManager service makes it works with Layout Builder.

+1 for adding this. Manually tested patches from #55 and #59, works nicely, but settings "Needs work" since maintainer has required tests for this issue.

I found a issue against core media library: #3221720: Improve workflow for translating Media entities with the Media Library

Reroll of patch 2721665-55 for version 2.8.

Patch #63 has some coding standard issues (spaces). It also seems that it does not address layout builder like patch #59 does.

#59 is the preferred patch here.

Updated #56 patch to use with latest module version.

@dewalt I assume you meant patch 59 was the source, as there is no patch in 56, and 56 is just a typo mistake.