Add tests to confirm setting up an Entity Embed button works

Similar to #2560787-3: Add tests for entity access.

No longer postponed, since #2328659: Convert all existing entity_embed tests from WebTestBase to BrowserTestBase landed a long time ago.

Note that #2466013: Write test for CKEditor integration already added test coverage for using an Entity Embed button: for configuring it for use in CKEditor. See \Drupal\Tests\entity_embed\FunctionalJavascript\CKEditorIntegrationTest::testIntegration().

But this issue is about having tests to verify that the UI at /admin/config/content/embed is working.

I can work on this.

Thanks for stepping up, but I think this is definitely of lesser importance. This is not something that's negatively impacting anybody today, and even when it breaks, it would only affect administrators, not end users.

So: decreasing priority.

I'd much prefer to contribute your valuable and much valued time on issues with higher impact :)

OK, I enjoy writing the functional javascript tests. But I agree this won't get us closer to the goal of media embeds in core.

If you enjoy it, feel free to do this of course :D But #3022768: Validate that `alt` and `title` are required attributes for `<drupal-entity>`, and ensure they're added by default for example would have a far higher impact on the community!

Yes, I think #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` would be a good one to focus on next.

Ehm, yes, that's the one I wanted to link! 😅

I started working on this. I ran into two issues so far:

1) The ajax request that returns the entities fails on testbot. I'm not sure if it's testbot or my config. For now I threw in a hack to continue. The bug is hard to identify, but I'm getting a headers too large error. If I reduce the number of config entities passed to ::getDefinitionOptionsForEntityType() it works. So I think it has to do with something in the testbot configuration.

