Problem/Motivation

Found when discussing #2528314: template_preprocess_node() does not add cacheability metadata.

node.html.twig does the following:

  {% if not page %}
    <h2{{ title_attributes }}>
      <a href="{{ url }}" rel="bookmark">{{ label }}</a>
    </h2>
  {% endif %}
  {{ title_suffix {{

Assuming that {{front}} is structured as a render array with #cache on it.

If the template does

{{ page }}
Then contexts/tags will get bubbled.

But if you do {% if not page %}

Then it won't.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

dawehner’s picture

Won't those bits result into potential security issues when smart cache gets in? What happens if we miss some crucial cache context, due to that?

Berdir’s picture

90% of the templates are already cached in entities, blocks and views I'd say. smartcache only changes this for page, html and other templates that are not yet cached.

Not sure how this is supposed to be fixed?

page is a simple boolean. It's impossible for twig to figure out where it is coming from or what cacheability metadata is associated with it? Whatever code provides the value for it *has* to provide cacheability for it, if relevant.

Typical examples for this is a render array variable, e.g. the regions in page.html.twig. I can't imagine a case however, where this could be break, as we already need to have the cacheability of which blocks are in that region anyway. It gets more complicated, if you have something like page, e.g. if (moon_phase = 'full') then {{ some_region }}.

But again, there is absolutely nothing that can be done in a generic way, it's impossible to provide cacheability metadata for a non-render-array variable, whatever preprocess or other hook adds the value for it must provide cacheability for it and add it unconditionally, whether it is used or not.

The only thing I can imagine is collecting cacheability in an if if it is a render array and manually bubble it up, not sure if that is even possible. If not, then this is probably a works-as-designed (more like cant-do-anything-about-it)?

dawehner’s picture

The only thing I can imagine is collecting cacheability in an if if it is a render array and manually bubble it up, not sure if that is even possible. If not, then this is probably a works-as-designed (more like cant-do-anything-about-it)?

Can't we collect all cacheability metadata of all the render arrays passed into a template?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

I'm trying to grok what the consequences of this issue are. A specific scenario and the results of that scenario would help.

  • Does this mean the rendered output of the page case gets cached and then displayed incorrectly for the non-page case? I.e., if the non-page case is rendered before the page case, will we get an invalid cache of duplicated node titles that are in both <h1> and <h2>?
  • Or does it only mean that if something changes somehow for the non-page case that is not invalidated by a different tag, then the non-page case might show a stale cache?
  • Let's say the variable is secret_user instead of page. Or is_promotion_season. Is there a risk of cache poisoning with output that is only for the secret user, or only for during the promotion season?
  • In other words, does this mean that the template caching fails to vary by these things, or only that some variants are not necessarily invalidated when one of these things changes if that change does not already result in invalidating a different attached tag?

My gut reaction is that a themer has no reason to expect that putting {% if secret_user %} in their template would also result in the contents of that section being displayed to non-secret users when the cache is warm. So if that's the bug then it seems really bad, and like we would need to turn off caching for templates with conditionals like that, which would probably be an enormous performance hit.

Even if page or secret_user or is_promotion_season are simple booleans in the template itself, they are clearly being set in some earlier part of the render process based on things that have cacheability metadata associated with them. So can we somehow ensure that any renderable things we check in setting up the template variables get their metadata attached to the template such that the template's caching must vary and be invalidated by those things? And how do we make it so people remember to do that?

alexpott’s picture

Issue tags: +Triaged D8 major

Discussed with @xjm, @Cottser, @joelpittet and @laurii. Given that this could cause information disclosure or at the very least displaying the wrong thing we felt this is a major issue. We also discussed whether it was possible for the Twig compiler to detect if and make the result uncacheable or somehow add something that will cause the cache to work as expected.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.