Problem/Motivation

The Big Picture of what we've already done

In order for Drupal 8 sites to have great performance, we've done a huge amount of work. A big part of that is making Drupal 8 the first release to no longer break render caching all over the place.
E.g. by no longer doing highly personalized things (such as quick edit links, "new" and "updated" comment markers …) or highly dynamic things (adding a class to the "active" links) without thinking about consequences. Those things have been replaced either with #post_render_cache placeholders/callbacks or client-side rendering.

We've already done this work for all of the obvious violators. So, we're mostly there. But we're not yet 100% there. We're ~95% there in terms of cache tags. But we're nowhere near 100% when it comes to cache contexts.

Cache Contexts: current status & potential

So far only applied cache contexts to 2 things: blocks (see BlockBase) and access results (see AccessResult).

If we'd bring the usage of cache contexts to 100%, then we could truly leverage all that performance work… and bring a massive leap in performance. Because we would have complete knowledge about the contexts by which every rendered thing is varied by, which combined with bubbling of cache contexts, would allow us to reliably cache ever larger pieces of the page, hence needing to invoke ever fewer objects to build ever fewer render arrays, to do ever fewer rendering… to also initialize ever fewer services.

Why?

It is precisely that metadata that was missing in prior versions of Drupal… which required us to generate pretty much everything on the entire page for every single request. But in Drupal 8, we have a Holy Cache Metadata Trinity:

  1. Cache keys identify a thing. E.g. ['node', 5]
  2. Cache contexts identify the different variations (representations) of that thing. (The contexts it depends upon.) E.g. ['cache_context.user'].
  3. Cache tags identify the reasons for (a variation of) a thing to be invalidated. (The other things it depends upon.) E.g. ['node:5', 'taxonomy_term:3']

Together, they give us exactly what we need to cache the majority of a page, instead of the minority (like in the past).

Finally: this allows us to add mindblowing alternative render pipelines, which e.g. automatically convert pieces of content that are varied by a "high frequency cache context" (like the "per user" cache context — there usually are many users on a site) into placeholders. That would mean we can still cache the entire page's HTML, but just have some placeholders in there for the highly personalized (high frequency cache contexts) things, which would be the only things that'd still need to be calculated. Those placeholders could even be rendered BigPipe-style.

For more info about the big performance improvements that this allows, see:

Proposed resolution

We need it to be applied to everything that can have different representations:

  1. field formatters (output may vary depending on the permissions of the user)
  2. text format filters (output may vary depending on e.g. language)

Remaining tasks

Must-haves (in order of importance, most important first)
All done!
Should-haves that we should be able to do after 8.0, because all blockers are critical
Nice-to-haves that we may not be able to do after 8.0 (in order of importance, most important first)
Can happen after 8.0, because no API changes:

