From #1847768-84: Remove ip_address() by Damien Tournoud:

I specifically mentioned accessing the current request from the side... If you do that, there is no caching possible at the sub-request level anymore... If your assumption is that everyone is being to play nice and that you can predict the outcome of everything centrally, you are designing a web framework (like Symfony), not a web platform like Drupal.

My (effulgentia's) interpretation of the problem:

  • Much of the Symfony related integration done for D8 was to enable caching of partial html responses (blocks and _content controllers).
  • To do that effectively, blocks and _content responses need to identify all of the request data that they depend on. For example, do they vary by user role, by user, by client IP address, etc. The implementation details of how these dependencies get identified is still being worked out, but are mostly irrelevant to the higher level discussion needed in this issue.
  • However, in Drupal, we have hooks. For example, hook_node_view(). These hooks don't know which blocks and _content controllers end up (indirectly) invoking them. For example, there can be many different blocks and _content controllers that render nodes. Neither node.module nor any particular implementation of hook_node_view() knows what all of them are.
  • If one of these hook implementations ends up calling Drupal::request() to get information from the request that wasn't listed as an explicit dependency of the block or _controller, and uses that information to affect what gets displayed, then the cache will get poisoned: a following request might serve the cached version despite it containing output not suitable to the new request. One example of this is #914382: Contextual links incompatible with render cache, which found contextual links poisoning the forum block cache. Another example is comment_node_view() adds a link for how many comments are new to the user (but only if history.module is enabled). We can likely find more examples in core, and many more examples in contrib. #1605290: Enable entity render caching with cache tag support is partially addressing the issue for entities, but there are likely many non-entity hooks to consider as well.

Options

  1. Put a warning on Drupal::request() (that's already in HEAD), fix all core use cases where cache poisoning occurs (like the contextual links and comment history examples), and leave it up to contrib developers to figure out their own cache poisoning issues themselves.
  2. Make cache poisoning impossible by cleansing the $request that represents the subrequest to only that information that is listed as a dependency of that block / _content controller. That way, when a hook calls Drupal::request(), it only has access to the subset of request information on which cache can vary. For example, if the block / _content didn't list ip address as a dependency, then for any hooks that run within that subrequest context, calling Drupal::request()->getClientIp() will return NULL. This necessitates giving modules a way to alter the dependency list of subrequests, but just that much is easy. However, it puts the burden on module developers to figure out which blocks and _content routes need to be altered.
  3. Whether or not we do the $request cleansing, we can also try to come up with a context mediator between the module implementing a hook and the blocks / _content controllers that indirectly invoke that hook. For example, a module implementing hook_node_view() with per-user information could somehow identify that there's a per-user dependency for any block / _content subrequest that depends on a $node, and then let the mediator be responsible for altering the routes accordingly. We probably need a bit more analysis on whether there are use cases without as clear a relationship between hook and route parameter, and how to mediate those.

Discuss!

Comments

catch’s picture

I think we need to do #1 whatever happens, then at least core is setting a good example.

#2 is tricky - some code might prevent caching based on that information or just use it for logging or similar, in which case it's not poisoning the cache and we're taking away information that might have been useful/harmless. Also if we do that I can see contrib/custom code just using raw superglobals anyway.

Also this issue isn't specific to hooks - any block that declares a particular set of dependencies but then calls out to other code, is going to run into this problem. If you have a field formatter plugin that can just as easily be accessing something in request as hook_entity_view_alter() might. With the code execution path depending on configuration it's impossible for a particular block plugin to be able to declare this stuff up front, even if everything down the rendering pipeline declares it as well there'd need to be a collection mechanism on cache misses which then figures out the cache key components for subsequent requests or similar.

Crell’s picture

More draconian approach, which I'm not necessarily advocating: throw an E_USER_DEPRECATED error inside Drupal::request(). Normally you'd turn that off so you don't see it, but any module that is still accessing the request from a place it shouldn't (vis, that it's not been passed to it) will yell at you/the developer until it's fixed.

(As I said, not necessarily advocating that option, just pointing out that we have it.)

dlu’s picture

Component: base system » cache system

Moved to cache system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

dlu’s picture

Issue summary: View changes

Formatting

catch’s picture

Issue summary: View changes
Priority: Major » Normal
Status: Active » Postponed

#1 is done. Anything else depends on blocks getting context injected.

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.

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.

Wim Leers’s picture

Status: Postponed » Closed (outdated)