Analyze/improve/fix handling of bubbleable metadata in entity_embed

A very big one: inaccessible entities are still rendered…! \Drupal\entity_embed\Plugin\Filter\EntityEmbedFilter::process() checks access, but then still happily renders inaccessible entities. This also means access is not checked for entities embedded using the Twig extension.
Except apparently EntityHelperTrait::renderEntityEmbedDisplayPlugin() does check access:

But it returns the empty render array, which means cacheability metadata for access checks is lost.

Even though we specifically used the following in the filter? I don't quite understand the point you're trying to make here.

           $access_metadata = CacheableMetadata::createFromObject($access);
           $entity_metadata = CacheableMetadata::createFromObject($entity);
           $result = $result->merge($entity_metadata)->merge($access_metadata);

Maybe the renderEntityEmbed() method should have an optional BubbleableMetadata parameter and merges in the access data and the info from the render array?

How do we solve missing metadata when embedded with Twig?

Ooh, renderEntityEmbed should totally do that!

This is a re-roll of Wim's pull request.

I think that we should add test coverage for this.

If I understand correctly this should address the problem nr. 3 in the issue summary. Also added test for that part which we don't have currently. We still need to add test for the cache metadata bubbling part.

+++ b/src/EntityHelperTrait.php
@@ -218,11 +213,12 @@ trait EntityHelperTrait {
+    // Make sure that access to the entity is respected.
+    $build['#access'] = $entity->access('view', NULL, TRUE);
+
     // @todo Should this hook get invoked if $build is an empty array?
     $this->moduleHandler()->alter(array("{$context['data-entity-type']}_embed", 'entity_embed'), $build, $entity, $context);
-    $entity_output = $this->renderer()->render($build);
-
-    return $entity_output;
+    return $build;

I think the problem here is that we're now calling the rendering no matter if the user can access it or not. I guess that might be the point?

This behavior seems odd to me because of what happens currently in Entity Reference field formatters: EntityReferenceFormatterBase and EntityReferenceEntityFormatter. While the account for merging in the access metadata, the field formatters do not actually render the inaccessible entities. It really doesn't feel like we should not be calling the building if the access is not allowed.

I wonder if this could be fixed by ensuring the first time we call $entity->access() we ensure that the access metadata is bubbled up properly.

I think this is what I was more thinking, with moving the entity access check *only* to the renderEntityEmbed() method, which also adds the metadata of both the access and the render array itself. This removes the entity access checking from the display plugin, which I think actually makes a lot of sense, and shouldn't break any backwards compatibility.

Benefits of #15, there is now only one centrally located call to $entity->access().

Are you sure this really is a problem? It seems that renderer will early-return an empty string if it determines that the user doesn't have access to an element. If that is true then it doesn't really matter from performance standpoint.

If the above statement is true then I'd prefer #13 because we end up with simpler code. If I am wrong then I am OK with #15 too.

Would be great to get some feedback from @Wim Leers about this.

Needs reroll. Let's decide on the approach first.

Reroll of #13.

This should fix tests.

Extended tests according to 21. User that has permission to view unpublished content is currently failing. Test if embeded content is displayed when its published is right below that, isn't that enough. The test for bubbleuble mettadata is not added yet

Also we have a bug here. When we try to embed a node the access check in ImageFieldFormatter trows an exception because of $entity->getFileUri()

Fatal error: Call to undefined method Drupal\node\Entity\Node::getFileUri()

Yea, sorry. addRole doesn't save the user. Tests are passing now. continuing with the rest.

1. +++ b/src/EntityEmbedDisplay/EntityEmbedDisplayBase.php
   @@ -261,6 +248,13 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
   +   * @todo Where doe sthis come from? The value must come from somewhere, yet
   +   * this does not implement any context-related interfaces. This is an *input*,
   +   * so we need cache contexts and possibly cache tags to reflect where this
   +   * came from. We need that for *everything* that this class does that relies
   +   * on this, plus any of its subclasses. Right now, this is effectively a
   +   * global that breaks cacheability metadata.
   +   *
   

   The context is referring to the individual embeds that are being rendered. So node 1 is the context of

   In fact the whole data from the embed code is "context".

2. +++ b/src/EntityHelperTrait.php
   @@ -144,14 +136,19 @@ trait EntityHelperTrait {
   +   * @todo Note that the signature here does NOT match that of \Drupal\Core\Entity\EntityViewBuilderInterface::view(), which can lead to subtle bugs.
   

   The only thing that's different is that a view mode is required. How does this lead to subtle bugs?

3. +++ b/src/EntityHelperTrait.php
   @@ -159,10 +156,12 @@ trait EntityHelperTrait {
       *   (optional) Array of context values, corresponding to the attributes on
       *   the embed HTML tag.
   

   FYI here is the context documentation in case you missed it.

4. +++ b/src/EntityHelperTrait.php
   @@ -219,15 +218,16 @@ trait EntityHelperTrait {
   +    // Make sure that access to the entity is respected.
   +    $build['#access'] = $entity->access('view', NULL, TRUE);
   +
   

   My main issue still and has always been that it seems logically very wrong to do all the rendering here, if the user can't even access it anyway. This change is not shared by ntityReferenceEntityFormatter::viewElements(). It merges the access metadata, but then does not render inaccessible entities. That's what I was trying to do in #15, merge the access metadata, but avoid rendering if no access. Why isn't doing that enough to ensure that when someone that cannot access a node should get a different cache than someone who can?

+++ b/src/EntityEmbedDisplay/EntityEmbedDisplayBase.php
@@ -72,20 +68,11 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
-    // @todo Add a hook_entity_embed_display_access()?

Please don't remove existing @todos.

Bug fix and some test coverage.

1. +++ b/src/EntityEmbedDisplay/FieldFormatterEntityEmbedDisplayBase.php
   @@ -112,12 +113,12 @@ abstract class FieldFormatterEntityEmbedDisplayBase extends EntityEmbedDisplayBa
   +  protected function isApplicableFieldFormatter() {
        $definition = $this->formatterPluginManager->getDefinition($this->getDerivativeId());
   -    return $definition['class']::isApplicable($this->getFieldDefinition());
   +    return AccessResult::allowedIf($definition['class']::isApplicable($this->getFieldDefinition()));
      }
   

   Needs doc block.

2. +++ b/src/EntityHelperTrait.php
   @@ -22,6 +16,11 @@ use Drupal\entity_embed\EntityEmbedDisplay\EntityEmbedDisplayManager;
   + *
   + * @todo this duplicates/wraps much of the Entity API. Is this really worth
   + * keeping? The downside is painfully illustrated: the documentation must be
   + * kept up to date with the actual API documentation… and that's not happening.
   + * This causes subtle bugs and makes maintenance harder.
   

   Agreed. We should create a follow-up and clean this up.

3. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ImageFieldFormatter.php
   @@ -85,15 +86,28 @@ class ImageFieldFormatter extends FileFieldFormatter {
   +  protected function isValidImage() {
   

   Needs doc block.

4. +++ b/src/Tests/EntityEmbedDialogTest.php
   @@ -102,6 +104,17 @@ class EntityEmbedDialogTest extends EntityEmbedTestBase {
   +   * Tests entity embed functionality.
   +   */
   +  public function testEntityEmbedFunctionality() {
   +    $edit = [
   +      'entity_id' => $this->node->getTitle() . ' (' . $this->node->id() . ')',
   +    ];
   +    $this->getEmbedDialog('custom_format', 'node');
   +    $this->drupalPostForm(NULL, $edit, t('Next'));
   +  }
   

   What exactly are we testing with this function? We need to improve tests or add a comment to explain the purpose of this submit.

5. +++ b/src/Tests/EntityEmbedFilterTest.php
   @@ -48,6 +43,30 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +    $this->webUser->removeRole('access_unpublished');
   

   Need to save user again.

Addressed #38
added test for bubbleable metadata. Looking for some feedback on it.
Looks like EntityEmbedFilterTest is failing because of the access hook looking into it

This should correctly test the cache tags. Not sure about the cache context, there is validation that trows exception when I add nonsensical cache context. Still need to check why the embeded node is apearing when the user doesn't have permission.

This should make tests pass.

Few nitpiks. I think this should be ready. All items from the IS were fixed and test coverage was vastly improved.

+++ b/src/EntityEmbedDisplay/EntityEmbedDisplayBase.php
@@ -261,6 +249,13 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
    *
+   * @todo Where doe sthis come from? The value must come from somewhere, yet
+   * this does not implement any context-related interfaces. This is an *input*,
+   * so we need cache contexts and possibly cache tags to reflect where this
+   * came from. We need that for *everything* that this class does that relies
+   * on this, plus any of its subclasses. Right now, this is effectively a
+   * global that breaks cacheability metadata.
+   *

This will always be embedded entity, which should pass all its cache metadata when the render array is being built.

However, the naming is confusing a bit. It seems that the context array always stores just the embedded entity and machine name of its entity type. Should we rename/simplify this to reflect that?

However there is also

    // Allow modules to alter the entity prior to embed rendering.
    $this->moduleHandler()->alter(array("{$context['data-entity-type']}_embed_context", 'entity_embed_context'), $context, $entity);

which seems to be a different context. It is added to the render array. We should at least update the hook documentation to make developers aware that they need to take care about cache metadata depending on how they alter the context. Can be a follow-up I think.

Follow-up created: #2760407: Clean up EntityHelperTrait

Added @todo. Output of getDefinitionsForContexts() is used to determine which display plugins can be presented to the user in the embed dialog and are not cached. I think that we should be fine there?

However, we should improve that to prevent any strangeness in the future, but that will require refactoring of things that are using the output.

Patch.

Committed. Thanks!