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!
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2741373-15.patch | 5.27 KB | phenaproxima |
| #12 | 2741373-10.patch | 2.62 KB | phenaproxima |
| #9 | 2741373-8.patch | 0 bytes | phenaproxima |
| #6 | 2741373-6.patch | 694 bytes | phenaproxima |
| #3 | 2741373-3.patch | 758 bytes | phenaproxima |
Comments
Comment #2
phenaproximaComment #3
phenaproximaThis 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!
Comment #4
dave reidYay this looks like a very, very nice and simple fix.
Do these values *have* to be defined?
Comment #5
phenaproximaI doubt it, but I wanted to keep things consistent with what's in check_markup(), just in case.
Comment #6
phenaproximaNope, turns out we don't need #langcode and #filter_types_to_skip. Fixed in this patch.
Comment #7
dave reidSo now the question is, we probably need tests?
Comment #8
phenaproximaAdded 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.
Comment #9
phenaproximaMaybe it would be helpful to attach the patch.
Comment #12
phenaproximaI should also generate the patch correctly.
Comment #15
phenaproximaBehold, a passing functional test!
Comment #17
slashrsm commentedCommitted. Thanks!