Done (in chronological commit order, first listed was committed first):

  1. #2318437: Replace the hardcoded langcode key on blocks with the 'language' cache context
  2. #2329101: CacheableInterface only has a getCacheKeys() method, no getCacheContexts(), leads to awkward implementations
  3. #2429261: Replace the hardcoded cache key on the book navigation block with a 'book navigation' cache context
  4. #2428563: Introduce parameter-dependent cache contexts
  5. #2430341: Comment pager-dependent cache key should be a cache context
  6. #2429257: Bubble cache contexts
  7. #2396333: BlockContentBlock ignores cache contexts required by the block_content entity
  8. #2445761: Add a X-Drupal-Cache-Contexts header to aid in debugging and testing
  9. #2433599: Ensure every (non-views) pager automatically associates a matching cache context
  10. #2445743: Allow views base tables and entity types to define additional cache contexts
  11. #2443073: Add #cache[max-age] to disable caching and bubble the max-age
  12. #2453891: Renderer::getCacheableRenderArray() does not include max-age
  13. #2444211: Document cacheability of render arrays, and the considerations to use when generating render arrays
  14. #2432837: Make cache contexts hierarchical (e.g. 'user' is more specific than 'user.roles')
  15. #2448823: Add languages:<type> cache contexts
  16. #2458413: BlockViewBuilder should specify cache contexts even for uncacheable blocks
  17. #2458993: #cache[expire] is undocumented, unused, untested: remove it, use #cache[max-age] instead
  18. #2428805: Remove the ability to configure a block's cache contexts
  19. #2459003: #cache[cid] breaks bubbling
  20. #2428703: Add a 'user.permissions' cache context (was: "Should cache contexts be able to associate a cache tag?")
  21. #2451679: Validate cache contexts (+ cache contexts in some views plugins wrong)
  22. #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE
  23. #2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()")
  24. #2458349: Route's access result's cacheability not applied to the response's cacheability
  25. #2460013: Uninstalling modules containing cache contexts or #post_render_cache callbacks may break Drupal if they are cached somewhere
  26. #2462851: Improve Views entity row renderer plugins' cache contexts
  27. #2452317: Let views result cache use cache contexts
  28. #2459819: Remove CacheableInterface (and no longer let block plugins implement it)
  29. #2464877: Update RendererInterface::addDependency() to accept *any* object, not only CachableDependencyInterface objects
  30. #2099137: Entity/field access and node grants not taken into account with core cache contexts
  31. #2469277: Changing #cache keys during #pre_render or anywhere else leads to cache redirect corruption
  32. #2468151: Rename the CacheContexts service to CacheContextsManager
  33. #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers
  34. #2466585: Decouple cache implementation from the renderer and expose as renderCache service
  35. #2335661: Outbound path & route processors must specify cacheability metadata
  36. #2489966: The Views table style plugin does not specify cache contexts for click sorting
  37. #2433591: Views using pagers should specify a cache context
  38. #2487099: Set cache contexts for exposed sorts / items_per_page / offset.
  39. #2493091: Installing block module should invalidate the 'rendered' cache tag
  40. #2493047: Cache redirects should be stored in the same cache bin
  41. #2470715: cacheGet-case: #post_render_callback's that result from other #post_render_calback are not processed
  42. #2483781: Move cache contexts classes from \Drupal\Core\Cache to \Drupal\Core\Cache\Context
  43. #2407195: Move attachment processing to services and per-type response subclasses
  44. #2500443: Cache API topic says nothing about cache context, add something
  45. #2511472: Refactor all usages of drupal_render()/Renderer::render() that break #2450993
  46. #2449459: [PP-1] Make URLs cache context aware
  47. #2487600: #access should support AccessResultInterface objects or better has to always use it
  48. #2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age
  49. #2495171: Block access results' cacheability metadata is not applied to the render arrays
  50. #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed
  51. #2443323: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock
  52. #2450993: Rendered Cache Metadata created during the main controller request gets lost
  53. #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity
  54. #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified
  55. #2349679: Support registration of global context
  56. #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching.
  57. #2351015: URL generation does not bubble cache contexts
  58. #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case
  59. #2524082: Config overrides should provide cacheability metadata
  60. #2430397: When mapping cache contexts to cache keys, include the cache context ID for easier debugging
  61. #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable
  62. #2559011: Ensure form tokens are marked max-age=0
  63. #2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at
  64. #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!)
  65. #2458763: Remove the ability to configure a block's cache max-age
  66. #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface
  67. #2483887: Mark RenderCache as internal

Comments

Wim Leers’s picture

Issue summary: View changes
larowlan’s picture

Great write up Wim. For what its worth, as someone who works with D7 in the D8 and tinkers on D8 in my spare time - I'm finding D8 feels faster already - keep up the great work.

Wim Leers’s picture

#2: Thanks, that's great to hear :) Though you said it exactly right: Drupal 8 feels faster. It's actually slower. But thanks to the front-end performance improvements (for auth users: JS at the bottom, some client-side rendering — much more for anon users), it actually feels faster.

(Of course, when comparing to more complex, "built out" Drupal 7 sites, Drupal 8 may very well be faster already.)

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Fabianx’s picture

I think we want the concept of 'global' cache contexts.

Motivation:

One of the biggest use cases in Drupal 7 for varying the page cache was 'device detection' based caching, e.g. on a cookie set by varnish.

While Varnish could well change the path to add a request parameter, such differing by path, the cache_context of a 'mobile version' is way more granular - as it only differs on the contexts of the cookie.

e.g. device = desktop | mobile | tablet

However usually this should apply globally as any code e.g. in a hook_preprocess_foo() {

}

could be changing this for mobile purposes.

While sometimes returning the cache context explicitly and bubbling it up is fine, having the possibility to define it globally is important, too.

This can also be used by sites knowing their cache profile to optimize certain cases.

Other possible global cache contexts that usually we want to always apply (for display and security reasons) are: theme, role

But some sites might decided to vary everything on 'language', too, e.g.
(@todo maybe create an issue for adding cache contexts globally)

On the other hand there might be sites that chose to remove cache contexts globally, e.g. that never want to differ on the users configured TZ.

We should think about allowing something for that _maybe_, though adding things to a global list of always applied ones, is more important IMHO.
(@todo maybe create an issue for removing cache contexts globally)

The global definition would ideally live in the container, so its:

a) fairly static
b) very reliable and usable at page middle ware level pre-boot and pre-routing.

In other words, an advanced page cache sitting at the middle ware level as explained in the doc, would probably make heavy use of the cache context hierarchy (issue needed), to propagate the mix of:

[ menu_trail:tools, book.navigation, users.roles ] to e.g. just [ 'page', 'users.roles' ] and only take the most upper level cache contexts into account that can be executed with very low overhead.

The cookie context described would fall into the same category as 'cookie:device' is an operation that is ideally would just do something like return 'cookie.' . XSS::checkPlain($_COOKIE[$name])); (@todo Make an issue for a generic cookie cache context service? might be useful for such cases?)

Having fairly simple cache contexts is also important for any ESI / AJAX implementation that needs to pass those (authorized) contexts via cookie or HTTP headers to keep those simple.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Oops.

Wim Leers’s picture

Issue summary: View changes
Fabianx’s picture

Issue summary: View changes
Wim Leers’s picture

Wim Leers’s picture

larowlan’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

YAY! The most important issue landed: #2429257: Bubble cache contexts.

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

Undoing #26.

Wim Leers’s picture

Fabianx’s picture

Issue summary: View changes

Added #2449459: [PP-1] Make URLs cache context aware to the IS, needs discussion, especially a render scope could make many things simpler.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

webchick’s picture

Issue tags: +Triaged D8 critical

The core committers discussed this and since this meets the definition of a performance critical it is in fact critical. Tagging.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Making it clear that the Done list is in commit order.

Wim Leers’s picture

Issue summary: View changes

Broke the HTML, oops.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

YesCT’s picture

Wim Leers’s picture

Fabianx’s picture

webchick’s picture

Issue tags: +D8 Accelerate Dev Days

Tentatively tagging for the Performance sprint at Dev Days coming up.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

yannisc’s picture

Issue summary: View changes

removing duplicate word.

Wim Leers’s picture

Wim Leers’s picture

Fabianx’s picture

Fabianx’s picture

Fabianx’s picture

Wim Leers’s picture

Wim Leers’s picture

plach’s picture

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

Two more issues got done.

jhodgdon’s picture

On #606840-133: Enable internal page cache by default I brought up the issue of timezones, and I was redirected to this meta issue.

I was wondering about date/time rendering using normal date/time formats (as opposed to "time ago" formats, which are a separate problem). This rendering is always timezone dependent -- it can depend on the setting for the overall site timezone, or if the setting that says "let the user choose time zone" is active, then it's dependent on the user's own time zone setting.

Are we capturing this adequately? Wim Leers mentioned on the other issue that there is a timezone cache context to handle this. But I didn't know about it. So:

a) It seems like we should add API documentation to all of our date format functions/methods mentioning that if you are using this output in a render array (which presumably you would be), you need to put a time zone cache context on that render array element.

b) If it hasn't been done already, we need to make sure that all Core uses of date formatting do this, such as the Views date/time field handlers and Field API date/time field formatters... and then any other calls making render arrays out of Field API and Views, if we still have some and I think we do.

c) Regarding the time zone cache context... hopefully that is being handled properly, depending on the time zone and user time zone settings? I would hope/assume so.

catch’s picture

a) I'm not sure this is viable everywhere - we have/need documentation of the different cache contexts generally, but I feel like that belongs more in the render API documentation (i.e. 'when building a render array take care of cache contexts, for example timezone' as opposed to the individual date formatting functions.

b) has been done afaik, for example https://api.drupal.org/api/drupal/core%21modules%21datetime%21src%21Plug...

And c) we use date_default_timezone_get() which respects the user's timezone (because we set it early-on in the request). So yes this works fine.

Wim Leers’s picture

c) Yes most definitely.

Also see the general Cache Contexts docs: https://www.drupal.org/developing/api/8/cache/contexts.

