DrupalMediaLibrary plugin breaks things when adding a new text editor

Do we need to check

$editor->get('status')

Otherwise looks good to me

Only two things, then RTBC once I've manually tested it. This blocks #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`.

+++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
@@ -108,6 +108,13 @@ public function getFile() {
+
+    // If the editor hasn't been saved, return an
+    // empty config.
+    if ($editor->isNew()) {
+      return [];
+    }

There's an empty line above this which shouldn't be here, and the we need to expand the comment because it doesn't currently explain why we do this. How about: "If the editor hasn't been saved, we will not be able to create a coherent MediaLibraryState instance, which is needed in order to generate the required configuration. However, if we're creating a new editor, we don't need to do that anyway, so just return an empty array instead."

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -205,4 +221,19 @@ public function testButton() {
+  /**
+   * Show visually hidden fields.
+   */
+  protected function showHiddenFields() {
+    $script = <<<JS
+      var hidden_fields = document.querySelectorAll(".visually-hidden");
+
+      [].forEach.call(hidden_fields, function(el) {
+        el.classList.remove("visually-hidden");
+      });
+JS;
+
+    $this->getSession()->executeScript($script);
+  }
+

In a JavaScript test, this feels like cheating. I would prefer if we entered the value in the 'name' field, then waited for the machine name to show up. (Or, alternately, we could wait for the machine name field to have the expected value.)

Thanks, @oknate. Looks good to me. RTBC when tests are green.

#15 strengthened the test coverage. It already was RTBC in #12.

I wanted to re-RTBC, but:

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -155,7 +155,20 @@ public function testConfigurationValidation() {
+    // Test that DrupalMediaLibrary button drupal-media adds allowed tags.
...
+    // Test that the saved filter format has the new allowed html tags on the
+    // <drupal-media> tag.

These comments need to be improved. The first sentence is very broken, the second sentence should have "html" capitalized to "HTML".

improved comments

I really wanna re-RTBC, but first a few small things:

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -141,6 +140,37 @@ public function testConfigurationValidation() {
   +    $this->assertNotEmpty($buttonElement = $assert_session->elementExists('xpath', '//li[@data-drupal-ckeditor-button-name="DrupalMediaLibrary"]'));
   +    $buttonElement->dragTo($target);
   

   Nit: From what I hear, the coding standards don't want us to mix snake_case and camelCase in the same file. So, $buttonElement should be $button_element.

   Also, and this is no big deal at all: why are some of the element locators in this test CSS selectors while others are XPath queries?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -141,6 +140,37 @@ public function testConfigurationValidation() {
   +    $assert_session->pageTextContains('Added text format Sulaco.');
   +    // Test that when adding the DrupalMediaLibrary button to the editor the
   

   The comment should have a blank line above it, I think; it's a new "section" of the test.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -141,6 +140,37 @@ public function testConfigurationValidation() {
   +    // Test that the saved filter format has the new allowed HTML tags on the
   +    // <drupal-media> tag.
   +    $format = FilterFormat::load('sulaco');
   +    $allowed_html = $format->filters('filter_html')->settings['allowed_html'];
   +    $this->assertContains($expected, $allowed_html);
   

   I'm not sure what this is adding -- it's just proving that the filter form works (and saves the entity), which is surely tested elsewhere and is not really in scope for this test, IMHO.

1. Changed $buttonElement to $button_element
2. Added a blank line above comment

Regarding #28:

+++ b/core/modules/simpletest/simpletest.module
@@ -565,14 +565,8 @@ function simpletest_classloader_register() {
- *
- * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use
- *   \Drupal\Tests\TestFileCreationTrait::generateFile() instead.
- *
- * @see https://www.drupal.org/node/3077768

wat.

Looks like there are some unintended changes in here...that patch is RTBC otherwise, I think. Can you post a FAIL patch too, just for completeness' sake?

And, DONE. Let's get this bad boy in.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -141,6 +140,33 @@ public function testConfigurationValidation() {
+    // correct tags are added to the <drupal-media> tag in the Allowed HTML

Not "tags", but "attributes" … 😊

(Can be fixed on commit.)

Committed/pushed to 8.8.x, thanks!

Thanks @catch, and thanks for fixing #34 on commit :)