Cache tags set by #pre_render callbacks are lost in page cache

Does this make #pre_render callbacks less useful, since they can't influence $element anymore (because they operate on a copy)?

Does this make #pre_render callbacks less useful, since they can't influence $element anymore (because they operate on a copy)?

I think it means that we're potentially running some subset of pre_render functions twice.

It's given me an idea though.

What if we actually just process all the pre_renders all at once #2099131-77: Use #pre_render pattern for entity render caching?

we do the "apply #pre_render callbacks on a copy of the render array" thing, only for the purpose of collecting cache tags.

I cannot help but wonder whether perhaps the cache-tag architecture needs an overhaul? Maybe it would make sense when drupal_render() would assign the cache-tags it encounters to a (request-scope?) service. Then the page cache service just may collect them from there?

@Wim Leers, there is no need to alter the way drupal_render() bubbles up the cache tags for its own usage. I think it would be enough when it additionally would pass them to a service where page cache could fetch them from.

This has come up before in relation to #attached - needing a way for the calling function to get those back out of drupal_render().

It's not a fantastic API, but

drupal_render(&$element, &$attached = NULL, &$cache_tags = NULL);

We could then fill those values in if they're passed, no API change.

Or add them to a key on $elements since that's passed by ref anyway?

@Wim probably. I'm slightly wary that despite us discouraging direct use of drupal_render(), we still have 274 calls in core.

#14 What about returning an object with a __toString() magic method? Then we could retain backward compatibility and still it would be possible to access rendering metadata:

// Old style:
print drupal_render($element);

// New style:
$result = drupal_render($element);
print $result->getMarkup();
var_dump($result->getTags());
var_dump($result->getAttached());

Perhaps we even could reuse (or extend) \Drupal\Core\Page\HtmlFragment? It already implements CacheableInterface.

Have to be careful with __toString() because you can't throw exceptions, but in this case we'll have the string already there and just return it, so yes that seems fine :)

Turning the return of drupal_render into an object will give me additional options to explore in #2099131: Use #pre_render pattern for entity render caching as well.

Cool, I think we have a direction and znerol has a mandate to explore their suggestion. Let's mark this as Needs Work for now.

I think something like #14 is the way to go. I don't see this being workable unless we have some object to encapsulate it.

I had a quick stab on #14 but regrettably __toString does not help us out of this. The problem is that calling code does all sorts of things with the result of drupal_render. In some cases the result is straightly encoded into JSON and ends up as object instead of as a string there. That leads to leads to all sorts of UI-regressions, including broken contextual links etc.

One solution which might work is extracting the inners of drupal_render into another function (say drupal_render_to_fragment returns a HTMLFragment - or even better into a method on a helper class). Then use something like this for backward compatibility:

function drupal_render(...) {
  return drupal_render_to_fragment(...)->getContent();
}

However that does not yet really solve the original issue yet. We still have the problem, that at the moment there is no sane way to pass tags collected in HtmlViewSubscriber::onHtmlPage to drupal_page_set_cache. I wonder whether it would be justifiable to get rid of FinishResponseSubscriber::onRespond and just move that over to HtmlViewSubscriber::onHtmlPage?

Though if we go that way, some content types will not be cached anymore. Especially Ajax responses handled by ViewSubscriber. However the lack of cache tag handling over there indicates that we need to address this anyway in order to make #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly happen. Otherwise we might end up with stale cached Ajax responses delivered through the page cache.

Given that this issue blocks #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags and there is no obvious easy way out, I think that #1 should go in.

This looks a lot better to me, thanks!

Looks quite reasonable to me.

There is also the issue the preprocess and templates can't set cache tags or attach assets but especially preprocess might render referenced data ...

Not sure this belongs here, but its an issue for sure.

Committed/pushed to 8.x, thanks!