jhodgdon’s picture

Wow, nice page there on Cache Contexts! There's not a mention of them on the api.d.o topic on the Cache API -- do you think we should add something short (and point to that page for more info)?
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...

We do have a quick (appropriate) mention of cache tags, keys, context, and age on the Render API topic page, so that's good:
https://api.drupal.org/api/drupal/core!modules!system!theme.api.php/grou...
And that leads to
https://www.drupal.org/developing/api/8/render/arrays/cacheability
for more information. That page specifically mentions cached contexts and specifically the timezone context. Not only that, it links to the Cache Contexts page mentioned in #80 for details, so that is perfect.

And agreed that my suggestion of adding docs to the date format functions/classes is too much; it does belong in the render/cache docs instead.

So... Good! Given all of this, I think all we should do is add a short section to the Cache topic page on api.d.o about cache context, linking to https://www.drupal.org/developing/api/8/cache/contexts for more information, and we'll be good. So I filed
#2500443: Cache API topic says nothing about cache context, add something
for that. I'll assign it to myself for now, especially if Wim agrees to review it (or vice versa -- Wim feel free to take it on if you have time, as I might not for a week or so).

Thanks for thinking this through, and Wim I think you wrote most of those docs (with some help) -- great job, everyone who worked on them!

Wim Leers’s picture

RE: link to that page from the Cache API api.d.o page — yes!

RE: second paragraph: glad you approve :)

RE: third paragraph: great.

RE: fourth paragraph: excellent :) And thanks! :) They will continue to be improved in the weeks and months to come, as core fleshes this out further, and fixes the remaining problems (which are minor in the grand scheme of things at this point — fortunately).

dawehner’s picture

One thing which would be great to do here is to identify which subissues could get critical at some point.
Those subissues are issues which makes things impossible to fix in 8.1.x or contrib because they would involved necessary API changes.

webchick’s picture

Category: Task » Plan
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue summary: View changes

I'm going through everything in this issue, to make it much, much more actionable again. Here's a first bit.

Wim Leers’s picture

Issue summary: View changes

Now we're getting somewhere: a high-level categorization of the child issues, with the most actionable ones listed at the top, and the ones that are almost done listed second.

Not yet finished.

Wim Leers’s picture

Issue summary: View changes

And another bunch of child issues have been updated.

Still not yet finished.

Wim Leers’s picture

Issue summary: View changes
Berdir’s picture

My main worry about this issue is that it's not clear which sub-issues are critical and which aren't. The new structure is certainly an improvement and helps, but it's not quite enough yet I think.. I think we agree that not everything here is release-blocking, but I'm not sure what...

IMHO, sooner or later, we need to do what we did with many other critical meta issues already.. mark the actual blocking issues as critical and downgrade this to major to have an overview. Will bump the critical count again a bit, but that's also more honest, and on the plus side, will make progress here more visible in that number ;)

Sounds like a good task for the upcoming critical sprint :)

dawehner’s picture

Well, critical has to be, what can't be done between 8.0 and 8.1

From my humble point of view, just #2487600: #access should support AccessResultInterface objects or better has to always use it is really a big API change
The following bits like #2407195: Move attachment processing to services and per-type response subclasses #2443323: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock and #2463581: #cache_redirect cache items should have an 'expire' timestamp that matches the merged max-age are more like a nice to have

@wim
It would be indeed nice if you could categorize them rather in what you think needs to be done and what is optional ...

Wim Leers’s picture

Issue summary: View changes

#89/#90: Yep, I was working towards exactly that. Please give me some time to work through it all :)


Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

