Add an input filter to display embedded Media entities

Here's the input filter only patch.

Hey phenaproxima,

I've generalized krlucas's patch and moved it to the filter module. I've gone ahead and refactored the filter class to dry up some of the logic. I ran into an issue however when working on this. If you're embedding an entity that references itself and we get a recursive nesting fatal error. I'm not 100% sure how we should handle this. I added a @todo for this in code.

Lastly I ran out of time today to work on tests, however I can knock those out another day.

@nathandentzau thanks! I'll give it a test drive.

Note the test in #4 was dropped in #8. We should add it back even if it's not quite right.

Updated patch with test both for the basic functionality of the filter but also with a test for recursion.

I had to reduce the recursion depth limit in the filter from 20 to 2 to get the recursive exception to be thrown before phpunit got weird.

The test definitely needs to be cleaned up.

It appears test bot is seeing whitespace differences in the output rendered directly from the entity view builder and that output from the embed filter. We could probably fix by stripping all whitespace from both expected and actual though I'm not sure if that's a fair test.

The patch in #11 did no longer apply to the latest commit on 8.7.x. Re-patched it in order to continue working on this issue.

Tested manually and here are some findings:

1. +++ b/core/modules/filter/src/Plugin/Filter/FilterEntityEmbed.php
   @@ -0,0 +1,323 @@
   +            sprintf('Unable to load embedded %s entity %s.', $entity_type, $id)
   

   $id is undefined.

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterEntityEmbed.php
   @@ -0,0 +1,323 @@
   +        <drupal-entity data-entity-type="node" data-entity-uuid="07bf3a2e-1941-4a44-9b02-2d1d7a41ec0e" data-view-mode="teaser" />');
   

   there is no data-view-mode, it's data-entity-embed-display now?

3. Image in the embed entity is not getting displayed properly.
   

1. Fixed #1 and #2
2. Updated issue summary with testing steps.

+++ b/core/themes/stable/templates/content-edit/entity-embed.html.twig
@@ -0,0 +1,15 @@
+<article{{ attributes }}>{{ children }}</article>

I'm not so sure that makes sense, as usually the entity itself will also be an article, so you get <article><article></article></article>?

The documentation says this is a container/wrapper, so that sounds like a div to me?

I also think there are older, existing issues for this and we will need a plan on how to handle this in the entity_embed module, as this is going to clash with that. Based on the very similar code, I guess this implementation is very much based on that (which makes me wonder a bit how copyright/credit rules apply when contrib code is moved to core, we should probably at least credit the people who worked on the contrib module?) but that had its own plugin system that could be extended that is gone but there still seem to be left-overs of that in at least one place (data-entity-embed-display defaults).

It's entity_embed vs filter, so I think the result would be that filter.module's plugin would win (because being later alphabetically and then existing data might not work anymore, e.g. cases that used something different than the default entity reference display plugin).

I also think there are older, existing issues for this and we will need a plan on how to handle this in the entity_embed module

I think that'd be #2927867: Why is my embedded image being wrapped in an <article> tag?.

I have yet to form a complete analysis and opinion about the patch attached to this issue. I've started triaging the entire https://www.drupal.org/project/issues/entity_embed issue queue to ensure we don't introduce knowable bugs into core.

Concerns about the current patch:

