"TypeError: originalGetFocusedWidget is not a function" if drupalimage plugin button not used

Thanks so much for reporting this, @artis! 🙏 👍

Ideally, we'd have a functional JS test that tests Entity Embed without the drupalimage button enabled, that should reproduce the JS error. You can find similar test coverage you can start from in \Drupal\Tests\entity_embed\FunctionalJavascript\CKEditorIntegrationTest::testIntegration().

It took me a bit to understand the problem. Here's a simplified version of the issue:

var emptyVar = {};
if (emptyVar) {
console.log("emptyVar evaluates to true");
}

If you run the code above, you'll see that emptyVar evaluates to true and the condition runs.

Unlike a php array, an empty javascript object will evaluate to true.

So if our fake drupalimage plugin is set the else condition:

else {
  CKEDITOR.plugins.drupalimage = {};
 }

then this code could fire:

 if (CKEDITOR.plugins.drupalimage) {
  return originalGetFocusedWidget(editor);
}

So the patch, which checks if originalGetFocusedWidget is set is better.

-        if (CKEDITOR.plugins.drupalimage) {
+        if (originalGetFocusedWidget) {
           return originalGetFocusedWidget(editor);
         }

Steps to reproduce:

• Use a format with both an entity embed button and the core drupalink ("Link") button enabled, but not the drupalimage button.
• Go to add or edit a node
• Click the "link" button without focusing on an entity embed.

If you press the drupalink button when not focused on the entity embed, the link dialog will fail to load (and an error will display in the console, although I don't know if we can test for the error directly.

If you focus on the entity embed and press the drupal link button, the error will not present itself.

I agree testing this is a bit silly considering it's a newly introduced bug, introduced by this module.

I mean, do we need to test all of core along with the module?

It wouldn't hurt. Adding test coverage for the proper functionality of the link button when not focused on an entity embed will make sure it doesn't resurface as plugin.js is modified. If it happened once, it could happen again.

Plus, we're directly overwriting the drupalimage plugin, and its method CKEDITOR.plugins.drupalimage.getFocusedWidget(). So it is quite relevant to this module to test how that overridden CKEDITOR.plugins.drupalimage object behaves.

#6:

So it seems to me that a test should exist in core for confirming that the link button works even if drupalimage is not present.

This ☝️. Except not in core, but in this module :)

#7:

I agree testing this is a bit silly considering it's a newly introduced bug, introduced by this module.

I disagree. Because while this is indeed a pretty simple oversight/bug in the JS code I wrote last week, it actually is very valuable to have strong guarantees that this functionality works independently of the existing drupalimage functionality in Drupal core, since it's totally feasible to stop using all of that completely by using Entity Embed's CKEditor integration today (or the port of the subset thereof to Drupal core in Drupal 8.8).

I mean, do we need to test all of core along with the module?

Not all of core, but high-impact edge cases: yes. Because …

If it happened once, it could happen again.

… of this.

Having a test for this gives us a higher degree of confidence. If this were a simple bug unrelated to interactions with other dependencies, then I wouldn't be asking for test coverage.

Plus, we're directly overwriting the drupalimage plugin, and its method CKEDITOR.plugins.drupalimage.getFocusedWidget(). So it is quite relevant to this module to test how that overridden CKEDITOR.plugins.drupalimage object behaves.

Exactly! 👏

Thanks a lot to both of you!

+++ b/tests/src/FunctionalJavascript/MediaImageTest.php
@@ -574,12 +575,44 @@ class MediaImageTest extends EntityEmbedTestBase {
   public function testCkeditorWidgetIsLinkable() {
...
+    // Remove DrupalImage.
+    // See https://www.drupal.org/project/entity_embed/issues/3061050
+    $editor = Editor::load('full_html');
+    $settings = $editor->getSettings();

This is now ironically swapping one half of test edge cases for another!

It used to only test with drupalimage, now it's only testing without. We want to test with both.

oh, good point.

Yay, only MediaImageTest::testCkeditorWidgetIsLinkableWhenDrupalImageIsAbsent() is failing, as expected. This means that using drupallink when not focusing the Entity Embed CKEditor widget while drupalimage is present does work. We're now successfully verifying that only the one case that @artis discovered to fail is actually failing.

This is now 💯 🥳

🚢

great, thanks!

Here is a patch for version: 8.x-1.0-rc1

Sorry the patch in comment 16 wasn't generated correctly

Here is a new patch for version: 8.x-1.0-rc1

Here is a new patch for version: 8.x-1.0-rc1