This issue is reproducible when using Embed + Entity Embed to embed any entity that has attached assets or libraries.

1) The user selects an entity to embed (i.e., with Entity Browser).
2) The drupalentity plugin sends an AJAX request to Embed. It passes the outer HTML of the drupal-entity element.
3) The Embed controller receives the element's HTML and runs it through the text processing system. If the text format has the Entity Embed filter enabled, it returns the fully rendered entity, as a string.
4) The rendered entity's HTML is packaged in an AJAX response, sent back to the client, and displayed in CKEditor.

The big gap here is if the entity, or any component of its render array, try to attach libraries (or settings or whatever). This can happen with certain entities that support rich previews using external APIs -- tweets provided by Media Entity Twitter are a prime example. Although the AJAX response itself does support attachments (it uses CommandWithAttachedAssetsTrait), the text processing system has no awareness of them at all. It just happily returns some HTML. The attachments are completely ignored.

This is a pretty big issue and potentially a deal-breaker for including Entity Browser in Lightning. I'm happy to write the patch, but I need some direction in terms of the approach to be taken to fix this. Help!

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Active » Needs review
StatusFileSize
new758 bytes

This patch addresses the problem. It replaces the call to check_markup in EmbedController::preview() with a render array that is essentially identical to what check_markup() builds and renders. But, because it is a render array, all attachment information is ultimately merged into the outgoing AJAX response (which does not happen when using check_markup(), since it returns a string). A beautiful, elegant fix!

dave reid’s picture

Yay this looks like a very, very nice and simple fix.

+++ b/src/Controller/EmbedController.php
@@ -43,9 +43,16 @@ class EmbedController extends ControllerBase {
+      '#filter_types_to_skip' => [],
+      '#langcode' => '',

Do these values *have* to be defined?

phenaproxima’s picture

I doubt it, but I wanted to keep things consistent with what's in check_markup(), just in case.

phenaproxima’s picture

StatusFileSize
new694 bytes

Nope, turns out we don't need #langcode and #filter_types_to_skip. Fixed in this patch.

dave reid’s picture

Issue tags: +Needs tests

So now the question is, we probably need tests?

phenaproxima’s picture

Issue tags: -Needs tests

Added a test. It's not a functional test, because I'd need to create an entire infrastructure in embed_test to support functional testing. Once this is committed, we could easily add a functional test of this to Entity Embed.

phenaproxima’s picture

StatusFileSize
new0 bytes

Maybe it would be helpful to attach the patch.

Status: Needs review » Needs work

The last submitted patch, 9: 2741373-8.patch, failed testing.

The last submitted patch, 9: 2741373-8.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new2.62 KB

I should also generate the patch correctly.

Status: Needs review » Needs work

The last submitted patch, 12: 2741373-10.patch, failed testing.

The last submitted patch, 12: 2741373-10.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new5.27 KB

Behold, a passing functional test!

  • slashrsm committed 1352819 on 8.x-1.x authored by phenaproxima
    Issue #2741373 by phenaproxima, slashrsm, Dave Reid: Embedded previews...
slashrsm’s picture

Status: Needs review » Fixed
Issue tags: +D8Media

Committed. Thanks!

Status: Fixed » Closed (fixed)

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