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

Conversation

wimleers
Copy link

@@ -102,13 +104,24 @@ public function process($text, $langcode) {
}

$access = $entity->access('view', NULL, TRUE);
$access_metadata = CacheableMetadata::createFromObject($access);
$entity_metadata = CacheableMetadata::createFromObject($entity);
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);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants