Problem/Motivation

IIRC entity_embed chose to use #post_render_cache to be able to support entity access checking correctly. At the time, this was the only solution possible to only show embedded entities if the current user could access them. It does come at the cost of significantly worse cacheability though!

Since then, #2429257: Bubble cache contexts has landed. This makes it possible to bubble cache contexts. Access results specify cache contexts. This is how we fixed #2099137: Entity/field access and node grants not taken into account with core cache contexts, which is basically the same problem.

Proposed resolution

Make entity_embed bubble the cacheability metadata associated with an entity's access result.

Basically:

$result = new FilterProcessResult($text);

// [Do the work of rendering the entity and its access checking.]
$access = …
$entity = …

// Make the FilterProcessResult have the cacheability metadata of the entity access result and the rendered entity itself.
$result = $result->merge($access)->merge($entity);

$result->setProcessedText(str_replace(…));

Remaining tasks

Discuss.

User interface changes

None.

API changes

The "context alter" hook would go away.

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Happy to provide a patch if desired.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Reconsider use of #post_render_cache » Reconsider use of #post_render_cache now that we have cache context bubbling
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Excellent Wim, I'll take a look at implementing this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Category: Task » Bug report

#2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache landed, #post_render_cache is gone, so by now this is a bug; it'll prevent Entity Embed in its current state from working.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Title: Reconsider use of #post_render_cache now that we have cache context bubbling » Replace #post_render_cache with #lazy_builder and use cache context bubbling
Assigned: Unassigned » dave reid

Will be working on this this weekend.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Relevant change notice: https://www.drupal.org/node/2498803

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Cool, ping me if you have questions :)

slashrsm’s picture

slashrsm commented
Issue tags: +Media Initiative, +sprint
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

@Wim Leers: So with this new code, we don't actually need placeholders at all? We should just render directly in the filter itself?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Correct.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

@Wim Leers: I'm having problems implementing this part:

// Make the FilterProcessResult have the cacheability metadata of the entity access result and the rendered entity itself.
$result = $result->merge($access)->merge($entity);

Neither using the entity object, nor the AccessResultInterface result from $entity->access() seems to be usable to provide to merge(). Any ideas on where we should look for that?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Hrm, looks like I should be doing something like this?

$result->merge(BubbleableMetadata::createFromObject($entity))->merge(BubbleableMetadata::createFromObject($access));
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Blargh, that doesn't work at all.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

When I attempt to do $result = $result->merge(...) I always get:

Warning: Missing argument 1 for Drupal\filter\FilterProcessResult::__construct(), called in /home/davereid/Sync/projects/drupal8/core/lib/Drupal/Core/Cache/CacheableMetadata.php on line 142 and defined in Drupal\filter\FilterProcessResult->__construct() (line 86 of core/modules/filter/src/FilterProcessResult.php).
Notice: Undefined variable: processed_text in Drupal\filter\FilterProcessResult->__construct() (line 87 of core/modules/filter/src/FilterProcessResult.php).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

So, FilterProcessResult extends BubbleableMetadata. BubbleableMetadata::merge() typehints to CacheableMetadata, which is the base class of BubbleableMetadata. So you can give it either CacheableMetadata or BubbleableMetadata objects.

This is therefore what you want:

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

Yes, this is painful. This is why for regular rendering code, we have RendererInterface::addCacheableDependency(). We could consider adding that to BubbleableMetadata/CacheableMetadata as well. Then it'd become simply:

$result = $result->addCacheableDependency($access)
  ->addCacheableDependency($entity);
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

(We cross-posted.)

Warning: Missing argument 1 for Drupal\filter\FilterProcessResult::__construct(), called in /home/davereid/Sync/projects/drupal8/core/lib/Drupal/Core/Cache/CacheableMetadata.php on line 142 and defined in Drupal\filter\FilterProcessResult->__construct() (line 86 of core/modules/filter/src/FilterProcessResult.php).

Oh :(

That's a bug. This is because of:

  public function merge(CacheableMetadata $other) {
    $result = new static();

Which means that upon merging, a new FilterProcessResult object is constructed, but it's not passed any parameters, even though BubbleableMetadata expects that.

IOW: You've found a core bug.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Shouldn't it be using $result = clone $this instead?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think you did :D :D

Wanna file a core patch? :)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Postponed

Filed #2516802: FilterProcessResult->merge() results in PHP warning: Missing argument 1 for FilterProcessResult::__construct() with patches.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Postponed » Active

#2516802: FilterProcessResult->merge() results in PHP warning: Missing argument 1 for FilterProcessResult::__construct() just landed!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Issue tags: -sprint +D8Media
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Fixed

Ok, calling this good for now.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Can you link to the commit(s) that fixed it?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

@WimLeers: https://github.com/drupal-media/entity_embed/pull/146 was the fix.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.