1. Comparing the \Drupal\filter\Plugin\Filter\FilterEntityEmbed class that this patch introduces with the upstream \Drupal\entity_embed\Plugin\Filter\EntityEmbedFilter whose functionality it is bringing into core, I'm surprised by the amount of change:
   

   1 file changed, 230 insertions(+), 97 deletions(-)
   

   That's pretty much a rewrite. That's relatively scary to be honest.

   It's absolutely possible that there are good reasons that justify this, but I don't see them listed here. Ideally, any concerns we have with the upstream code should be fixed there first.

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterEntityEmbed.php
   @@ -0,0 +1,324 @@
   +    // The caption text is double-encoded, so decode it here.
   +    if (isset($context['data-caption'])) {
   +      $context['data-caption'] = Html::decodeEntities($context['data-caption']);
   ...
   +    // Maintain data-caption if it is there.
   +    if (isset($context['data-caption'])) {
   +      $build['#attributes']['data-caption'] = $context['data-caption'];
   +    }
   

   This … seems sketchy. It also violates the principle that Drupal text @Filter plugins should be composable and not need to be aware of each other. This code is specifically aware of the data-caption attribute, which is handled by the \Drupal\filter\Plugin\Filter\FilterCaption filter in core.

   Note that the original code for this filter, at \Drupal\entity_embed\Plugin\Filter\EntityEmbedFilter, is NOT doing anything with data-caption. So this is at least one place where my initial gut feeling in the previous point seems to be confirmed.

3. My comment at #2577891-3: Entity Embed 8.x-1.0.0-rc1 release from a month before Drupal 8.0.0 was released:
   

   Given #2593315: Improve documentation of hook_entity_embed_alter() and #2593379: Analyze/improve/fix handling of bubbleable metadata in entity_embed, I think we need to first seriously scrutinize the APIs of Entity Embed, to prevent API changes after RC?

   Unfortunately, there's no progress to report on there. That does not inspire confidence in the state of the Entity Embed module's code, especially when also considering its last commit dates back over 18 months, and the one before that over two years.

   (That's NOT to be held against its maintainers — maintaining open source code requires a lot of work! But it does mean there is a certain level of risk to bring this code into Drupal core. That's my key concern here.)

I'll dig deeper and will report back by the end of the day tomorrow.

Sounds like a good MVP plan to me! Like you said, get it working just with Media first, then look to see if it can be broadened. Personally I'm a fan of the concepts that Drupal embodies about not being a cookie cutter solution, so I'd be keen to see drupal-entity remain on the cards for future work, but I think for now keeping the scope narrow is sensible.

Looking forward to seeing how this progresses.

We've made tons of progress on #2577891: Entity Embed 8.x-1.0.0-rc1 release in the past few days. Time to get a patch going for this issue!

This patch:

• extracts a subset of \Drupal\entity_embed\Plugin\Filter\EntityEmbedFilter filter, to work only with media entities (see #25, first bullet) and view modes (see #25, second bullet)
• uses <drupal-media> instead of <drupal-entity> (see #25, third bullet)
• data-align and data-caption work (but #2752253-29: Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default will still need to land upstream)
• test coverage is super minimal for now, it's painful to write test coverage for now because Entity Embed's test coverage is doing rather rough HTML assertions, which are impossible to transform to Media entity HTML assertions because Media entities are rendered in more complex ways; we should first improve \Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest further
• in-place editing and contextual links are explicitly disabled; the reason being that it's going to be a terrible UX if content authors can in-place edit embedded entities, because most of them will assume that will only affect that specific embed (to be ported to Entity Embed, and this will also affect its test coverage)

(#25, fourth bullet needs to be done in a follow-up, that's solely adding configuration.)

To test:

1. install Standard + Media Library
2. create an Image media item
3. apply this patch, and enable its filter, add <drupal-media data-entity-type data-entity-uuid data-view-mode data-align data-caption> to the allowed HTML tags
4. there's no CKEditor integration yet (this is only a filter), so copy/paste this:
   

   <drupal-media data-align="center" data-caption="The first core Media embed ever." data-entity-type="media" data-entity-uuid="a1eec161-2f6e-42af-843e-703738a135eb" data-view-mode="embed"></drupal-media>
   
   <drupal-media data-align="center" data-caption="This media entity is missing" data-entity-type="media" data-entity-uuid="wrong" data-view-mode="embed"></drupal-media>
   
   <drupal-media data-entity-type="media" data-entity-uuid="" data-view-mode="embed"></drupal-media>
   

   and adjust the UUID to match the one of the media item you just created, and change the view mode to one that is available on your site

5. You should see something like this:
   

   The first embed works as expected (but the caption filter's HTML needs to be hardened obviously), the second embed which is referring to a now-missing entity falls back nicely and the third embed whose UUID attribute is empty is ignored by the filter.

This should be green.

A review of just the filter would be very welcome. Extreme nitpicking of that class is already welcome — this is going to be the heart of this feature, and it should be very reliable.

#30 Thanks for the review! 🙏

1. ✅
2. ✅ (marked it final too)
3. ✅ (instead removed the constant). I'm super fascinated by your proposal to pave the path for derivatives. I think we should KISS, and not make the code even the slightest bit more complex for future extensions. Beyond that, I think the whole point is that we reuse the existing entity render system, with view modes etc. If we introduce entity type-specific data- attributes, we move away from the "single way of declaring what you're embedding", and therefore make it harder to do transformations and migrations in the future. One of which would be to move on from <drupal-media> to <drupal-entity> (per #25, third point).
4. ✅
5. 😀
6. 🤔Maybe in the future. Definitely out-of-scope here.
7. 👎Let's only do that when the need arises.
8. 👎\Drupal\filter\Plugin\Filter\FilterCaption::process() does the same.
9. ✅(discussed with @phenaproxima, the constant makes less sense now)
10. ✅
11. 🙂
12. ✅
13. ✅
14. 🤔If we're going to do that, we should do it in this patch IMHO. I'll do that next (early next week).
    I wanted to move this method into a utility class like you asked, but unfortunately it needs the renderer service, and \Drupal\Component\Utility\Html doesn't use the service container at all, and for good reason: it lives in \Drupal\Component.
15. ✅
16. Because it is an object that gets updated by reference, so that when we serialize the DOM whose nodes we're modifying
17. 👎👎👎 That would expand the scope massively. I just renamed it to avoid triggering this understandable reflex :)

    Also spotted some nits myself, fixed those too.

Of course I find additional nits just after submitting 😅

After an initial quick pass together with @alex.pott, I found some leftovers from an earlier patch iteration. Also added an explicit @todo for #30.14 (recursive rendering protection). And added more explicit @todos for the missing upstream test coverage that should be improved and then brought into this patch.

Walked through it properly with @alex.pott this time, he suggested:

1. to not use final, because it's not an existing pattern
2. to not use private, because it's not an existing pattern
3. to use ->error(…) rather than ->log(RfcLogLevel::ERROR, …)

And last but not least: he said the filter should not generate config dependencies (on the used view modes etc) because that'd then require modifying existing content just to be able to modify config (to remove entity view modes & displays), the existing fallback behavior is preferable over that.

So, embedding means injecting proprietary tags such as <drupal-media ...>? This is very sad. This kind of markup has no meaning outside Drupal. Other system doesn't know how to deal with tags such as <drupal-media ...>. In the case you want to achieve interoperability this is a NO GO. Why not injecting the rendered media which already have native tags (img, a, etc.) wrapped in this proprietary HTML tag has all he Drupal metadata (uuid, view mode) as data attributes? The wrapper will be just ignored on other systems but the inner markup can still be interpreted. That said the process of rendering the media will take place on the embedding phase, not in the filter

After a 4 month period of silence here due to my working on getting JSON:API to ship with Drupal 8.7, time to pick this up again!

That said the process of rendering the media will take place on the embedding phase, not in the filter

That's exactly why we don't inject rendered media. When the media in the library is updated, we need those changes to appear everywhere else too. That's how https://www.drupal.org/project/entity_embed works too.

Note that any consumer of the website never sees our custom HTML tags — the outside world only ever sees the rendered media just as you say!

This addresses @alex.pott's feedback from #27. Next up is test coverage, as described in #27, specifically these two bullets:

• test coverage is super minimal for now, it's painful to write test coverage for now because Entity Embed's test coverage is doing rather rough HTML assertions, which are impossible to transform to Media entity HTML assertions because Media entities are rendered in more complex ways; we should first improve \Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest further
• in-place editing and contextual links are explicitly disabled; the reason being that it's going to be a terrible UX if content authors can in-place edit embedded entities, because most of them will assume that will only affect that specific embed (to be ported to Entity Embed, and this will also affect its test coverage)

Updating title per #25.

Patch #38:

+  protected function renderMissingMedia() {
+    return [
+      '#type' => 'html_tag',
+      '#tag' => 'img',
+      '#attributes' => [
+        'src' => file_url_transform_relative(file_create_url('core/modules/media/images/icons/no-thumbnail.png')),
+        'width' => 180,
+        'height' => 180,
+        'alt' => $this->t('Deleted content encountered, site owner alerted.'),

Is this text alternative going to be presented publicly to a website visitor? The message here describes the inner working of the CMS, and governance process in the organization who runs it. A public visitor doesn't benefit from being told about this. In any case, "site owner alerted" is not conveyed by the image itself; we don't want to provide extra information to screen reader users which we haven't provided for a sighted user. Also "deleted" isn't the same as "missing".

This is a tricky situation to come up with alt text for. I'd suggest something simpler, like "missing image" or "broken image". This would be a more matter-of-fact statement, which doesn't say anything about the CMS or owner's processes. It would be good to tailor it to the media type ("image" or "video" instead of vague "content").

Time for a big update. https://www.drupal.org/project/entity_embed has been stabilized 🥳 And comprehensive test coverage was added in #3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test.

Time to bring that all over in clear steps to this issue!

Let's start with something simple: the visual brokenness you can see in #27.5's screenshot.

Before

After

❤️ StableLibraryOverrideTest

This ports all the test coverage related to foundations from #3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test. This includes test coverage for:

• the absolute basics: data-view-mode is able to select a view mode and associated cacheability is bubbled
• entity access is respected and associated cacheability is bubbled
• graceful degradation when entities are deleted or manually crafted HTML with invalid UUIDs is specified — and of course sensible alt text per #40
• only <drupal-media> tags are processed
• the alignment and caption filters still work as expected, including when the media embed itself is a link

To be able to pass the above tests, three changes had to be made in the filter's code:

1. The Entity Embed module implemented @andrewmacpherson's feedback from #40 in #3063705: Improve `alt` and `title` attributes for missing entity indicator, and has explicit test coverage for it.
2. "consumed" attributes should be removed, just like \Drupal\filter\Plugin\Filter\FilterAlign and \Drupal\filter\Plugin\Filter\FilterCaption do.
3. Any non-consumed attributes must be retained, to ensure that a next filter is able to act on them.

#27 pioneered the disabling of Contextual Links and Quick Edit — that's where this core patch identified a gap in https://www.drupal.org/project/entity_embed and leaped ahead.

#27 included rudimentary test coverage for this in a functional test. But now we can do this in a kernel test, with more precision, and faster :)

So far, we only covered things that were essentially already supported in #27. We're just adding test coverage so far and doing finishing touches it seems.

But three important features are still missing in the current patch.

The first of those three: per-embed overrides. The ability to have <drupal-media … alt="this alt text is better tuned for the context where this media is embedded"> 🥳And of course, it comes with test coverage.

The second of those three: translations. Again, with test coverage of course.

And last but not least: recursion protection.

Considering this is a blocker of #2801307: [META] Support WYSIWYG embedding of media entities which in turn is a blocker for #2834729: [META] Roadmap to stabilize Media Library, bumping priority to Major.

Also revamped the issue summary.

D'oh. Something silly ruining an otherwise beautiful streak of green patches! 😞

Fixing the CS violations I introduced while porting this.

1. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,399 @@
   + *   description = @Translation("Embeds media items using a custom HTML tag. If used in conjunction with the 'Align/Caption' filters, make sure this filter is configured to run after them."),
   ...
   + *   weight = 100,
   

   Note that the default weight already ensures this happens by default. If reviewers would like that, we could add the additional form validation callback that the Entity Embed module is adding, entity_embed_filter_format_edit_form_validate(), which goes through great lengths to ensure that the site builder cannot set up things incorrectly.
   In Entity Embed we even have extensive functional JavaScript tests that prove this validation logic is working correctly and providing actionable error messages in all scenarios.

2. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,399 @@
   + * @internal
   

   This was done at the request of #30.2, but are we sure about that?

3. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,399 @@
   +      \Drupal::logger('media')->error('Recursive rendering detected when rendering embedded media: %entity_id. Aborting rendering.', [
   ...
   +          \Drupal::logger('media')->error('The media item with UUID "@uuid" does not exist.', ['@uuid' => $uuid]);
   ...
   +          \Drupal::logger('media')->error('The view mode "@view-mode-id" does not exist.', ['@view-mode-id' => $view_mode_id]);
   

   This should probably get logger.factory injected. Or not? Because this is only used for error cases, perhaps we ought to keep \Drupal::logger()? I'm not sure what the best practice is here.

4. +++ b/core/modules/media/tests/src/Functional/FilterTest.php
   @@ -0,0 +1,141 @@
   +class FilterTest extends BrowserTestBase {
   

   This test coverage that #27 introduced is now obsolete, since the kernel tests cover everything this covers, and much more!

   (It made sense at the time, because Entity Embed had little test coverage, but that's no longer the case.)

This reroll addresses point 4, points 1, 2 and 3 are open questions to reviewers 😊

Fixed the last test failure, which wasn't an actual failure, just a use of deprecated code in a test's setUp().

Also fixed the last few coding standards violations.

This should probably get logger.factory injected.

Unless it's really difficult, I'd prefer if it were injected.

Done.

If reviewers would like that, we could add the additional form validation callback that the Entity Embed module is adding, entity_embed_filter_format_edit_form_validate(), which goes through great lengths to ensure that the site builder cannot set up things incorrectly.

My opinion is that, generally, if it is even possible to set it up incorrectly to the point where users and site builders will be scratching their heads and wondering WTF, why did my site break...we should do something to protect people from stumbling into that position. That said, we don't need to go insanely far; we just need to clearly warn people if they are going to inadvertently break something.

Sure.

I wanted to keep this separate from essentials like #64 so that we can easily choose to not do this if we want.

Note that this test coverage hasn't gone through quite the same level of scrutiny as the test coverage above; if we decide we like the additional validation this adds, we can of course scrutinize that all we want :)

This patch is looking really good!

1. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,411 @@
   +      static::$recursiveRenderDepth[$recursive_render_id]++;
   

   At some point, we need to decrement this, right? Otherwise, this prevents rendering 20 total embeds of the same media entity even if they're not recursive. For example, if you have a View of 20 nodes that each embeds the same media entity.

2. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,411 @@
   +    // - Contextual Links do not make sense for embedded entities; we only allow
   +    //   the host entity to be contextually managed;
   +    $build['#pre_render'][] = static::class . '::disableContextualLinks';
   +    // - Quick Edit does not make sense for embedded entities; we only allow the
   +    //   host entity to be edited in-place.
   +    $build['#pre_render'][] = static::class . '::disableQuickEdit';
   

   Given that at least these two features need to be conditional on whether the media is embedded or not, it seems likely to me that other modules (including contrib) will need to be as well. Therefore, is there an API we can add to support that? Not sure if a new hook would make sense, or whether we want to leverage existing view alter hooks and just need to add some #property to identify that this is embedded?
   Handling the contextual links here seems appropriate, since EntityViewBuilder is what adds them, but let's move the quickedit disabling to the quickedit module, so it can serve as a reference consumer of whatever API or #property that we add here.

3. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,411 @@
   +      '#type' => 'html_tag',
   +      '#tag' => 'img',
   

   Is there a reason we're not using '#theme' => 'image' here?

4. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,411 @@
   +      foreach ($xpath->query('//drupal-media[@data-entity-type="media" and normalize-space(@data-view-mode)!="" and normalize-space(@data-entity-uuid)!=""]') as $node) {
   

   If the tag name is drupal-media, why do we require (or even allow) the data-entity-type attribute?

   Why do we require the data-view-mode attribute, instead of using 'full' as a default if it's empty (to match the default of EntityViewBuilderInterface)?

5. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,411 @@
   +        $media->{$image_field}->alt = $node->getAttribute('alt');
   +        $media->thumbnail->alt = $node->getAttribute('alt');
   ...
   +        $media->{$image_field}->title = $node->getAttribute('title');
   +        $media->thumbnail->title = $node->getAttribute('title');
   

   We're applying the same alt and title to two different images: the source image field and the thumbnail. Do we know for certain that both won't be rendered? If so, let's add a comment as to why we know that. Or, if both can be rendered, then is it sensible for the same alt and title to apply to both? If so, let's add a comment explaining why that's sensible/desired.

Re 1, the problem is that we delayed rendering, we don't actually know when it will happen, maybe some trickery with a callback as I thought about in #2876709: Recursive rendering protection mistakenly kills repeated rendering, too, but the core entity reference formatter has the same problem, so possibly we can solve it for both things together in #2940605: Can only intentionally re-render an entity with references 20 times

#66: Thanks 😊 And yay! 🥳

1. I specifically made this use the same logic as core already does where it has recursive rendering protection: \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceEntityFormatter::viewElements(). Also: what @Berdir said in #67 — I'd rather have this follow a pre-existing pattern in core with a particular known problem rather than inventing something new here which suffers from a different set of problems :)
2. ✅ Very good point! A property is declarative and hence is IMO preferable over introducing a hook. This never even crossed my mind since this originates from stabilizing a contrib module, where we obviously cannot change things in Drupal core for the better end result. So I'm very grateful you pointed this out 🙏
   Added the declarative property, removed ::disableQuickEdit(), updated quickedit_entity_view_alter().
3. ✅ Not particularly. I don't have a preference. Switched to your suggestion.
4. RE: data-entity-type: Excellent question :) Because years ago, before Drupal 8 shipped, Drupal core coordinated with Entity Embed (well, I coordinated with Dave Reid) to ensure we have a single consistent way to detect the presence of entities in what effectively are blobs of HTML. We agreed on data-entity-type and data-entity-uuid. That's how \Drupal\editor\Plugin\Filter\EditorFileReference is able to reliably generate file usage data. Except of course that there are lots of known problems with our file usage, entity usage, and entity dependency tracking. Nonetheless, by having this filter generate (and require) the same structure, we make it possible to in the future have a single coherent way of tracking usage. Without having to retroactively update all user-generated content. If you agree with this rationale — and I hope you do — we should document it in the patch.

   RE: data-view-mode: explicitness. Because there are lots of surprising aspects to this:

   1. This is a blob of HTML. Non-Drupal experts may look at it. It may be migrated at some point.
   2. Maybe the default of full on EntityViewBuilderInterface::view() disappears.
   3. full is a lie for Node entities: if you load full, you'll end up with default.
   4. This will be exposed to non-Drupal consumers via JSON:API, GraphQL and REST. We should not leave semantics open for interpretation when we know that we expect it to behave in a particular way in Drupal itself.

   Weighing the trade-off of this spectrum of potential problems versus having to specify data-view-mode="full", and taking into account the fact that nobody will be writing that HTML themselves manually since it'll be generated by #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library, I think it is prudent to be more specific.

5. ✅ Comment added. Related: #2956368: MediaThumbnailFormatter produces unhelpful text alternative and title attributes for media thumbnails.

+++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
@@ -0,0 +1,401 @@
+    $build['#media_embed'] = TRUE;

Would it make sense to rename this to just #embed? In other words, is it likely that quickedit should not apply itself for all entity embeds, not just media? Note that there's already a separate #media, or more generically #ENTITY_TYPE, property present if alter hooks need to distinguish by that.

Out of curiosity, I searched HEAD to see if #embed is already a minted name, and it is, for Views embed displays. I'm not sure if that's a pro or con to using the same name here. I think it might be a pro, as it's roughly the same concept both here and in Views. But maybe it's a con if there's differences in the two concepts that matter.

I haven't debugged this yet, so maybe there's just something weird going on for me locally, but posting this here anyway, in case it's a real bug...

When I follow the instructions in the IS, then it all works great when I view the node at its URL (e.g., /node/2). However, when I go back to the front page, and then see that article as a teaser, it just displays the word "and", but without rendering the media above or the missing media below.

+++ b/core/modules/media/media.module
@@ -359,3 +359,142 @@ function media_entity_type_alter(array &$entity_types) {
+      $required_attributes = [
+        'data-entity-type',
+        'data-entity-uuid',
+        'data-align',
+        'data-caption',
+        'alt',
+        'title',
+      ];

Per #68.4, 'data-view-mode' should be added to this list.

Why are data-align, data-caption, alt, and title in this list? What if I don't want to allow my content authors to provide captions, or to override the media item's alt and title?

+++ b/core/modules/media/media.module
@@ -359,3 +359,142 @@ function media_entity_type_alter(array &$entity_types) {
+    // Require `<drupal-media>` HTML tag if filter_html is enabled.

Is it possible to automatically add it when the Media embed filter is newly enabled (when I click its checkbox in the UI)? Or is there a reason that we shouldn't do that?

when I go back to the front page, and then see that article as a teaser

That reminds me... Something that I would want if I were building a site is to be able to render the media in a different view mode based on the view mode of the host entity. For example, if I'm rendering the article in full, then maybe I want the embedded media in full. And if I'm rendering the article as a teaser, then maybe I want to render the embedded media as a teaser/thumbnail/whatever. In fact, I might want to enforce this, and not allow content authors to specify a data-view-mode attribute when inserting the media. That way, presentation decisions are kept in the Manage Display configurations (where they belong), and not inside the content.

Maybe it makes sense to punt that to contrib? But I'm not sure, because I'd rather that core encourage best practices, and I'm not sure if it's best practice for core to steer people into making presentation choices inside their content. For example, when using a Media field, content authors are not able to pick the view mode as part of adding media to that field: they can only do that via the field's formatter.

> That reminds me... Something that I would want if I were building a site is to be able to render the media in a different view mode based on the view mode of the host entity.

To be honest, I feel like that level of control requires different tools. e.g. layout builder, paragraph-based approaches or just simply separate fields and fixed layouts, which is how we almost always handle teasers. IMHO, trying to use (automatically) trimmed versions of text for teasers almost never works, even when it's just text, and less so when you have more complex elements like embedded entities in there.

I feel like that level of control requires different tools

Hm, maybe so. But I still feel like requiring the data-view-mode attribute steers people down the wrong path. What if we make it optional, and add a default_view_mode setting to the MediaEmbed filter? That way, site builders who want to keep presentation decisions out of their content can do so (by not adding data-view-mode as an allowed attribute on <drupal-media>), while site builders who want to expose the ability for content authors to pick the view mode on a per-embed basis can add it as an allowed attribute.

I think that makes sense, if we have a setting we could also make it a multi-selection, because even *if* you want to allow access to select a view mode, there's about a 99% chance that you only want to allow a few embed specific view modes that make sense. The entity_embed module handles that through its buttons & plugins, not on the filter level.

+1 to all this, especially both #75 and #76.

Excited to see this moving forward. Hope to have time for a more thorough review and testing, we'll see.

Thanks,
-Derek

Big +1 from me to giving the site builder more control over which view modes are available to content authors, and forcing a specific one, it would enhance and be in line with #784672: Allow text field to enforce a specific text format.

20 recursive renders seems like a lot for embedded media. I can understand that for regular entity references, but perhaps a lower limit, like 10, would be more appropriate here?

See #66.1 and #67: Until those issues are fixed, this number limits the total renders (per page request) of the same media entity, whether recursive or not. So -1 to reducing it, at least until it's solely a recursion limit.

#69: 🥳✅Changed it to '#embed' => TRUE.

#70: I cannot reproduce this. I just see the first embedded media.

#71: Good catch on data-view-mode — that's a mistake I made in the porting of #65. WRT data-align & friends: like I wrote in #65: this test coverage hasn't gone through quite the same level of scrutiny . For now, omitted those.
Related to that: we could make data-align required if the filter_align filter is enabled, and do the same for data-caption + filter_caption. OTOH, it is possible that one wants alignment/captions for <blockquote>s and not for <drupal-media>. So given that, I think we should stick to what's in the current patch, and then just ship Standard with sane defaults.

#72: Excellent question! 🙏

• One of the next patches in this patch set (see #2801307: [META] Support WYSIWYG embedding of media entities) specifically does add this: #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library — see the 6th and 7th bullet in Testing steps 🙂
• The previous bullet is possible thanks to #1894644: Unidirectional editor configuration -> filter settings syncing, which as the name already indicates, was designed only for editor configuration affecting filter configuration — not for one filter affecting another filter. #2567801: Deprecate core/modules/editor/js/editor.admin.js JS APIs in Drupal 10, for removal in Drupal 11 exists to improve this, but was never prioritized.

The same challenge is already present in Drupal core when enabling the filter_align, filter_caption or editor_file_reference @Filter plugin.

Does that convince you that we shouldn't do it here either, or do you think we should try to do that anyway?

#73: If I understand your comment correctly, you're wondering if we shouldn't enforce a single view mode by default? If so: YES PLEASE! 👏👏👏👏👏👏 This is exactly what I proposed during the Barcelona Media Sprint in December, and it's documented in the 4th bullet of #25. I personally believe this would be a massive improvement: choosing a view mode for embedded media is cool, but how often does it really make sense to embed media in Full or Media Library view mode (the two defaults core ships with)? And even if we add an Embed view mode by default, when does it make sense to select one of those other two?
I totally see how a Tiny embed and Detailed embed could be valuable. But that is a relatively advanced use case — used by perhaps 10 or 20% of Drupal 8 sites. Why not defer that to a media_embed_viewmode contrib module?
This would simplify so much. 🙏

#74: I think you too are leaning towards something I suggested above for #73?

#75: Or why not defer this to a contrib module that decorates the existing plugin implementation?

#76: Correct, but this means that even though the Entity Embed contrib module's user interface only allows say view modes X and Y, its filter still allows anything. I think you would agree that in core we would stronger guarantees about what presentation-level choice is available in the structured data that is generated?

#78:

1. No, because this asset library is only ever used by \Drupal\media\Plugin\Filter\MediaEmbed. Put differently: it's only ever used if the filter module uses the @filter plugin we provide, and in that case the filter/caption asset library will also exist. Very sharp observation though! 👍
2. 🦅👁✅ Done, although arguably now if somebody adds another submit button, then they won't get the validation anymore. But this seems ridiculously far-fetched. I agree your suggestion hardens the situation.
3. No, this is not getting the settings (allowed_html) for filter_html, but whether it's turned on or off.
4. 🤔Your second point was about playing well with form alters. This ensures form alters making label alterations are respected. So wouldn't it be better to keep this as-is?
5. 🦅👁🤔! Do you have a suggestion for how to fit all that in 80 cols?
6. I agree with @effulgentsia in #80 — I just want to keep this the same as other places in core with the same logic.
7. Hm … I guess we could. But the end result would be the same. This feels like premature optimization to me. I'd rather have us generate a declarative result (a render array) and then rely on all the existing infrastructure to do this for us. That's also what we do in many (most?) other places in Drupal core. I don't feel very strongly about this though. I wonder what @effulgentsia thinks?
8. ✅ Comment added. (To convince yourself: remove that top-level key and run \Drupal\Tests\media\Kernel\MediaEmbedFilterTest::testAccessUnpublished(), observe it fail.)
9. ✅ 1) Of course! Done., 2) We can't assume it will always be lowercase. 3) Done.
10. http://drupal4hu.com/node/416.html :)

Also renamed MediaFilterConfigurationUiTest to MediaEmbedFilterConfigurationUiTest.

#79: Cool, that sounds like another vote for my proposal in #73. Looking forward to your thoughts on my proposal too 😊

#80: +1

#82:

1. Good question. Yes, we can be certain of that. Because the three filters being checked here are all provided by filter.module and are therefore always present.
2. 🦅👁🙏✅ Fixed! I disagree with missing test coverage. We don't want to have test coverage for every subset of every string. Then Drupal would be at least 10x bigger.
3. ✅
4. ✅ Right, that's a remnant from an earlier iteration where we didn't have data-view-mode. Removing data-view-mode is being discussed again ironically :)
5. ❤️ — Trying to help future maintainers, which may be I, or it may be somebody who's yet to discover Drupal!
6. ✅
7. ✅
8. 👍 (Had to look this up too by the way!)
9. There is: \Drupal\Tests\simpletest\Unit\SimpletestPhpunitRunCommandTest::setUpBeforeClass() + \Drupal\Tests\block\Kernel\BlockRebuildTest::setUpBeforeClass().
10. ✅
11. ✅
12. Oh, hah! You're absolutely right. We missed that in #3060749: entity_embed_filter_format_edit_form_validate() Logic is Faulty too apparently. Excellent catch! Will address that tomorrow.

#83:

1. ✅
2. 🤔✅ Hm … I'm not quite convinced by this, because we know from MediaEmbedFilterTest's passing that filtering is not failing. But still, I see your point. Done.
3. 👎 Close, but not quite! getAttachments() could also return attachments other than library. See \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processAttachments(). This asserts that only one type of attachment exists: library. The next assertion asserts that only the expected values for the library attachment type exist. Very subtle, I know :)
4. 👍
5. ✅
6. You're right, currently we're only testing that it's possible to override these. I'll add test coverage for this tomorrow.
7. 🤔 I personally don't think that's worth testing.
8. 👎 We are. The $verification_selectors 'figure > figcaption', 'div[data-media-embed-test-view-mode].align-center', 'figure > a[href="https://www.drupal.org"] + figcaption', 'a[href="https://www.drupal.org"] > div[data-media-embed-test-view-mode].align-center' etc assert the processing of FilterAlign and FilterCaption have happened.
9. 👍✅ Good idea!
10. 👎 No because that would be inaccurate. The filter plugin has no knowledge of the host entity at all. It only receives the text language. And the text language happens to match the host entity's language. That's why I chose this specific wording. Given that extra info, do you agree with it now?
11. 🇧🇷 Hah! I think this is thanks to @marcoscano 🥳
12. Anywhere you're returning an array you can use it :)

Tomorrow: #82.12 and #83.6

Add missing test coverage discovered in #82.12.

Add missing test coverage discovered in #83.6.

+++ b/core/modules/media/tests/src/Kernel/MediaEmbedFilterTest.php
@@ -213,21 +215,38 @@ public function testOverridesAltAndTitle() {
+      '`title` field property enabled ⇒ `title` is not overridable' => [

Nit: s/is not/is/

Only reviewed the non-test parts. Really impressive stuff, had a couple comments that @phenaproxima had above as well that were already answered so removed those and only a couple minor things remain. Really impressive work!

1. +++ b/core/modules/media/media.module
   @@ -359,3 +359,138 @@ function media_entity_type_alter(array &$entity_types) {
   +    $filter_format = $form_state->getFormObject()->getEntity();
   ...
   +    $filter_html = $filter_format->filters()->get('filter_html');
   +    $filter_html->setConfiguration(['settings' => $form_state->getValue($filter_html_settings_path)]);
   

   I think it is strange to be updating the entity stored in the form object here. If it does not already have the submitted form values at this point I don't think we should change them here, so I think we need to clone the filter before modifying it.

2. +++ b/core/modules/media/media.module
   @@ -359,3 +359,138 @@ function media_entity_type_alter(array &$entity_types) {
   +    $singular = 'The %media-embed-filter-label filter needs to be placed after the %filter filter.';
   +    $plural = 'The %media-embed-filter-label filter needs to be placed after the following filters: %filters.';
   +    $error_message = \Drupal::translation()->formatPlural(count($error_filters), $singular, $plural, [
   +      '%media-embed-filter-label' => $get_filter_label('media_embed'),
   +      '%filter' => reset($error_filters),
   +      '%filters' => implode(', ', $error_filters),
   +    ]);
   

   Nice idea to use formatPlural! Unfortunately you cannot split out the messages into separate variables because the PO extractor will not recognize the strings that way, making them in effect untranslatable.

3. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,406 @@
   +    // need to generate a counter that takes into account all the
   +    // relevant information about this field and the referenced entity
   +    // that is being rendered.
   ...
   +    $recursive_render_id = $media->uuid();
   

   But that's not really true, is it? In particular the field part...

4. +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   @@ -0,0 +1,406 @@
   +        $media->{$image_field}->alt = $node->getAttribute('alt');
   ...
   +        $media->thumbnail->alt = $node->getAttribute('alt');
   ...
   +        $media->{$image_field}->title = $node->getAttribute('title');
   +        $media->thumbnail->title = $node->getAttribute('title');
   

   The media entity might be in the static cache of the entity storage, so we should clone it before modifying it, otherwise these changes may leak to unexpected places.

When I go back to the front page, and then see that article as a teaser, it just displays the word "and", but without rendering the media above or the missing media below.

#70: I cannot reproduce this. I just see the first embedded media.

I found out what was happening. What I was testing when reporting that was a YouTube video as the 1st media item. And the problem with that is that it renders as an <iframe>. TextTrimmedFormatter::preRenderSummary() then trims that, which converts it from a MarkupInterface object to a plain string, and assigns it to #markup. Because it's now a plain string, Renderer::ensureMarkupIsSafe() strips out the iframe.

I'm not sure what the correct fix for this is. Maybe text_summary() needs to return a MarkupInterface object if it is passed one. Though we'd have to think about whether that's a secure thing to do. If something along those lines is the proper fix, then that can be a separate issue that doesn't need to block this one.

#89: Oh, fascinating! Reproduced. With no content in a body field other than an embedded video, I see this:

… that's indeed very broken. Not only does the video not show up, the "node links" end up in something that sort of looks like a caption. 🤯 Turns out the HTML is completely broken. The first bit looks okay:

<div property="schema:text" data-quickedit-field-id="node/9/body/en/teaser" class="clearfix text-formatted field field--name-body field--type-text-with-summary field--label-hidden field__item"><figure role="group" class="caption caption-drupal-media">
<article data-align="center" class="media media--type-remote-video media--view-mode-full">
  
      
            <div data-quickedit-field-id="media/5/field_media_oembed_video/en/full" class="field field--name-field-media-oembed-video field--type-string field--label-hidden field__item">

… but what then follows isn't the remainder of the embedded media, but an automatically inserted closing tag (albeit only for the last opening tag, not for the previous ones), followed by the "node links" HTML:

</div>
        <div class="node__links">
    <ul class="links inline"><li class="node-readmore"><a href="/node/9" rel="tag" title="video embed test" hreflang="en">Read more<span class="visually-hidden"> about video embed test</span></a></li><li class="comment-add"><a href="/node/9#comment-form" title="Share your thoughts and opinions." hreflang="en">Add new comment</a></li></ul>  </div>

Ouch.

This is not caused by this issue, it's a pre-existing bug. But once this lands, it'll be a problem many people will encounter. But precisely because it's a pre-existing issue, I can't imagine nobody else has run into this. So I searched. And my search attempt didn't disappoint, although it sure is frightening.
I present to you #221257: text_summary() should output valid HTML and Unicode text, and not count markup characters as part of the text length. Let me point out that this is a six-digit node ID. Yes, it dates back to a long time ago. Nearly 11.5 years ago to be precise 😨 Right in the issue summary it says this:

The current code generates invalid HTML, such as summaries that end in the middle of a closing tag. In fact, current tests require this mis-behavior, as does the WYSIWYG module.

This is exactly what's happening! The last significant work on this happened more than four years ago.

#88: Thanks for the kind words! :) But remember, this is built off years of work by the https://www.drupal.org/project/entity_embed maintainers. It's much easier to see a clean path forward when the v1 was already written and a cleaner, simpler v2 can be extracted from it :)

1. ✅
2. ✅ — I remember pointing this out in a review but apparently that never was addressed. Easy fix :)
3. 🦅👁✅ — A mistake I made in porting to core!
4. 🦅👁✅

Woah! YAY! Thanks, @phenaproxima!

But … I'm going to do something unexpected and un-RTBC it myself, because I think we should answer @effulgentsia's concern in #73 wrt data-view-mode (and the ensuing discussion between @effulgentsia and @Berdir until #76). I responded to that in #81. Let me quote those two comments in full, since that is AFAICT the only remaining concern and those two comments capture the key points and potential solutions, so it makes sense to not force every reader who's following along to be scrolling like a maniac :P

@effulgentsia in #73:

when I go back to the front page, and then see that article as a teaser

That reminds me... Something that I would want if I were building a site is to be able to render the media in a different view mode based on the view mode of the host entity. For example, if I'm rendering the article in full, then maybe I want the embedded media in full. And if I'm rendering the article as a teaser, then maybe I want to render the embedded media as a teaser/thumbnail/whatever. In fact, I might want to enforce this, and not allow content authors to specify a data-view-mode attribute when inserting the media. That way, presentation decisions are kept in the Manage Display configurations (where they belong), and not inside the content.

Maybe it makes sense to punt that to contrib? But I'm not sure, because I'd rather that core encourage best practices, and I'm not sure if it's best practice for core to steer people into making presentation choices inside their content. For example, when using a Media field, content authors are not able to pick the view mode as part of adding media to that field: they can only do that via the field's formatter.

@Wim Leers in #81:

#73: If I understand your comment correctly, you're wondering if we shouldn't enforce a single view mode by default? If so: YES PLEASE! 👏👏👏👏👏👏 This is exactly what I proposed during the Barcelona Media Sprint in December, and it's documented in the 4th bullet of #25. I personally believe this would be a massive improvement: choosing a view mode for embedded media is cool, but how often does it really make sense to embed media in Full or Media Library view mode (the two defaults core ships with)? And even if we add an Embed view mode by default, when does it make sense to select one of those other two?
I totally see how a Tiny embed and Detailed embed could be valuable. But that is a relatively advanced use case — used by perhaps 10 or 20% of Drupal 8 sites. Why not defer that to a media_embed_viewmode contrib module?
This would simplify so much. 🙏

Assigning to @effulgentsia.

I don't think site admins wanting to provide multiple view mode options is an edge case. We're using entity embed heavily in D8 with lots of view modes. For example, if an image entity is selected for embed, we have options to control the size of the image embed. We also have an "image group" media entity, and we have view modes to control how the images should be arranged (slider? cluster? stacked?).

That said, I think that just means I'm in agreement with the previous comments! The view mode will be customizable in the embed token. We can further discuss the ability to select view mode in #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library (maybe punting to contrib).

For me, the key for this issue is that we should not require the data-view-mode attribute. In other words, these code hunks:

+++ b/core/modules/media/media.module
@@ -359,3 +359,141 @@ function media_entity_type_alter(array &$entity_types) {
+      $required_attributes = [
...
+        'data-view-mode',
+      ];
+++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
@@ -0,0 +1,405 @@
+    foreach ($xpath->query('//drupal-media[@data-entity-type="media" and normalize-space(@data-view-mode)!="" and normalize-space(@data-entity-uuid)!=""]') as $node) {

In order to not require it, we need a default. Probably the simplest way to represent that default is as a setting for the filter per #75. Unless there's other ideas for where/how to represent that default?

I think #76 is potentially worth doing as well, but that would be a separate setting (constraining the options rather than applying a default when no option is selected), but IMO, it's fine for that second setting to be added in a followup.

Setting to NW, but I'm also open to arguments for why we need the attribute to be required. I don't think I've heard/read any, but maybe I missed something in the previous comments?

but I'm also open to arguments for why we need the attribute to be required. I don't think I've heard/read any, but maybe I missed something in the previous comments?

I actually provided those reasons in #68. I'll repeat my answer here:

RE: data-view-mode: explicitness. Because there are lots of surprising aspects to this:

1. This is a blob of HTML. Non-Drupal experts may look at it. It may be migrated at some point.
2. Maybe the default of full on EntityViewBuilderInterface::view() disappears.
3. full is a lie for Node entities: if you load full, you'll end up with default.
4. This will be exposed to non-Drupal consumers via JSON:API, GraphQL and REST. We should not leave semantics open for interpretation when we know that we expect it to behave in a particular way in Drupal itself.

Weighing the trade-off of this spectrum of potential problems versus having to specify data-view-mode="full", and taking into account the fact that nobody will be writing that HTML themselves manually since it'll be generated by #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library, I think it is prudent to be more specific.

I am not convinced those reasons are more important than the simplicity of making data-view-mode optional. So I think @effulgentsia's proposal makes sense.

Overall, #96 is in line with what I anticipated @effulgentsia would say 😊

However, if we're going to elect a default, we then also need to agree in this issue what that default would be. Currently the Media module only ships with a single default view mode: full. That view mode is designed for the canonical route for media entities. Yes, as of #3017935: Media items should not be available at /media/{id} to users with 'view media' permission by default , the "standalone URL" is no longer available — see https://www.drupal.org/node/3018742. But that view mode is still inextricably coupled to that. It is also that name that is used for all other entity types and hence comes with a certain expectation.

(Installing the Media Library module adds another view mode for media entities: media_library. But that one also isn't a good fit for embedding. And the Media module can't rely on a view mode that only exists if another module is installed.)

That means that if we want to choose a default without expanding the scope of this patch, we can only choose full. So did that. That means now this appears in the text format configuration UI when the media_embed filter is enabled:

I personally still think we should add an Embed view mode, but deciding on the details of each of the corresponding view displays will likely involve bikeshedding, so I think we should defer that to a follow-up issue. Tagged Needs followup.

So IMHO next steps are:

1. People agreeing with @effulgentsia and hence this patch can RTBC this patch 🤞🥳
2. If we also agree we need as new view mode, we can create that follow-up issue. If we disagree, and want to stick to full as the default, then we can just remove Needs followup.

Failing due to

Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was Drupal\media\Plugin\Filter\MediaEmbed::disableContextualLinks. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725

which was introduced in #2966327: Limit what can be called by a callback in render arrays to reduce the risk of RCE just a few days ago.

(Also fixed coding standards violations.)

Yay, #99 is green again! Rerolled all subsequent patches too:

• #2994696-23: Render embedded media items in CKEditor
• #2994699-24: Create a CKEditor plugin to select and embed a media item from the Media Library
• #2994702-17: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`

Excellent work here. I reviewed the code and ran the tests tonight. The only thing I could find were minor language nits.

1) Not a blocker, but I noticed the keys are inconsistent in MediaEmbedFilterConfigurationUiTest::providerTestValidations(), about half way through it switches from keys like this:

        'filters[filter_html][status]' => FALSE,
        'filters[filter_align][status]' => FALSE,
        'filters[filter_caption][status]' => FALSE,
        'filters[filter_html_image_secure][status]' => FALSE,

to keys like this:

        'filter_html' => TRUE,
        'filter_align' => TRUE,
        'filter_caption' => TRUE,
        'filter_html_image_secure' => TRUE,

I know it doesn't really matter, but I think it would be nicer to be consistent. I'm looking for things that could be improved.

2) minor nit, I think considering that the only entity type for embedded media is media, that calling it an embedded media media is redundant. I would change the key here to 'user cannot access embedded media' or 'user cannot access embedded media entity'. This is especially true because they next key doesn't have the redundancy, but reads 'user can access embedded media' .

+  /**
+   * Data provider for testAccessUnpublished().
+   */
+  public function providerAccessUnpublished() {
+    return [
+      'user cannot access embedded media media' => [

3) in MediaEmbedFilterTranslationTest::testTranslationSelection(), the help text is a bit confusing due to the use of the word "either":

a particular translation of the embedded entity is selected based on either the host entity's language, which should require a cache context to be associated.

I would expect an "or" conjunction to follow the use of "either". Should the word "either" just be dropped? "a particular translation of the embedded entity is based on the host entity's language, which should require a cache context to be associated."

4) Another minor nit about comment verb usage.

// Any attributes not consumed by the filter should be persisted onto
// the rendered embedded entity. For example, `data-align` and
// `data-caption` should persist,

The usage "to be persisted" is unidiomatic, albeit clear enough. Persist is something something does, not something that is done to something. I would suggest this usage:

"should be carried over to the rendered embedded entity. For example, `data-align` and `data-caption` should be carried over.

Also since the attributes are being transferred from one place to another, persist is not really the right verb for what they are doing.

1. 👍✅
2. 🦅👁✅(just a typo)
3. 🦅👁✅ (oversight in porting, dropped "either")
4. Woah! You truly are the eagle-eyed reviewer! I have not seen you pay this much attention to detail before, in the Entity Embed issue queue, but I'm super grateful you're doing this! And better yet, you're helping me improve my English at the same time! 🙏🥳✅

Keeping RTBC because all of the changes are in comments.

Now that this is RTBC, I want to make sure all people who ever contributed are credited. People who contributed to Entity Embed's src/Plugin/Filter/EntityEmbedFilter.php, which is the spiritual predecessor:

Berdir
cs_shadow
Dave Reid
deepakkumar14
Denchev
dpi
gngn
jibran
marcoscano
oknate
phenaproxima
samuel.mortenson
slashrsm
stevector
webflo
Wim Leers

Re #90: #3067116: text_summary() returns malformed (not normalized) HTML for basic_html and other formats that use filter_html instead of filter_htmlcorrector

Note that #90 encountered a different problem than #89. Here's the issue for #89: #3067124: text_summary() returns a plain string, even if passed a MarkupInterface object.

Crediting @andrewmacpherson for #40.

Thank you for everyone's great work on the Entity Embed project and in this core issue! Pushed to 8.8.x! See you in the next step in #2994696: Render embedded media items in CKEditor.

Yay! Rerolled patch at #2994696-24: Render embedded media items in CKEditor, awaiting reviews from everyone!

redacted

Tagging for potential mention in the release highlights as one of the many new features offered by the stable Media Library.

Also, as a significant API addition, this perhaps merits a CR? (Not the release notes though; there's no disruption.)

There's no API, this is just product-level functionality. Unless we consider all filters "APIs", which would be news to me 😅

Agreed about change record, created one: https://www.drupal.org/node/3087775.

Regarding #46, which makes this comment:

"consumed" attributes should be removed, just like \Drupal\filter\Plugin\Filter\FilterAlign and \Drupal\filter\Plugin\Filter\FilterCaption do.

Does anyone know *why* these are removed? I can't see that they do any harm, and preserving them provides very useful context for downstream filters, should they want to act on them. For instance, I'd like my media templates to be aware that there was a caption on this entity. I know that, for my specific use-case, I should be asking about why the Caption filter removes its attribute, but the question is basically the same. Anyone have any thoughts?