Skip to content
This repository has been archived by the owner on Jan 5, 2018. It is now read-only.

Improve/fix handling of bubbleable metadata in entity_embed #187

Open
wants to merge 1 commit into
base: 8.x-1.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions src/EntityHelperTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ protected function renderEntity(EntityInterface $entity, $view_mode, $langcode =
* (optional) Array of context values, corresponding to the attributes on
* the embed HTML tag.
*
* @return string
* The HTML of the entity rendered with the display plugin.
* @return array
* A render array from entity_view().
*/
protected function renderEntityEmbed(EntityInterface $entity, array $context = array()) {
// Support the deprecated view-mode data attribute.
Expand All @@ -185,8 +185,8 @@ protected function renderEntityEmbed(EntityInterface $entity, array $context = a
// 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);

// Build and render the display plugin, allowing modules to alter the
// result before rendering.
// Build the display plugin, allowing modules to alter the result before
// rendering.
$build = $this->renderEntityEmbedDisplayPlugin(
$entity,
$context['data-entity-embed-display'],
Expand All @@ -195,9 +195,7 @@ protected function renderEntityEmbed(EntityInterface $entity, array $context = a
);
// @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;
}

/**
Expand Down
21 changes: 17 additions & 4 deletions src/Plugin/Filter/EntityEmbedFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Render\BubbleableMetadata;
use Drupal\Core\Render\RenderContext;
use Drupal\entity_embed\EntityHelperTrait;
use Drupal\entity_embed\Exception\EntityNotFoundException;
use Drupal\entity_embed\Exception\RecursiveRenderingException;
Expand Down Expand Up @@ -102,13 +104,24 @@ public function process($text, $langcode) {
}

$access = $entity->access('view', NULL, TRUE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it ok to remove the metadata about the entity itself? Are we assuming that every field formatter that can be re-used to display an embedded entity correctly has this metadata in its render API array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks like we're losing the metadata from the access check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the second line because we are rendering the embedded entity, that will itself include the cacheability metadata of the entity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks like we're losing the metadata from the access check.
Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, because I couldn't see the very next line:
$result->addCacheableDependency($access);

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

$context = $this->getNodeAttributesAsArray($node);
$context += array('data-langcode' => $langcode);
$entity_output = $this->renderEntityEmbed($entity, $context);
$build = $this->renderEntityEmbed($entity, $context);
// We need to render the embedded entity:
// - without replacing placeholders, so that the placeholders are
// only replaced at the last possible moment. Hence we cannot use
// either renderPlain() or renderRoot(), so we must use render().
// - without bubbling beyond this filter, because filters must
// ensure that the bubbleable metadata for the changes they make
// when filtering text makes it onto the FilterProcessResult
// object that they return ($result). To prevent that bubbling, we
// must wrap the call to render() in a render context.
$entity_output = $this->renderer()->executeInRenderContext(new RenderContext(), function () use (&$build) {
return $this->renderer()->render($build);
});
$result = $result->merge(BubbleableMetadata::createFromRenderArray($build));

$depth--;
}
Expand Down