(Also: #2443323: New convention: CacheContextInterface implementations should mention their ID in their class-level docblock is RTBC and the #2407195: Move attachment processing to services and per-type response subclasses must-have is blocked on review.)

dawehner’s picture

In which sense are big pipe and smart cache a must have?

catch’s picture

Right I'd probably divide things up like this if it's useful:

Smartcache/BigPipe blockers that must happen before 8.0.0 (->critical)
Smartcache/BigPipe blockers that can happen in minor releases (->major + minor version target - we can still try to get these in before 8.0.0 but wouldn't block on them)
Smartcache/BigPipe blockers that can happen in patch releases (-> major)

Nice to have - must happen before 8.0.0 or move to 9.x
Nice to have - can happen in patch or minor releases.

Wim Leers’s picture

In the sense that they hugely improve authenticated user performance. catch told me a few days ago that he wants to ship with SmartCache.

BigPipe itself is not necessarily a must-have (despite it also greatly improving authenticated user performance), but making sure no further API changes are necessary AFAICT means that we have to consider it a must-have for now, to get some of the blockers done, so we can have a high level of confidence that we can add BigPipe in 8.1.0 without API changes.

dawehner’s picture

Yeah sure, I don't question that at all, but I 'd like to see us focusing on what is needed in order to release Drupal 8.0.x, not what would be also nice in terms of authenticated user performance.

catch’s picture

I'd like to get SmartCache into 8.0.0 if we can - even if it's close to release candidate. i.e. I wouldn't want to bump it to 8.1.x until the very last minute.

However as long as we're confident we're able to enable it in a minor release (i.e. no critical blockers and major blockers well documented/fleshed out), then I don't consider it critical in itself.

What I absolutely want to avoid is that we release 8.0.0 with an issue like #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed not properly documented/scoped and get a nasty surprise later.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

Discussed #2480077 with catch, we found two ways to fix it (one very suboptimal, one close to optimal): #2480077-12: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags. Neither requires an API change. Hence, moving it to the "post-8.0" category.

Wim Leers’s picture

Issue summary: View changes

Removed a duplicate from the must-haves.

Wim Leers’s picture

Issue summary: View changes

In #102, I said I was working on #2450993: Rendered Cache Metadata created during the main controller request gets lost to better assess the criticalness of that and related issues.

So, by now, I've got good news to report:

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

I investigated every single remaining test failure on the SmartCache issue and made sure there's a child issue to fix all of them: #2429617-138: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). Now postponed on 8 issues, but all individually critical issues are now marked as such. Therefore, moving SmartCache to a new "should-have, but should be able to happen post-8.0" category.

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

To be able to do #2458763: Remove the ability to configure a block's cache max-age, we need to get #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface done first. Been pushing that one forward, and almost there.

Added both to the "nice-to-have" section.

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

berdir did the analysis work necessary to be able to know for certain that #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons is a should-have, not a must-have. Hurray!

effulgentsia’s picture

Looks like #2512718: EntityManager::getTranslationFromContext() should add the content language cache context to the entity is in there twice now.

Other than that, looks like #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts is the only "must have" that isn't itself critical, which means the only "meta" part of this issue that remains critical is to determine if there are any critical blockers to that and make sure those are promoted to critical. Am I understanding that correctly?

Wim Leers’s picture

Fabianx’s picture

Finally added #2525910: Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case to the list.

I think we should check / assert whether this could lead to security problems ASAP.

If we decide its the callers responsibility, that is fine, but we should at least take a look ...

xjm’s picture

Issue tags: +D8 Accelerate London
Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

Other than that, looks like #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts is the only "must have" that isn't itself critical, which means the only "meta" part of this issue that remains critical is to determine if there are any critical blockers to that and make sure those are promoted to critical. Am I understanding that correctly?

Yes :)

So I triaged the blockers for BigPipe together with @Fabianx. To get BigPipe in, there are 3 issues we need to fix:

  1. blocker: auto-placeholdering (#2499157: [meta] Auto-placeholdering)
  2. blocker: render strategies (#2349011: Support placeholder render strategies so contrib can support BigPipe, ESI…)
  3. BigPipe itself (#2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts)

The conclusions:

  1. Confirmed that the first is not a critical by very explicitly listing the remaining tasks and ensuring that they're not API changes: #2499157-2: [meta] Auto-placeholdering.
  2. Confirmed that the second is not critical by very detailedly analyzing the already-existing patch and ensuring that it A) does not make API changes, B) as it is worked on to be committable also will not require API changes: #2349011-14: Support placeholder render strategies so contrib can support BigPipe, ESI….
  3. The third is by definition not a critical because it's a pure feature addition that the APIs should allow to be added at any time with zero disruption.

Therefore, I moved the BigPipe issue from "must-have" to "should-have".

Wim Leers’s picture

Priority: Critical » Major

Given #121 and #122, this can now finally be demoted to major!

Fabianx’s picture

After thinking about #122 opened:

I don't think they need API changes (maybe API addition for forms), but are important should-have's to reduce the unknown-unknown's.

effulgentsia’s picture

If there's agreement on #124, let's get those into the right section of the issue summary.

Wim Leers’s picture

Issue summary: View changes

Moved #124 into IS as per #125.

Wim Leers’s picture

Issue summary: View changes

#2349679: Support registration of global context was fixed as part of another issue.

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

I'd forgotten to add #2524082: Config overrides should provide cacheability metadata previously. Now listed in the must-haves.

In better news: #2351015: URL generation does not bubble cache contexts was committed yesterday! :)

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes

#2524082: Config overrides should provide cacheability metadata landed late yesterday night. Which means all must-haves (criticals) have now been finished! Yay :)

Tweeted the happy news: https://twitter.com/wimleers/status/624680987734224896

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
Fabianx’s picture

Found a major bug:

* EntityViewBuilder::viewMultiple() is _not_ compatible with render caching.
* This only works, because we use views to render entities and views uses EntityViewBuilder::view() instead of viewMultiple(). (which is pretty surprising, too).

This bug is present since the original patch to add render caching to entities :D.

The reason is that the following array is returned for multiple entities:

[
  '#pre_render' => [EVB, 'buildMultiple'],
  '31 '=> [...],
  '32' => [...].
];

but the 31, etc keys contain the #cache keys entries ...

This is pretty surprising, because:

Overall we go to great lengths to ensure that buildMultiple() works efficiently with all the view modes, etc. and nothing in core uses it ...

Berdir’s picture

Weird.

IIRC, we pretty much gave up on a performant view multiple when we switched to using pre_render for the field formatters. I remember that that was discussed back then.

Wim Leers’s picture

Nice find! We can easily fix this during the RC phase fortunately :)

yched’s picture

Weird indeed.

viewMultiple() is important because it runs formatter's prepareView() on all the entities at once, making it possible to multiple-load referenced entities for ER fields.
Originally it was used by the various "hardcoded listing pages" like /node, taxonomy/[tid], but most of those progressively moved to Views. It is however very unfortunate that Views doesn't use viewMultiple(). Apparently this was changed in #2099131: Use #pre_render pattern for entity render caching.

IIRC, we pretty much gave up on a performant view multiple when we switched to using pre_render for the field formatters. I remember that that was discussed back then

Hmm - can you pretend I'm a total noob and axpand on that a bit ? ;-) why does pre_render kill performant viewMultiple() ?

yched’s picture

Hm so yeah, this slowly comes back to me. We basically gave up on Views leveraging multiple-entity for formatter's prepareView() in #2099131: Use #pre_render pattern for entity render caching, with prepareView() now running as part of the #pre_render of each entity render array, instead of as part of the buidling of the array for all entities previously.

See #169 and #177 over there - in short, answer by @Wim back then :

Once we get to the point where Views bubbles up cache tags correctly, we'll be able to render cache the entire View output. So this [building formatters's prepareView() entity by entity] will become moot.

