Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set

Here's a patch that adds the language pulled from the editor and saves it to private temp storage. Using private temp storage solved an issue I was having with the ckeditor editor information (as apposed to the drupal Editor.php editor) no longer being available after the entity browser selection. Using private temp storage solves this problem.

It's currently saved in tempStoreFactory->get('entity_embed_dialog')->get('editor_language')

It seems not quite right to use variable across all entity embed dialogs, but you really can only use one at a time, so that might be ok.

Yes, I can add test coverage.

Getting started with test coverage. I want to add media test coverage with alt and title, and I want to add more end-to-end testing of embedding of nodes, but this gets it started.

This demonstrates the bug in this issue.

For now, I couldn't get the fix to work, so I put in a hack temporarily, as I need to go to bed and will continue on this later.

diff --git a/js/plugins/drupalentity/plugin.js b/js/plugins/drupalentity/plugin.js
index 27bbf56..5119845 100644
--- a/js/plugins/drupalentity/plugin.js
+++ b/js/plugins/drupalentity/plugin.js
@@ -37,7 +37,7 @@
           var existingElement = getSelectedEmbeddedEntity(editor);
 
           var existingValues = {
-            'data-editor-langcode' : editor.langCode
+            'data-editor-langcode' : drupalSettings.path.currentLanguage
           };
           if (existingElement && existingElement.$ && existingElement.$.firstChild) {
             var embedDOMElement = existingElement.$.firstChild;

I'm hoping that the original fix works and I just need to fix the test set up. The change would mean it's not pulling from the field's language but from the parent entity. If a field isn't translatable, it should use the original language. Back when I was working on this in December, the editor.langCode changed appropriately. I just need to keep working on the test, I think.

I was able to get editor.langCode to work correctly. Unfortunately, this requires the locale module to be enabled.

In /web/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php:

    if ($this->moduleHandler->moduleExists('locale')) {
      $ckeditor_langcodes = $this->getLangcodes();
      $language_interface = $this->languageManager->getCurrentLanguage();
      if (isset($ckeditor_langcodes[$language_interface->getId()])) {
        $display_langcode = $ckeditor_langcodes[$language_interface->getId()];
      }
    }

The odd thing is that the autocomplete in the selection phase of the dialog followed the parent without the locale module enabled, but then when you press "Next" and go to the review step, it would show the wrong language on the label of the embedded entity. So the locale requirement is inconsistent, the dialog is unaware of the correct language, but the autocomplete works properly: odd! So perhaps the patch should use whatever autocomplete is using to detect the language, or maybe it's better to fix the autocomplete to respect the editor language.

For now I'll proceed with the review stage using the editor language and enable locale for the test.

NOTE: I have a namespace error in the test class, it took me a minute to figure that out, so the test didn't run and I didn't realize this.

• Adds test coverage for media
• Adds test only patch to demonstrate the bug
• I realize now that I can't add test coverage for alt and title yet, because that feature doesn't exist yet for media embeds. See #2924391: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`. So we should probably get this one in, and then update this test, once title and alt editing goes in.

Remaining question: Should we have the functionality work without the locale module enabled. It seems to me that it should, but I'm not sure of the right approach to pull the language from the parent entity. It's in a lot of places, but the editor seems most appropriate. But that doesn't work without locale enabled.

NOTE: I have a namespace error in the test class, it took me a minute to figure that out, so the tests didn't run and I didn't realize this.

I have a namespace error in the test class, it took me a minute to figure that out.

Ignore this patch.

Here we go, I realized what was confusing me. My test wasn't actually running.

It was ignoring the test on testbot because of an error in the namespace. I had a copy and paste error in my namespace.

-namespace Drupal\Tests\content_translation\FunctionalJavascript;
+namespace Drupal\Tests\entity_embed\FunctionalJavascript;

• Removing the special characters because they look off in the patch, makes the patch easier to read.
• Since I updated the embedded content titles, the "trap" parent name doesn't make sense, since I'm not using "mouse" for the embed. So changing that. It's just cosmetic and shouldn't affect the test.

This adds some changes outside the scope of the new test, but some good clean up.

• Removes unused getTestFile method.
• Moves set up in base class to EntityEmbedDialogTest.
• Copies assignNameToCkeditorIframe method to base class, updates classes to extend base class.
• Fixes another cut and paste error:
  

  - * @group content_translation
  + * @group entity_embed
  

• Fixes deprecation of file_unmanaged_copy
• removes screenshot I accidentally added to the module in a previous commit:
  

  $this->createScreenshot(\Drupal::root() . '/sites/default/files/simpletest/screen.png');
  

• Fixes inconsistent naming of file:

       \Drupal::service('file_system')->copy(\Drupal::root() . '/core/misc/druplicon.png', 'public://Smeagol.jpg');
       /** @var \Drupal\file\FileInterface $file */
       $file = File::create([
  -      'uri' => 'public://example.jpg',
  +      'uri' => 'public://Smeagol.jpg',
  

• I made the titles longer, so it's obvious what's French and what's English:

  -    $node_fr->title = 'Superman';
  +    $node_fr->title = 'Superhomme';
  

  and

  -      'name' => 'Smeagol',
  +      'name' => 'Smeagol likes cheese',
  

  and

  -    $media_fr->name = 'Gollum';
  -    $media_fr->field_media_image->alt = 'Gollum';
  -    $media_fr->field_media_image->title = 'Gollum';
  +    $media_fr->name = "Gollum n'aime que la bague";
  +    $media_fr->field_media_image->alt = "Gollum n'aime que la bague";
  +    $media_fr->field_media_image->title = "Gollum n'aime que la bague";
  

• There were a few places the comments had the wrong language due to cutting and pasting:

  -    // Assert suggestions are in parent language (en).
  +    // Assert suggestions are in parent language (fr).
  

Because I keep stepping on my own toes, I'm creating a meta issue with some of my test updates:
#3055928: [META] 8.x-1.0-beta4 plan

I think editor.langCode is the ckeditor language code, not the drupal code, so we need to rethink this.

It needs to get \Drupal::service('language_manager')->getCurrentLanguage() from the form holding the ckeditor.

This is easier than I thought, the language is available in the dialog from \Drupal::service('language_manager')->getCurrentLanguage()

the url changes for the dialog based on the parent form, notice the "/fr/" in the url:

https://my.site/fr/entity-embed/dialog/basic_html/media_entity_embed?_wr...

Updating the way we fetch the language. I still think we might need to pass it through the js. I'm thinking of stupid edge cases, where you have a form with an inline entity form with a wysiwyg with an entity embed button. It should pull the language from the entity being edited in the inline entity form. I'm not sure if it would or wouldn't. I'd have to investigate. Or a paragraph with a wysiwyg, that would probably work, but what about a paragraph on an entity within an inline entity form?

• Removing dependency on locale: which is great.
• Adding additional test coverage outside the scope of the bug, such as testing that translation appears correctly in preview and when the entity is saved, and that adding data-langcode will override the default behavior.

Minor comment update.

Removing left over code from when I was using tempstorefactory.

Next time I update this, I need to fix this comment:

+    // Assert suggestions do not contain translation.
+    $this->assertNotContains('Superhomme', $suggestions);

Text should read "Assert suggestions contain translation."

Replacing some code with a more verbose, but I think more elegant bit of code. The code is adding an attribute to a dom element, and I think DomDocument is a more elegant way of handing html in PHP than string manipulation with str_replace.

-    $new_value = str_replace('drupal-entity ', 'drupal-entity  data-langcode="en" ', $value);
-    $source->setValue($new_value);
+    $dom = Html::load($value);
+    $xpath = new \DOMXPath($dom);
+    $drupal_entity = $xpath->query('//drupal-entity')[0];
+    $drupal_entity->setAttribute("data-langcode", "en");
+    $source->setValue(Html::serialize($dom));

for #27, why "sleep(1)"? It's shorter than

$this->getSession()->wait(1000);

Ideally, if I knew what was causing the random failure, we could add a condition:

$this->getSession()->wait(1000, '$("#something-special").length == 1');

Then if the condition is met, it doesn't have to wait. The problem is I don't know what is causing the bug. When it happens, clicking the entity embed button doesn't open the dialog. But if I wait a little bit, such as adding sleep() command, it'll work. So some javascript library probably needs to initialize. If we can figure out how to detect when that's ready, we could add a condition.

Note, for this:

It's possible I'm using the site in French but embedding an entity in a Spanish translation.

I don't this is currently supported when adding an entity. There's no option to select what language you'd like to select during the selection phase. If you manually set the language, say using the source button in the wysiwyg and then add data-langcode to the embed code, then when you open the dialog it should use that language. That would be a pretty easy change to make to the last patch. The data-langcode should override the language manager negotiated language when editing an embed.

Changing the title, as I'm realizing (based on #32) that when editing an embed with data-langcode explicitly set, it should override the parent language.

This also means, I should add to the test coverage: Add this action:

reopen the dialog after adding data-langcode manually by clicking the "source" button, check that the entity displays the label in English, despite the fact that the parent language is fr, when the embed's data-langcode is set to "en".

No, you're not missing something. Interface language is irrelevant.

If the parent is a French node, and you open the embed dialog, the the autocomplete shows French translations (this works), and when you click the next button, it should show the french translation, if available. That was the original bug. It wasn't respecting that. It was showing the original label, even though there was a french translation.

So it's more than just display. It's that the embed button should embed the entity in the same language as the parent by default.

I think you're right that the fact that it works in my test is just luck, The negotiated language just happened to match the parent language. But if the parent was a spanish node within an inline entity form within a french node and one clicked the embed button, it would probably show the page's language, rather than the parent. So we should probably pass through the javascript the language of the parent entity to the form, and then it should be passed to the preview step. The preview step would only not show the parent language if you're editing an embed and it has data-langcode set to a different language, then it would use that language. And I suppose it should validate that a translation in that language exists, and if not, fall back to the parent language.

Ideally the test should also have an edge case where you have a nested entity with a wysiwyg that has a different language than the parent, such as in inline entity form. Or perhaps if we get a source that is rock solid in pulling from the parent then we don't need to worry about that.

I was able to pass the language using hook_field_widget_form_alter().

Since a field could be untranslated, if "Users may translate this field" is unchecked, it should pull from the field, when possible. I left the entity as a fallback. I'm not sure that fallback would ever be used in hook_field_widget_form_alter().

I checked, and if "Users may translate this field" is unchecked, the language returned from $context['items']->getLangcode() reverts to the original language.

So that's a better place to get it than from the parent entity.

+
+/**
+ * Implements hook_field_widget_form_alter().
+ */
+function entity_embed_field_widget_form_alter(&$element, FormStateInterface $form_state, $context) {
+
+  // Add a langcode attribute so that entity embed javascript can pass it to
+  // EntityEmbedDialog.
+  if (!empty($element['#type']) && $element['#type'] == 'text_format') {
+    if (!empty($context['items']) && $context['items'] instanceof FieldItemListInterface) {
+      $element['#attributes']['data-langcode'] = $context['items']->getLangcode();
+    }
+    else {
+      $entity = $form_state->getFormObject()->getEntity();
+      if ($entity instanceof ContentEntityBase) {
+        $element['#attributes']['data-langcode'] = $entity->language()->getId();
+      }
+    }
+  }
+}

I also replaced the "sleep(1)" with a better way of waiting:

+    $this->getSession()->wait(5000, "(typeof CKEDITOR != 'undefined' && typeof CKEDITOR.instances['edit-body-0-value'] != 'undefined' && CKEDITOR.instances['edit-body-0-value'].instanceReady)");

It waits that long unless the condition is met. I'm pretty sure this is right, since the modal wasn't appearing when it failed, I think it was because the CKEditor instance wasn't ready yet.

I introduced a new bug that the tests caught! Here's a fix.

I love the helper method for autocomplete! Using getAutocompleteSuggestions() is much easier to read!

Oops!

-waitFoEditor()
+ waitForEditor()

That's funny. Nice catch.

The attribute data-langcode is also missing here:

 allowedContent: 'drupal-entity[data-embed-button,data-entity-type,data-entity-uuid,data-entity-embed-display,data-entity-embed-display-settings,data-align,data-caption]',

But attributes being stripped out doesn't seem to be a thing any more.

See #2917132: EntityEmbedBuilder removes data- attributes set by other text filter plugins.

See line 204 of EntityEmbedFilterTest.

 $content = '<drupal-entity data-foo="bar" foo="bar"

I can add it, but it just works already, FYI.

Sure enough. And I failed to cover that in the test. Updated so that the test uses the 'filter_html' filter. I ran it without updating the allowed html, and the test failed:

There was 1 failure:

1) Drupal\Tests\entity_embed\FunctionalJavascript\ContentTranslationTest::testDialogLanguage
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Smeagol likes cheese alt'
+'Gollum n'aime que la bague alt'

Then added 'data-langcode alt title' and it passed.

Also, now I think we should rename the test function 'testDialogLanguage()'. Since the dialog language would be the interface language, and the test was expanded to cover more than that. I'll change it to 'testHostEntityLangcode()'.

Awesome! Thanks for getting this committed. I'm happy that we have this test coverage running now. Between CKEditorIntegrationTest and ContentTranslationTest we have pretty good test coverage of the usual use cases I think.