2) There is a schema error related to entity browser.

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Core\Config\Schema\SchemaIncompleteException</em>: Schema errors for embed.button.node with the following errors: embed.button.node:type_settings.entity_browser_settings variable type is NULL but applied schema class is Drupal\Core\Config\Schema\Mapping in <em class="placeholder">Drupal\Core\Config\Development\ConfigSchemaChecker-&gt;onConfigSave()</em> (line <em class="placeholder">95</em> of <em class="placeholder">core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php</em>). <pre class="backtrace">call_user_func(Array, Object, &#039;config.save&#039;, Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher-&gt;dispatch(&#039;config.save&#039;

I want to document this before I continue, so here's a patch demonstrating the schema error.

This fixes the schema error. It's just that entity_browser_settings has to be an empty array instead of NULL.

Also, I realize this test won't work on testbot yet, because I was building it on top of the test module in 2924391.

Updated to verify that when reopening the form, the embed type and entity type fields are disabled.

Also, since I was doing the same thing with slight variations, I added a loop to reduce code duplication.

The one known issue, which is a blocker, is that I'm using a hack to remove the config entities. Without that the test fails. See #12.1 above.

diff --git a/src/Plugin/EmbedType/Entity.php b/src/Plugin/EmbedType/Entity.php
index 42bb3fa..8dce309 100644
--- a/src/Plugin/EmbedType/Entity.php
+++ b/src/Plugin/EmbedType/Entity.php
@@ -227,6 +231,9 @@ class Entity extends EmbedTypeBase implements ContainerFactoryPluginInterface {
   protected function getEntityTypeOptions() {
     $options = $this->entityTypeRepository->getEntityTypeLabels(TRUE);
 
+    // @todo Remove this hack
+    unset($options['Configuration']);
+
     foreach ($options as $group => $group_types) {
       foreach (array_keys($group_types) as $entity_type_id) {
         // Filter out entity types that do not have a view builder class.

I found a workaround for #12.1 that isn't a hack. While I still don't know what the underlying issue was, I noticed that it was processing parts of the for loop that were unnecessary if the first condition was met. Each condition, when met runs this:

unset($options[$group][$entity_type_id]);

So there's no reason to run the other two conditions if the first is met, and no reason to run the third if the second is met. Perfect situation for some "elseif" conditions.

When I added elseif statements, the failure went away.

diff --git a/src/Plugin/EmbedType/Entity.php b/src/Plugin/EmbedType/Entity.php
index 8dce309..d682ea4 100644
--- a/src/Plugin/EmbedType/Entity.php
+++ b/src/Plugin/EmbedType/Entity.php
@@ -231,9 +231,6 @@ class Entity extends EmbedTypeBase implements ContainerFactoryPluginInterface {
   protected function getEntityTypeOptions() {
     $options = $this->entityTypeRepository->getEntityTypeLabels(TRUE);
 
-    // @todo Remove this hack
-    unset($options['Configuration']);
-
     foreach ($options as $group => $group_types) {
       foreach (array_keys($group_types) as $entity_type_id) {
         // Filter out entity types that do not have a view builder class.
@@ -241,12 +238,12 @@ class Entity extends EmbedTypeBase implements ContainerFactoryPluginInterface {
           unset($options[$group][$entity_type_id]);
         }
         // Filter out entity types that do not support UUIDs.
-        if (!$this->entityTypeManager->getDefinition($entity_type_id)->hasKey('uuid')) {
+        elseif (!$this->entityTypeManager->getDefinition($entity_type_id)->hasKey('uuid')) {
           unset($options[$group][$entity_type_id]);
         }
         // Filter out entity types that will not have any Entity Embed Display
         // plugins.
-        if (!$this->displayPluginManager->getDefinitionOptionsForEntityType($entity_type_id)) {
+        elseif (!$this->displayPluginManager->getDefinitionOptionsForEntityType($entity_type_id)) {
           unset($options[$group][$entity_type_id]);
         }
       }

Secondly, I accidentally left a sleep() call in my for loop (for testing purposes). The testbot execution in #15 took 40 minutes because of this. Oops!

diff --git a/tests/src/FunctionalJavascript/ButtonAdminTest.php b/tests/src/FunctionalJavascript/ButtonAdminTest.php
index 44bee01..12a7714 100644
--- a/tests/src/FunctionalJavascript/ButtonAdminTest.php
+++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
@@ -104,8 +104,6 @@ class ButtonAdminTest extends WebDriverTestBase {
         ->selectExists('type_id')
         ->selectOption('entity');
 
-      sleep(500);
-
       $this->assertSession()->assertWaitOnAjaxRequest();
 
       $this->assertSession()

This looks really nice, but I have some questions and comments here.

1. +++ b/src/Plugin/EmbedType/Entity.php
   @@ -210,7 +210,11 @@ class Entity extends EmbedTypeBase implements ContainerFactoryPluginInterface {
   -    $form_state->setValue('entity_browser_settings', $form_state->getValue('entity_browser_settings'));
   +    $entity_browser_settings = $form_state->getValue('entity_browser_settings');
   +    if (empty($entity_browser_settings)) {
   +      $entity_browser_settings = [];
   +    }
   +    $form_state->setValue('entity_browser_settings', $entity_browser_settings);
   

   FormStateInterface::getValue() can return a default argument, so this can be collapsed down to one line:

   $form_state->setValue('entity_browser_settings', $form_state->getValue('entity_browser_settings', []));

2. +++ b/src/Plugin/EmbedType/Entity.php
   @@ -234,12 +238,12 @@ class Entity extends EmbedTypeBase implements ContainerFactoryPluginInterface {
   -        if (!$this->entityTypeManager->getDefinition($entity_type_id)->hasKey('uuid')) {
   +        elseif (!$this->entityTypeManager->getDefinition($entity_type_id)->hasKey('uuid')) {
   

   Changing from 'if' to 'elseif' is a subtle, but potentially significant, change in logic. if/elseif is basically "case 1 or case 2, but not both", whereas if/if is "case 1 and/or case 2".

   Which means the test coverage needs to account for all possible cases. Maybe I'm being overly paranoid here, but I thought I'd point that out.

3. +++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
   @@ -0,0 +1,146 @@
   +    $view_mode_storage = $this->container
   +      ->get('entity_type.manager')
   +      ->getStorage('entity_view_mode');
   +    $view_mode_storage->create([
   +      'id' => 'media.thumb',
   +      'targetEntityType' => 'media',
   +    ])->save();
   

   Nit: $view_mode_storage is not really needed; this can all be one single chain of calls. $this->container->get()->getStorage()->create()->save()

4. +++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
   @@ -0,0 +1,146 @@
   +    // Delete the existing buttons, since we'll be testing
   +    // creating buttons.
   +    $storage = $this->container->get('entity_type.manager')
   +      ->getStorage('embed_button');
   +    $buttons = $storage->loadMultiple();
   +    $storage->delete($buttons);
   

   Why is this needed?

5. +++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
   @@ -0,0 +1,146 @@
   +    $this->assertSession()
   +      ->fieldExists('label')
   +      ->setValue($entity_type_id);
   +
   +    $this->assertSession()
   +      ->selectExists('type_id')
   +      ->selectOption('entity');
   

   Nit: It's a bit more readable to do $page = $this->getSession()->getPage() at the top of the method, then call methods like $page->fillField() and $page->selectFieldOption(). It's cleaner than using $this->assertSession(), and will throw nice exceptions if there are problems.

6. +++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
   @@ -0,0 +1,146 @@
   +    $this->assertSession()
   +      ->waitForField('type_settings[entity_type]')
   +      ->selectOption($entity_type_id);
   +
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   +
   +    if (isset($bundle_id)) {
   +      $this->assertSession()
   +        ->waitForField('type_settings[bundles][' . $bundle_id . ']')->check();
   +    }
   

   JsWebAssert::waitForField() returns NULL if the element is not found, so you need to do something like this to prevent potential fatal errors:

   $element = $this->assertSession()->waitForField();
   $this->assertNotEmpty($element);
   $element->selectOption();
   

For #18 2, while in general I agree that three ifs do not equal an if and two elseifs, I think in this case they're equivalent, since they perform the same code

$a = 1;

if ($x) { 
  unset($a);
}
if ($y) { 
  unset($a);
}
if ($z) { 
  unset($a);
}

Since the operations within the second two conditions do the same thing as the first, if the first condition is met, you can safely skip the second two conditions.

Also if the first condition is not met, but the second condition is met, you can skip the third condition. Having the same line of code within three if statements is essentially the same here as creating an if with two elseifs.

Have I missed something?

For #18 4, I was originally extending the test base and installing entity_embed_test module, which installed some buttons. I think I can drop this code. Entity Embed has a suggested config for an entity_embed button with name 'node', but I can rework the code so I don't need to delete the existing buttons.

Thank you for the review @phenaproxima. Very helpful. I will get a chance to update the patch tonight.

Thanks for the dataprovider change in #17, @wimleers. That's cool. It counts as four tests now instead of one!

Responses to @phenaproxima's review in #18:

• #18.1: updated
• #18.2: left as is, for now. See #19.
• #18.3: using the chain now.
• #18.4: I tried to rework the test, but it was more verbose. It's easier to just delete the existing 'node' button, so that the label, entity_type_id and button id are all the same.
• #18.5: It was a bit annoying to rewrite the entire test, but it did force me to test out the Mink DocumentElement class. It definitely is less verbose.
• #18.6: Refactored away.

Thanks, @phenaproxima and @oknate!

1. +++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
   @@ -59,60 +60,51 @@ class ButtonAdminTest extends WebDriverTestBase {
   +    // Delete the existing node button provided by entity_embed module,
   +    // so that we can create a button with the same machine name.
   

   80 cols nit.

2. +++ b/tests/src/FunctionalJavascript/ButtonAdminTest.php
   @@ -59,60 +60,51 @@ class ButtonAdminTest extends WebDriverTestBase {
   -  public function testEmbedButtonAdmin($entity_type_id, $bundle_id, $entity_embed_display_plugin_id) {
   +  public function testEmbedButtonAdmin($entity_type_id = '', $bundle_id = NULL, $entity_embed_display_plugin_id = '') {
   

   This change made all these arguments optional, even though they aren't optional.

🚢

WRT #18.2: this could also just have been expressed by OR'ing all the conditions together and only having a single branch body, since all three branch bodies are identical anyway. That's why the elseif already is an improvement: it doesn't try re-executing the same branch body multiple times if a particular entity type matches multiple of the possible reasons for it to be executed. Committed as-is since it's already an improvement anyway.

Since there were comments on each condition and the conditions were many characters, I felt leaving the multiple lines was more readable than one big condition.

Yep, I'm just saying that some will probably find that easier to read. I think this is really just a minor matter of preference, which is why I didn't want to spend more brain cycles on this :)

Yes! On to the more interesting challenges.