Not sure whether we have reached that point ? (Views bubbling up cache tags correctly) ;-)

So in the end, formatters (notably ER formatters) go through the non-trivial FormatterInterface::prepareView() API and logic to optimize for multiple entities, but our main listing builder, Views, ditches that anyway, because it runs entity-by-entity and render-caches on a higher level. This kind of questions the shape of the Formatter API and the interest of the convoluted prepareView() construct now :-/
Also, the building time on cold caches or cache misses is much much worse, since ER fields load referenced entities one by one. But yeah, that's kind of the deal with building formatters at #pre_render time.

However, there are still non-Viewified pages that use EVB::buildMultiple() in core atm (and it's still part of the API), so it seems we need to fix it if something's wrong...

Wim Leers’s picture

Not sure whether we have reached that point ? (Views bubbling up cache tags correctly) ;-)

We have, some months ago :)

This is all very unfortunate obviously. Can we open a dedicated issue for this, instead of discussing it on a meta issue? :)

ivanjaros’s picture

I have a formatter where I'm displaying multiple entities and I too am using view() with foreach loop instead of viewMultiple() because I am displaying the entities as item list so I expect to receive a simple array with processed entities without any pre/process handlers.

Wim Leers’s picture

Issue summary: View changes

The following issues from the IS have landed:

Beyond that, many more relevant issues have landed in the past several weeks that I've lost track of. We're in a good place :)

mfb’s picture

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.