Problem/Motivation

Discovered while reviewing #3260554: [drupalMedia] Support alignment on <drupal-media>, see https://git.drupalcode.org/project/drupal/-/merge_requests/1719/diffs#no....

We have no test coverage at all proving that sites using media_embed and the DrupalMediaLibrary button get upgraded as expected.

Steps to reproduce

N/A

Proposed resolution

N/A

Remaining tasks

Review.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Status: Active » Needs review
StatusFileSize
new4.45 KB
wim leers’s picture

StatusFileSize
new755 bytes
new4.45 KB
bnjmnm’s picture

Status: Needs review » Needs work
+++ b/core/modules/ckeditor5/tests/src/Kernel/SmartDefaultSettingsTest.php
@@ -109,6 +113,36 @@ protected function setUp(): void {
+    $basic_html_format_with_media_embed['filters']['media_embed'] = [
+      'id' => 'media_embed',
+      'provider' => 'media',
+      'status' => TRUE,
+      'weight' => 100,
+      'settings' => [
+        'default_view_mode' => 'default',
+        'allowed_view_modes' => [],
+        'allowed_media_types' => [],
+      ],
+    ];

The only config property needed for this test is 'status' => TRUE, while the additional config is not doing any harm, I prefer not adding anything that isn't directly relevant to the test.

wim leers’s picture

Status: Needs work » Needs review
StatusFileSize
new1.31 KB
new4.21 KB

Those settings are copied 1:1 from the annotation of \Drupal\media\Plugin\Filter\MediaEmbed — they’re exactly the defaults.

IIRC there were some problems with not passing valid configuration to plugins. But … in this case, it seems to work just fine if I omit them. So, did what you asked :)

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Re #5

Those settings are copied 1:1 from the annotation of \Drupal\media\Plugin\Filter\MediaEmbed — they’re exactly the defaults.

That pretty much lines up with my suspicions regarding why it was there because they certainly looked like default values. It may have zero impact long term, but I like that the test demonstrates that 'status' => TRUE is the only property relevant to that part of the process. Thanks for indulging the preference 😎

  • lauriii committed c715824 on 10.0.x
    Issue #3261712 by Wim Leers, bnjmnm: Expand SmartDefaultSettingsTest to...

  • lauriii committed 90818af on 9.4.x
    Issue #3261712 by Wim Leers, bnjmnm: Expand SmartDefaultSettingsTest to...

  • lauriii committed 4398dc5 on 9.3.x
    Issue #3261712 by Wim Leers, bnjmnm: Expand SmartDefaultSettingsTest to...
lauriii’s picture

Version: 10.0.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed c715824 and pushed to 10.0.x. Also cherry-picked to 9.4.x and 9.3.x because CKEditor 5 is experimental. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.