entity_embed_filter_format_edit_form_validate() Logic is Faulty

I'm still seeing 'The %embed button requires among the allowed HTML tags.' on step 10 with 'Limit allowed HTML tags and correct faulty HTML' unchecked.

I would expect to see no error related to filter_html when the option is unchecked.

filter_html->status is wrong!

I think the major issue here is this is a validation hook, and runs before the entity object is updated, but it's checking if the filter is enabled on the not-yet-updated filter plugin object. So it works intermittently when you're not changing the filter statuses! If you were resaving the form without changing filters and writing the validation code, you would have missed the bug.

Changing where we check if a filter is enabled to check on the form_state values works much better!

-  if ($allowed_html = $form_state->getValue($allowed_html_path)) {
+  $filter_html_enabled = $form_state->getValue([
+    'filters',
+    'filter_html',
+    'status',
+  ]);
+
+  if ($filter_html_enabled && $allowed_html = $form_state->getValue($allowed_html_path)) {


-        if (!isset($allowed['drupal-entity']) && $filter_html->status) {
+        if (!isset($allowed['drupal-entity'])) {

That explained why I was getting some very unexpected behavior when testing.

I wrote some test cases on #3060728: entity_embed_filter_format_edit_form_validate() should check if 'filter_html' is enabled before validating and #3060729: entity_embed_filter_format_edit_form_validate() should check if <drupal-entity> has attributes.

Here's a first test case. This is failing, so the bug still needs work.

Another surprising find. The validation wasn't running when adding a filter format!

Here's a fix for the first test case. I will add additional test coverage for the validation, and I'd like to move the validation in ImageFieldFormatterTest into this class FormatsValidationTest. I don't know if we want to get the fix in first, or wait for the test coverage.

@oknate I can confirm the patch works fine. I talked to Wim and he will take a look later, see if we can move this forward!

@Chris Burge: Great find! Thanks for the detailed explanation and patch! 👍👏

#6 + #8 Nicely spotted :)

#8: Let's create a separate issue for moving the test coverage out of ImageFieldFormatterTest into this new test. Let's also move the configuration UI test coverage in \Drupal\Tests\entity_embed\FunctionalJavascript\CKEditorIntegrationTest::testIntegration() into this new test in that same issue.

1. I've renamed this from FormatsValidationTest to ConfigurationUiTest.

2. +++ b/tests/src/FunctionalJavascript/FormatsValidationTest.php
   @@ -0,0 +1,116 @@
   +  /**
   +   * A test node to be used for embedding.
   +   *
   +   * @var \Drupal\node\NodeInterface
   +   */
   +  protected $node;
   

   This is unused.

3. +++ b/tests/src/FunctionalJavascript/FormatsValidationTest.php
   @@ -0,0 +1,116 @@
   +    // Create a page content type.
   +    $this->drupalCreateContentType([
   +      'type' => 'page',
   +      'name' => 'Basic page',
   +    ]);
   

   Unused.

4. +++ b/tests/src/FunctionalJavascript/FormatsValidationTest.php
   @@ -0,0 +1,116 @@
   +      'administer display modes',
   +      'administer embed buttons',
   +      'administer site configuration',
   +      'administer display modes',
   +      'administer content types',
   +      'administer node display',
   +      'access content',
   +      'create page content',
   +      'edit own page content',
   

   These aren't necessary.

5. +++ b/tests/src/FunctionalJavascript/FormatsValidationTest.php
   @@ -0,0 +1,116 @@
   +    $page->checkField('filters[filter_html][status]');
   +    $page->checkField('filters[entity_embed][status]');
   +    $page->selectFieldOption('editor[editor]', 'ckeditor');
   +    $page->waitFor(10, function () use ($page) {
   +      return $page->hasField('filters[filter_html][status]');
   +    });
   +    $page->checkField('filters[filter_html][status]');
   +    $page->checkField('filters[entity_embed][status]');
   

   This is doing the same thing twice, and with an unnecessary wait. Simplified.

6. +++ b/tests/src/FunctionalJavascript/FormatsValidationTest.php
   @@ -0,0 +1,116 @@
   +    $buttonElement = $this->assertSession()->elementExists('xpath', '//li[@data-drupal-ckeditor-button-name="test_media_entity_embed"]');
   

   Nit: we don't use camelCase.

7. +++ b/tests/src/FunctionalJavascript/FormatsValidationTest.php
   @@ -0,0 +1,116 @@
   +    // Verify the <drupal-entity> tag is not yet allowed.
   +    $allowed_html = $this->assertSession()->fieldExists('filters[filter_html][settings][allowed_html]')->getValue();
   +    $this->assertNotContains('drupal-entity', $allowed_html);
   ...
   +    $this->assertSession()->pageTextContains('The Media Entity Embed button requires <drupal-entity> among the allowed HTML tags.');
   

   So … we're testing that this is NOT working. Okay. That's … confusing.

   I did some digging. This is caused by a pre-existing core bug: #2763075: Adding new text format gives Uncaught TypeError: f.format_tags.split is not a function. Having this test coverage is therefore a useful way for us to know when this is fixed in Drupal core.

I will add additional test coverage for the validation, […] I don't know if we want to get the fix in first, or wait for the test coverage.

Let's add the test coverage first.

Because the current test coverage passes without the changes here; it's actually a test unrelated to the reported bug :) It is test coverage for the additional bug that #8 found.

and I'd like to move the validation in ImageFieldFormatterTest into this class FormatsValidationTest.

This we can do in a next issue. Or here, if you prefer. But it's definitely lower priority

I actually built the test case off of manual testing: https://www.drupal.org/project/entity_embed/issues/3060729#comment-13140560

Thanks for the copious feedback, Wim. I'll try to get it updated this morning.

Oh, I see you already made most of the changes. We just need to finish the test coverage.

Adds dataprovider based test to test both adding and editing the form.

One important change:

-  if ($filter_html_enabled && $allowed_html = $form_state->getValue($allowed_html_path)) {
+  if ($entity_embed_enabled && $filter_html_enabled && $allowed_html = $form_state->getValue($allowed_html_path)) {

We don't need to validate the allowed values if $entity_embed_enabled is FALSE.

There are some cs that need cleanup. I have to head in to work now, though.

@oknate When do you think you'll continue? Otherwise I have somebody with me here at Drupal Dev Days who could fix those coding standards violations and the patch feedback I have below :)

1. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +        'expected_error_message' => NULL,
   ...
   +        'expected_error_message' => FALSE,
   

   We're using NULL and FALSE, but AFAICT they're treated the same.

2. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +      'Tests validation when drupal-entity element lacks a few attributes.' => [
   

   s/a few attributes/some required attributes

3. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +  public function testValidationWhenAddingProvider() {
   ...
   +  public function testValidationWhenEditingProvider() {
   

   Data providers' method names cannot start with the prefix test — if they do, they'll be executed as test methods :)

4. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +   * @param string $filter_html_status
   ...
   +   * @param string $entity_embed_status
   

   These aren't strings but booleans.

5. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +   * @param string $allowed_html
   

   This can currently be either a string or FALSE, so the documentation isn't accurate.

6. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +   *   The allowed html to set.
   

   Nit: s/html/HTML/

7. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +   * @dataProvider testValidationProvider
   +   * @dataProvider testValidationWhenAddingProvider
   

   Woah! :D I didn't know this was possible :D

8. +++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
   @@ -33,25 +33,142 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
   +    $this->getSession()->executeScript("jQuery('.visually-hidden').removeClass('visually-hidden');");
   

   Clever. But let's not make this rely on jQuery. Let's use native DOM APIs (document.querySelectorAll()).

Go ahead, I will be out of pocket for 10 hours

Here's an update. My apologies to anyone in Cluj who was hoping to work on it today. Thanks for all of the feedback, Wim. Lots of good catches. I'm kicking myself I put some booleans as strings in the param descriptions.

Thanks oknate for the update, I just have some small items of feedback:

#1

+++ b/tests/src/FunctionalJavascript/ConfigurationUiTest.php
@@ -75,19 +75,19 @@ class ConfigurationUiTest extends EntityEmbedTestBase {
+  public function validationProvider() {

In core all the dataprovider method names are prefixed like this: provider. Just to be more consistent with core.

#2: A more general one: All tests and dataProviders in core are grouped together, and the test function is placed first followed by the dataProvider just below it. So I would reorder them to be more consistent, like in core.

Attached is a new patch with the renamed and moved dataproviders.
I also included 2 interdiffs, one for the renaming and one for the moving, just to make it more clear what happend.

Patch now applies.

+    Editor::create([
+      'format' => 'embed_test',

Can you change this to use $format->id()? That's the only nitpick.

Thanks so much to both of you! :)

There you go. Last nitpick fixed.

Thank you guys for the help and feedback!

For consistency, there are other places in the functional javascript tests where we're doing Editor::create() and hard coding the id. Should those be changed as well? Or to avoid unrelated changes, save that for another time?

Good call oknate! I would move that to a different issue, at least in my opinion. Just to prevent this issue from containing cs changes in all tests of the entity embed module.

Or to avoid unrelated changes, save that for another time?

This.

What was still missing was a failing test run for the test coverage added in #22. I did that locally, and … success 👍

/usr/local/bin/php /private/var/folders/4l/5sndwsz50tqgxjh2525n10br0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/Work/d8/core/phpunit.xml --filter "/(::testValidationWhenEditing)( .*)?$/" Drupal\Tests\entity_embed\FunctionalJavascript\ConfigurationUiTest /Users/wim.leers/Work/d8/modules/entity_embed/tests/src/FunctionalJavascript/ConfigurationUiTest.php
#!/usr/bin/env php
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\entity_embed\FunctionalJavascript\ConfigurationUiTest
..EE..                                                              6 / 6 (100%)

Time: 1.16 minutes, Memory: 6.00MB

There were 2 errors:

1) Drupal\Tests\entity_embed\FunctionalJavascript\ConfigurationUiTest::testValidationWhenEditing with data set "Tests validation when entity_embed filter not enabled and filter_html is enabled." (true, false, 'default', false)
Behat\Mink\Exception\ResponseTextException: The text "The text format Embed format has been updated." was not found anywhere in the text of the current page.

/Users/wim.leers/Work/d8/vendor/behat/mink/src/WebAssert.php:787
/Users/wim.leers/Work/d8/vendor/behat/mink/src/WebAssert.php:262
/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/FunctionalJavascript/ConfigurationUiTest.php:201

2) Drupal\Tests\entity_embed\FunctionalJavascript\ConfigurationUiTest::testValidationWhenEditing with data set "Tests validation when drupal-entity element has no attributes." (true, true, '<a href hreflang> <em> <stron...ntity>', 'The <drupal-entity> tag in th...title.')
Behat\Mink\Exception\ResponseTextException: The text "The <drupal-entity> tag in the allowed HTML tags is missing the following attributes: data-entity-type, data-entity-uuid, data-entity-embed-display, data-entity-embed-display-settings, data-align, data-caption, data-embed-button, alt, title." was not found anywhere in the text of the current page.

/Users/wim.leers/Work/d8/vendor/behat/mink/src/WebAssert.php:787
/Users/wim.leers/Work/d8/vendor/behat/mink/src/WebAssert.php:262
/Users/wim.leers/Work/d8/modules/entity_embed/tests/src/FunctionalJavascript/ConfigurationUiTest.php:198

This means the test coverage is indeed reproducing the bug in HEAD.

Sweet! That's some pretty thorough coverage!

Indeed — thanks to all of you! And especially @wannesderoy and @oknate 🥳