Commit c248cda0 authored by Wim Leers's avatar Wim Leers Committed by slashrsm

Issue #2593379 by Wim Leers, Denchev, slashrsm, Dave Reid: Fix handling of bubbleable metadata.

parent 657e32ee
<?php
/**
* @file
* Contains \Drupal\entity_embed\EntityEmbedDisplay\EntityEmbedDisplayBase.
*/
namespace Drupal\entity_embed\EntityEmbedDisplay;
use Drupal\Component\Utility\NestedArray;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Language\LanguageInterface;
......@@ -73,19 +69,11 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
*/
public function access(AccountInterface $account = NULL) {
// @todo Add a hook_entity_embed_display_access()?
// Check that the plugin's registered entity types matches the current
// entity type.
if (!$this->isValidEntityType()) {
return FALSE;
}
// Check that the entity itself can be viewed by the user.
if ($entity = $this->getEntityFromContext()) {
return $entity->access('view', $account);
}
return TRUE;
return AccessResult::allowedIf($this->isValidEntityType())
// @see \Drupal\Core\Entity\EntityTypeManager
->addCacheTags(['entity_types']);
}
/**
......@@ -261,6 +249,13 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
/**
* Gets the entity from the current context.
*
* @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.
*
* @return \Drupal\Core\Entity\EntityInterface
*/
public function getEntityFromContext() {
......
......@@ -58,8 +58,8 @@ interface EntityEmbedDisplayInterface extends ConfigurablePluginInterface, Plugi
* (optional) The user for which to check access, or NULL to check access
* for the current user. Defaults to NULL.
*
* @return bool
* TRUE if this Entity Embed Display plugin can be used, or FALSE otherwise.
* @return \Drupal\Core\Access\AccessResultInterface
* The access result.
*/
public function access(AccountInterface $account = NULL);
......
......@@ -73,7 +73,9 @@ class EntityEmbedDisplayManager extends DefaultPluginManager {
foreach ($contexts as $name => $value) {
$display->setContextValue($name, $value);
}
return $display->access();
// We lose cacheability metadata at this point. We should refactor to
// avoid this. @see https://www.drupal.org/node/2593379#comment-11368447
return $display->access()->isAllowed();
}
catch (PluginException $e) {
return FALSE;
......
......@@ -7,6 +7,7 @@
namespace Drupal\entity_embed\EntityEmbedDisplay;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Field\BaseFieldDefinition;
use Drupal\Core\Field\FormatterPluginManager;
......@@ -112,12 +113,18 @@ abstract class FieldFormatterEntityEmbedDisplayBase extends EntityEmbedDisplayBa
* {@inheritdoc}
*/
public function access(AccountInterface $account = NULL) {
if (!parent::access($account)) {
return FALSE;
}
return parent::access($account)->andIf($this->isApplicableFieldFormatter());
}
/**
* Checks if the field formatter is applicable.
*
* @return \Drupal\Core\Access\AccessResult
* Returns the access result.
*/
protected function isApplicableFieldFormatter() {
$definition = $this->formatterPluginManager->getDefinition($this->getDerivativeId());
return $definition['class']::isApplicable($this->getFieldDefinition());
return AccessResult::allowedIf($definition['class']::isApplicable($this->getFieldDefinition()));
}
/**
......
<?php
/**
* @file
* Contains Drupal\entity_embed\EntityHelperTrait.
*/
namespace Drupal\entity_embed;
use Drupal\Component\Utility\Html;
......@@ -12,7 +7,6 @@ use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Entity\EntityStorageException;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Render\RendererInterface;
use Drupal\entity_embed\EntityEmbedDisplay\EntityEmbedDisplayManager;
/**
......@@ -22,6 +16,11 @@ use Drupal\entity_embed\EntityEmbedDisplay\EntityEmbedDisplayManager;
* classes that would implement ContainerInjectionInterface. Services registered
* in the Container should not use this trait but inject the appropriate service
* directly for easier testing.
*
* @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.
*/
trait EntityHelperTrait {
......@@ -46,13 +45,6 @@ trait EntityHelperTrait {
*/
protected $displayPluginManager;
/**
* The renderer.
*
* @var \Drupal\Core\Render\RendererInterface.
*/
protected $renderer;
/**
* Loads an entity from the database.
*
......@@ -132,7 +124,7 @@ trait EntityHelperTrait {
}
/**
* Returns the render array for an entity.
* Builds the render array for the provided entity.
*
* @param \Drupal\Core\Entity\EntityInterface $entity
* The entity to be rendered.
......@@ -144,14 +136,19 @@ trait EntityHelperTrait {
*
* @return array
* A render array for the entity.
*
* @see \Drupal\Core\Entity\EntityViewBuilderInterface::view()
*
* @todo Note that the signature here does NOT match that of \Drupal\Core\Entity\EntityViewBuilderInterface::view(), which can lead to subtle bugs.
*/
protected function renderEntity(EntityInterface $entity, $view_mode, $langcode = NULL) {
$render_controller = $this->entityManager()->getViewBuilder($entity->getEntityTypeId());
return $render_controller->view($entity, $view_mode, $langcode);
protected function buildEntity(EntityInterface $entity, $view_mode, $langcode = NULL) {
return $this->entityManager()
->getViewBuilder($entity->getEntityTypeId())
->view($entity, $view_mode, $langcode);
}
/**
* Renders an embedded entity.
* Builds the render array for an embedded entity.
*
* @param \Drupal\Core\Entity\EntityInterface $entity
* The entity to be rendered.
......@@ -159,10 +156,12 @@ trait EntityHelperTrait {
* (optional) Array of context values, corresponding to the attributes on
* the embed HTML tag.
*
* @return string
* The HTML of the entity rendered with the Entity Embed Display plugin.
* @return array
* A render array.
*
* @todo improve documentation
*/
protected function renderEntityEmbed(EntityInterface $entity, array $context = array()) {
protected function buildEntityEmbed(EntityInterface $entity, array $context = array()) {
// Support the deprecated view-mode data attribute.
if (isset($context['data-view-mode']) && !isset($context['data-entity-embed-display']) && !isset($context['data-entity-embed-settings'])) {
$context['data-entity-embed-display'] = 'entity_reference:entity_reference_entity_view';
......@@ -199,7 +198,7 @@ trait EntityHelperTrait {
'#entity' => $entity,
'#context' => $context,
);
$build['entity'] = $this->renderEntityEmbedDisplayPlugin(
$build['entity'] = $this->buildEntityEmbedDisplayPlugin(
$entity,
$context['data-entity-embed-display'],
$context['data-entity-embed-settings'],
......@@ -219,15 +218,16 @@ trait EntityHelperTrait {
$build['#attributes']['data-caption'] = $context['data-caption'];
}
// 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;
}
/**
* Renders an entity using an Entity Embed Display plugin.
* Builds the render array for an entity using an Entity Embed Display plugin.
*
* @param \Drupal\Core\Entity\EntityInterface $entity
* The entity to be rendered.
......@@ -242,7 +242,7 @@ trait EntityHelperTrait {
* @return array
* A render array for the Entity Embed Display plugin.
*/
protected function renderEntityEmbedDisplayPlugin(EntityInterface $entity, $plugin_id, array $plugin_configuration = array(), array $context = array()) {
protected function buildEntityEmbedDisplayPlugin(EntityInterface $entity, $plugin_id, array $plugin_configuration = array(), array $context = array()) {
// Build the Entity Embed Display plugin.
/** @var \Drupal\entity_embed\EntityEmbedDisplay\EntityEmbedDisplayBase $display */
$display = $this->displayPluginManager()->createInstance($plugin_id, $plugin_configuration);
......@@ -336,29 +336,4 @@ trait EntityHelperTrait {
return $this;
}
/**
* Returns the renderer.
*
* @return \Drupal\Core\Render\RendererInterface
* The renderer.
*/
protected function renderer() {
if (!isset($this->renderer)) {
$this->renderer = \Drupal::service('renderer');
}
return $this->renderer;
}
/**
* Sets the renderer.
*
* @param \Drupal\Core\Render\RendererInterface $renderer
* The renderer.
*
* @return self
*/
public function setRenderer(RendererInterface $renderer) {
$this->renderer = $renderer;
return $this;
}
}
......@@ -11,7 +11,9 @@ use Drupal\Component\Utility\Html;
use Drupal\Core\Entity\EntityManagerInterface;
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\Core\Render\RendererInterface;
use Drupal\entity_embed\EntityHelperTrait;
use Drupal\entity_embed\Exception\EntityNotFoundException;
use Drupal\entity_embed\Exception\RecursiveRenderingException;
......@@ -34,6 +36,13 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
use EntityHelperTrait;
use DomHelperTrait;
/**
* The renderer service.
*
* @var \Drupal\Core\Render\RendererInterface
*/
protected $renderer;
/**
* Constructs a EntityEmbedFilter object.
*
......@@ -48,10 +57,11 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
* @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
* The Module Handler.
*/
public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler) {
public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, RendererInterface $renderer) {
parent::__construct($configuration, $plugin_id, $plugin_definition);
$this->setEntityManager($entity_manager);
$this->setModuleHandler($module_handler);
$this->renderer = $renderer;
}
/**
......@@ -63,7 +73,8 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
$plugin_id,
$plugin_definition,
$container->get('entity.manager'),
$container->get('module_handler')
$container->get('module_handler'),
$container->get('renderer')
);
}
......@@ -101,14 +112,22 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
$node->setAttribute('data-entity-uuid', $uuid);
}
$access = $entity->access('view', NULL, TRUE);
$access_metadata = CacheableMetadata::createFromObject($access);
$entity_metadata = CacheableMetadata::createFromObject($entity);
$result = $result->merge($entity_metadata)->merge($access_metadata);
$context = $this->getNodeAttributesAsArray($node);
$context += array('data-langcode' => $langcode);
$entity_output = $this->renderEntityEmbed($entity, $context);
$build = $this->buildEntityEmbed($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--;
}
......
......@@ -62,13 +62,4 @@ class FileFieldFormatter extends EntityReferenceFieldFormatter {
return $form;
}
/**
* {@inheritdoc}
*
* @return \Drupal\file\FileInterface
*/
public function getEntityFromContext() {
return parent::getEntityFromContext();
}
}
......@@ -7,6 +7,7 @@
namespace Drupal\entity_embed\Plugin\entity_embed\EntityEmbedDisplay;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Field\FormatterPluginManager;
use Drupal\Core\Form\FormStateInterface;
......@@ -85,15 +86,35 @@ class ImageFieldFormatter extends FileFieldFormatter {
* {@inheritdoc}
*/
public function access(AccountInterface $account = NULL) {
if (!parent::access($account)) {
return FALSE;
return parent::access($account)->andIf($this->isValidImage());
}
/**
* Checks if the image is valid.
*
* @return \Drupal\Core\Access\AccessResult
* Returns the access result.
*/
protected function isValidImage() {
// If entity type is not file we have to return early to prevent fatal in
// the condition above. Access should already be forbidden at this point,
// which means this won't have any effect.
// @see EntityEmbedDisplayBase::access()
if ($this->getEntityTypeFromContext() != 'file') {
return AccessResult::forbidden();
}
$access = AccessResult::allowed();
// @todo needs cacheability metadata for getEntityFromContext.
// @see \Drupal\entity_embed\EntityEmbedDisplay\EntityEmbedDisplayBase::getEntityFromContext()
/** @var \Drupal\file\FileInterface $entity */
if ($entity = $this->getEntityFromContext()) {
return $this->imageFactory->get($entity->getFileUri())->isValid();
$access = AccessResult::allowedIf($this->imageFactory->get($entity->getFileUri())->isValid())
// See the above @todo, this is the best we can do for now.
->addCacheableDependency($entity);
}
return TRUE;
return $access;
}
/**
......
<?php
/**
* @file
* Contains \Drupal\entity_embed\Tests\EntityEmbedDialogTest.
*/
namespace Drupal\entity_embed\Tests;
use Drupal\editor\Entity\Editor;
......@@ -16,6 +11,13 @@ use Drupal\editor\Entity\Editor;
*/
class EntityEmbedDialogTest extends EntityEmbedTestBase {
/**
* Modules to enable.
*
* @var array
*/
public static $modules = ['image'];
/**
* Tests the entity embed dialog.
*/
......@@ -101,6 +103,20 @@ class EntityEmbedDialogTest extends EntityEmbedTestBase {
$this->assertFieldByXPath('//input[contains(@class, "button--primary")]', 'Embed', 'Embed is a primary button');*/
}
/**
* 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'));
// Tests that the embed dialog doesn't trow a fatal in
// ImageFieldFormatter::isValidImage()
$this->assertResponse(200);
}
/**
* Retrieves an embed dialog based on given parameters.
*
......
<?php
/**
* @file
* Contains \Drupal\entity_embed\Tests\EntityEmbedFilterTest.
*/
namespace Drupal\entity_embed\Tests;
/**
......@@ -48,6 +43,31 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
$this->assertNoText(strip_tags($content), 'Placeholder does not appear in the output when embed is successful.');
$this->assertRaw('<article class="embedded-entity">', 'Embed container found.');
// Tests that embedded entity is not rendered if not accessible.
$this->node->setPublished(FALSE)->save();
$settings = [];
$settings['type'] = 'page';
$settings['title'] = 'Test un-accessible entity embed with entity-id and view-mode';
$settings['body'] = [['value' => $content, 'format' => 'custom_format']];
$node = $this->drupalCreateNode($settings);
$this->drupalGet('node/' . $node->id());
$this->assertNoRaw('<drupal-entity data-entity-type="node" data-entity');
$this->assertNoText($this->node->body->value, 'Embedded node does not exist in the page.');
$this->assertNoText(strip_tags($content), 'Placeholder does not appear in the output when embed is successful.');
// Tests that embedded entity is displayed to the user who has the view
// unpublished content permission.
$this->createRole(['view own unpublished content'], 'access_unpublished');
$this->webUser->addRole('access_unpublished');
$this->webUser->save();
$this->drupalGet('node/' . $node->id());
$this->assertNoRaw('<drupal-entity data-entity-type="node" data-entity');
$this->assertText($this->node->body->value, 'Embedded node exists in the page.');
$this->assertNoText(strip_tags($content), 'Placeholder does not appear in the output when embed is successful.');
$this->assertRaw('<article class="embedded-entity">', 'Embed container found.');
$this->webUser->removeRole('access_unpublished');
$this->webUser->save();
$this->node->setPublished(TRUE)->save();
// Tests entity embed using entity UUID and view mode.
$content = '<drupal-entity data-entity-type="node" data-entity-uuid="' . $this->node->uuid() . '" data-view-mode="teaser">This placeholder should not be rendered.</drupal-entity>';
$settings = array();
......@@ -60,6 +80,7 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
$this->assertText($this->node->body->value, 'Embedded node exists in page.');
$this->assertNoText(strip_tags($content), 'Placeholder does not appear in the output when embed is successful.');
$this->assertRaw('<article class="embedded-entity">', 'Embed container found.');
$this->assertCacheTag('foo:' . $this->node->id());
// Ensure that placeholder is not replaced when embed is unsuccessful.
$content = '<drupal-entity data-entity-type="node" data-entity-id="InvalidID" data-view-mode="teaser">This placeholder should be rendered since specified entity does not exists.</drupal-entity>';
......
......@@ -74,7 +74,7 @@ class EntityEmbedTwigExtension extends \Twig_Extension {
'data-entity-embed-display' => $display_plugin,
'data-entity-embed-settings' => $display_settings,
);
return $this->renderEntityEmbed($entity, $context);
return $this->buildEntityEmbed($entity, $context);
}
}
......@@ -5,7 +5,9 @@
* Helper module for the Entity Embed tests.
*/
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Session\AccountInterface;
/**
* Implements hook_theme().
......@@ -72,3 +74,12 @@ function entity_embed_test_entity_embed_alter(array &$build, EntityInterface $en
// Set title of the 'node' entity.
$entity->setTitle("Title set by hook_entity_embed_alter");
}
/**
* Implements hook_entity_access().
*/
function entity_embed_test_entity_access(EntityInterface $entity, $operation, AccountInterface $account) {
if ($entity->label() == 'Embed Test Node') {
return AccessResult::neutral()->addCacheTags(['foo:' . $entity->id()]);
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment