Embed specific attributes are cached along with the entity

Cross posting from github:
Problem was that the data attributes were added to the embedded entity itself. This caused the same data attributes to appear everywhere the same view mode was used/displayed on the site. That also meant that the cached version was not invalidated until the embedded entity's cache was cleared.

To prevent all mentioned problems we wrap the embedded entity and append the data attributes to the wrapper element.

Pull request: https://github.com/drupal-media/entity_embed/pull/220

The filter that renders embedded entities must associate the necessary cacheability metadata. It sounds like that's not happening.

This is therefore basically a duplicate of #2593379: Analyze/improve/fix handling of bubbleable metadata in entity_embed. This only covers one of the many potential side-effects/consequences. The lack of test coverage for this is what's the real problem: EntityEmbedFilterTest only tests if something works at all, not if it is updated when necessary. We should at least test:

• that if an embedded entity is modified, the host entity's rendered output is also updated
• that if a tag on an embedded entity is added or removed, the host entity's rendered output is also updated
• that an embedded entity that becomes unpublished, it is also no longer rendered by the host entity's rendered output, because it's no longer accessible … but only for anonymous users

Even if they're not the exact same problem (which means this issue summary doesn't do a good enough job explaining the problem), they're at least strongly related.

I stand by my statement that the real problem is the lack of test coverage for cache-related aspects of the Entity Embed filter. If we'd have that, then there would also be far less potential for confusion.

Cool :) I will be able to review this patch. If you get stuck, I should be able to find time to help with the patch itself, but that may take longer. So, you rolling the patch and me unblocking you on reviews would be perfect IMO.

I'm attaching the test scenario that should indicate what the main problem is.

Adding the wrapper broke EntityEmbedHooksTest tests.

Can we check if the caption & alignment filter in core removes the data attributes or keeps them on image elements?

+++ b/src/EntityHelperTrait.php
@@ -213,11 +217,6 @@ trait EntityHelperTrait {
-    // If this is an image, the image_formatter template expects #item_attributes.
-    if (!empty($build['#theme']) && !empty($build['#attributes']) && $build['#theme'] == 'image_formatter') {
-      $build['#item_attributes'] =  $build['#attributes'];
-    }

I'm guessing we lack test coverage for this, since I would wager this is a regression.

I guess my next question, is with the wrapping element, how would #2640398: Allow <drupal-entity> elements to be inline CKEditor Widgets be possible with the wrapping div forced back on now? Eventually supporting inline displays of file entities was on the list, and I would like to make sure we're not backing ourselves into a corner again.

After reading more about cache contexts and render cacheability, I think the wrapper might be the only way.

Seems OK to me. After re-reading the W3C spec (https://www.w3.org/TR/html5/sections.html#the-article-element) looks like <article> is an appropriate choice for the wrapper element.
...but shouldn't we add some tests for the wrapper too?

Updated tests with asserts for the new wrapper element....

....argh. Screwed up creating the patch :) Uploading a new one. Interdiff still holds.

<article> is already used for nodes, so now we would have two consecutive/nested article tags, which seems like it's heading in a strange direction?

It is a bit strange, I agree. On the other hand the spec states

When article elements are nested, the inner article elements represent articles that are in principle related to the contents of the outer article.

and from this perspective it might be OK.
IMHO either <article> or <span> should be used.