BEFORE COMMENTING ON THIS ISSUE, PLEASE READ THE ISSUE SUMMARY OF #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts!
A requirement for #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts is to have placeholders. This is a child issue of that one to bring the single most crucial concept for BigPipe to Drupal 8 core — quoting myself at #2469431-40: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts:
I think it's crucial that we first flesh out the "placeholders" part of this patch. If we agree that we that we can remove
#post_render_cachein favor of the conceptually simpler "placeholders", then I think we could file an issue for doing just that and mark it as a blocker for this issue, but it would be a DX improvement regardless.
Fabianx & yched +1'd that.
#post_render_cache enough?#post_render_cache.#post_render_cache, which has a very painful DX, B) and for those that do use #post_render_cache, they will always use placeholders, not only when it is necessary (i.e. when the contents are indeed very dynamic/uncacheable).#post_render_cache are now misguided and adversely impact performance. Key example: entity_embed. This is a D8 contrib module that will very likely see enormous, highly prominent use in the Drupal 8 ecosystem. It uses #post_render_cache to be able to do entity access checking, because entity access checks could be highly dynamic — e.g. per user — and #post_render_cache was the only solution for that until recently. But, recently we fixed #2099137: Entity/field access and node grants not taken into account with core cache contexts, which is an analogous problem to the one that entity_embed faces. It was possible to fix that thanks to #2429257: Bubble cache contexts. And so, thanks to #2429257, an entire class of problems that D8 was only able to fix using #post_render_cache can now be fixed without it! Consequently, entity_embed will no longer have to render each embedded entity on every page load, which is an enormous performance improvement. So: all current #post_render_cache callbacks in D8 contrib MUST be re-evaluated. (The issue for Entity Embed: #2496363: Replace #post_render_cache with #lazy_builder and use cache context bubbling.)#lazy_builder callbacks.
#post_render_cache callbacks do today, and with actual documentable arguments instead of a single $context argument for the callback.
$build, do its own rendering of $build, generate placeholder markup, find the placeholder markup in $element['#markup'], bubble metadata from $build onto $element" misery — instead simply: "build $build and return it.#post_render_cache callback logic can be retained, you only have to delete code, and make the function signature sane :) See the change record.#create_placeholder
#create_placeholder = TRUE, and if a #lazy_builder is present, then it behaves like #post_render_cache does today.
#create_placeholder to TRUE.
#pre_render callbacks to #lazy_builder callbacks, and the system will be able to optimize it automatically (i.e. choose when to lazily build this!).#pre_render.#pre_render callbacks. But #pre_render callbacks don't expect to work in isolation; they are designed to "decorate" an existing render array. So, we initially called it #pre_render_cache, what we now call #lazy_builder. Rationale being: "it's a #pre_render callback that can run in isolation, much like #post_render_cache, which meant we could just map any #pre_render_cache callback to become a #post_render_cache callback". But after many discussions and iterations, we ended up with #lazy_builder, because that term best embodies 1) the fact that this builds a render array rather than decorates it (#pre_render), or merely runs at a certain stage (#post_render_cache), 2 the laziness (only build the render array (sub)tree when necessary, i.e. during regular rendering or afterwards).Renderer, when it encounters an element with #create_placeholder === TRUE && isset(#lazy_builder), it replaces that element in its entirety with placeholder markup, plus bubbleable metadata that informs Drupal about the placeholder markup that it should replace. So, conceptually:$element = [
'#lazy_builder' => ['Class::renderCommentForm', ['arg1', 'arg2'],
'#create_placeholder' => TRUE,
];
to:
$element = [
'#markup' => '<drupal-render-placeholder STUFF></drupal-render-placeholder>',
'#attached' => [
'placeholders' => [
'<drupal-render-placeholder STUFF></drupal-render-placeholder>' => [
'#lazy_builder' => ['Class::renderCommentForm', ['arg1', 'arg2'],
],
],
],
];
See the new general recommendation.
None.
None.
#lazy_builder callbacks, which are given only arguments that may contain only primitive types (string, int, bool, float, NULL), not objects.#create_placeholder, which allows a render array to be forced to be placeholdered.max-age = 0 or high-cardinality cache contexts are present for the subtree, set #create_placeholder = TRUE.)#post_render_cache (the above is its successor).RendererInterface::generateCachePlaceholder() and drupal_render_cache_generate_placeholder() because they are now obsolete.
- A
#lazy_buildercan build a render array from scratch. It is given only the arguments that you provide it, and that context may contain only primitive types (string, bool, int, float, NULL).- When specifying
#cache, provide a#lazy_builderto build out the entire render array in case of a cache miss.- Together, the two above points allow Drupal to optimize: it is able to determine based on how dynamic the contents of the
#buildercallback are, whether the#lazy_builder(and thus the associated render array) should be rendered during the initial page load, or via BigPipe or ESI.
| Issue category | Task because not technically a bug, but definitely a blocker to having BigPipe in Drupal 8 core. |
|---|---|
| Issue priority | Major because blocks BigPipe. |
| Prioritized changes | The main goal of this issue is performance, as well as significantly improving DX. |
| Disruption | Disruptive for core/contributed and custom modules/themes because it will require a BC break: #post_render cache is removed. In favor of #lazy_builder (which is easy to transition to, thanks to similar semantics).This is a hard BC break, but for excellent reasons: way easier to understand, BigPipe support, automatic placeholdering. Developers won't have to learn about #post_render_cache complexities, they are simply encouraged to use #lazy_builder instead of #pre_render, and Drupal 8 will automatically determine the best time to render the render array returned by the #lazy_builder callback.Note that the most important D8 contrib users ( entity_embed, masquerade) of #post_render_cache is already switching away from it, now that we have cache context bubbling. See #2496363: Replace #post_render_cache with #lazy_builder and use cache context bubbling + #2448699: Implement caching for the masquerade block and links with a 'masquerade' cache context.
|
| Comment | File | Size | Author |
|---|---|---|---|
| #97 | interdiff.txt | 13.2 KB | wim leers |
| #97 | placeholders-2478483-97.patch | 159.87 KB | wim leers |
| #88 | interdiff.txt | 979 bytes | wim leers |
| #88 | placeholders-2478483-88.patch | 158.79 KB | wim leers |
| #87 | interdiff.txt | 5.79 KB | wim leers |
Comments
Comment #1
wim leersComment #2
wim leersWorking on this.
Comment #3
fabianx commentedWe decided that I will roll an initial MVP patch for this and that we use a symmetric system, where the placeholdering is still set explicitly right now.
Comment #4
wim leersDiscussed with Fabianx; I understand what needs to happen, but Fabianx will take this on, as he knows much better what needs to happen, rather than merely understanding it :)
Comment #5
wim leersLOL @ d.o
I unassigned myself, and d.o didn't even tell me that it was assigned to somebody else in the mean time!
Comment #6
wim leers#2478443: Set the 'is-active' class for anonymous users in a Response Filter instead of a #post_render_cache callback just landed, which means the path to do this is wide open!
Comment #7
fabianx commentedHere we go with a first patch, but first a more patch friendly title.
Comment #8
fabianx commentedOkay, first patch:
- The minimum placeholders support (--placeholders-only.patch)
- Converted all #post_render_cache usages in core to placeholders, including a hack to make BubbleableMetadata create a placeholder instead of a thing to show that it is equivalent and conversions are simple. (--conversions.txt)
As can be seen the DX improves in all cases and as long as you have a render array that is cacheable (e.g. no objects, etc.) you can easily create a placeholder by just doing:
even manually.
You can also use the old drupal_generate_cache_placeholder() if you want to as long as you combine it with #pre_render_cache.
Note:
#pre_render_cache itself is a concept that works as follows:
- #pre_render_cache is called only when #pre_render is not set. (we could change that however and have #pre_render callbacks check for that itself).
The reason is to support the following use-case:
- If you already have an entity and done the trouble of loading it, it would be better to stick it in directly, so you don't have to do the loading twice on a cold-cache.
Though entities are likely a bad example as those are cached statically per request anyway ...
So the idea was that you could have:
and #pre_render_cache can be used to re-create #pre_render, #property, etc. but that #cache and #pre_render_cache are stored together in the render cache array.
Comment #9
fabianx commentedSelf-Review with comments:
This could use a normal (and faster) array_merge, because placeholders are unique as they are hash based.
I am still pondering whether #cache placeholders or #render_placeholders.
Pro #render_placeholders:
In this patch placeholders do not have much to do with caches or the cache system and are explicitly set with #cache_placeholder => TRUE and #pre_render_cache.
Contra #render_placeholders:
If we want to support non-explicit placeholdering, we need to support bubbling cases, which means we need to store the information that we placeholder somewhere, which is stored in the render cache layer.
Also as not everything can be dynamically re-created, but could still be cached, the render cache system needs to be placeholder-aware.
This shows how a previous #post_render_cache can be converted to placeholders, if it uses the generateCachePlaceholder pattern ...
#render_strategies are not needed here and only here as I use this patch on top of the BigPipe one for testing purposes, too.
This is a good example to show that a conversion removes code and improves DX.
In most cases using a subkey of $element and returning that is enough to convert a #post_render_cache callback to #pre_render_cache + #cache_placeholder.
Its also pretty similar to how #pre_render itself works, just that the context is not in $elements, but in $context.
It is important to provide render strategies with means to render a placeholder without it being placeholder'ed again.
As the logic to check $elements['#cache_placeholder'] == TRUE/FALSE got very complicated I used the cache_placeholder / cache_no_placeholder pattern instead.
This is less evident in this patch, but more in the all-in-logic of the bigpipe patch.
Basically:
For every element need to specify that a placeholder should never be created - regardless if #cache_placeholder is TRUE or FALSE.
It might be possible to just use one property though, where it being empty is 'auto-detect'.
It is important to only create a placeholder if #pre_render_cache and #cache_placeholder are set.
This allows the 'auto-detection' follow-up.
Here also the check for #cache_no_placeholder is used.
This is where #pre_render_cache is called if #pre_render is not yet set, so the #pre_render_cache can populate #pre_render.
This was mainly to ease conversion of the BlockViewBuilder.
It is likely not needed in general, lets discuss it.
By contract cacheSet can change $elements and the metadata on $elements, the same is true for later processPlaceholders calls (though those need to live in a loop similar to post render cache).
So for cacheSet to be able to do whatever it wants, the stack is emptied (as all data is in $elements) and a new empty stack frame is pushed upon it.
Then after the processing the stack is updated again.
The reason is that updateStack does two things:
1) It updates elements with the stack data.
2) It updates the stack with the elements data.
So the data is redundantly stored in both places, but if cacheSet was to change assets for whatever reason then that data would be out of sync.
This change prevents this.
Probably should set an attribute in the $render_cache_array so that placeholders can be easily identified.
Might need to use a hash_hmac so that no-one can 'guess' a placeholder. (Not sure what the security implications of that are, but was asked for the messages part.)
Uses the same markup for now, this is later replaced by the rendering strategies with their ESI tags, [divs], etc.
Comment #10
fabianx commentedConversion comments:
Its entirely possible to do this without using placeholders at all, which is a little strange as those placeholders will always be empty.
Similar to how a library can add empty data during hook_library_info_alter() this could just do:
$elements['#attached']['drupalSettings']['history_attach_timestamp']['node_id'][$entity->id()];
and then use hook_js_settings_alter() to add the specific things.
Train of thought
However, the placeholdered approach is more effective, because it allows caching in higher up levels and would lead to that this information is e.g. automatically retrieved via AJAX, while the page is cached in Varnish ...
This example shows how a conversion from #post_render_cache to #pre_render_cache can be almost done automatically.
And for #post_render_cache placeholders we have really nice DX again :).
Comment #11
wim leers#8:
Strong -1 on manually creating placeholders. It's too easy to create a not-actually-unique placeholder string.
#9.3:
Exactly. Plus, placeholders are not cacheability metadata, but bubbleable rendering metadata. Therefore this new metadata should not live under
#cache. Basically, this should replaceBubbleableMetadata::postRenderCachePlaceholders.#9.5: interesting, so you're saying that that code exists to prevent placeholder recursion? Then 1) that is not at all clear from the code or comments, 2) *why* is it impossible to do recursion?
#9.8: I still don't understand this. Can you include a concrete example in the code comments, using a render as the concrete example?
Review of the conversion:
Seeing these, I think this DX would make more sense:
and then, optionally, one can specify which render strategy to use:
I don't think so, I know of only one filter that actually uses this already (https://github.com/drupal-media/entity_embed)
Reviewing the "placeholders only" patch:
This should still remove the post_render_cache stuff.
This comment is confusing.
I think both "single flush" and "BigPipe" can use this markup, right?
It's only for ESI that we need custom mark-up, I think?
Comment #12
fabianx commentedPer quick discussions we are gonna do:
- Use #render_placeholder and #render_placeholder_strategy_hints as names
- Store the placeholders in ['#attached']['placeholders'] as they are truly out-of-band data and you should not attach placeholders yourself, also the format is suitable for the #attached merging we use.
Comment #13
yched commented['#attached']['placeholders']++, nice :-)
Had a cursory n00b look at the patch, I had the impression we're not completely consistent as to what we call "placeholder" in the var names, method names, code comments : the placeholder string, or the $element to render in its place, or the combination of both.
Probably no big deal, but precise naming could really help make the code easier to follow IMO :-)
Looks like this step doesn't actually need the $placeholder param ?
(--> in the spirit of the above, accurate name is renderPlaceholderElement() ?)
Comment #14
fabianx commented#13: Yes, maybe to be consistent ['#attached']['render_placeholders']
++ to renderPlaceholderElement
++ to using #render_placeholder => FALSE in above function
However the knowledge of which placeholder string this is rendering will be needed in the future, so I would like the API to support it.
The reason being that - once we have auto-placeholdering - when something takes 3s to create, but then it turns out to be not cacheable, we need to placeholder it, but that would mean to take 3s again. Therefore the BigPipe patch had a static for-this-request cache to store data about already generated PlaceholderElement markup.
Comment #15
fabianx commentedWim is continuing with this.
Comment #16
dawehner... its hard to even understand if things aren't described what this issue is remotely about.
Comment #17
wim leersAs a first step, only doing #12.
Will fail with 99% probability because testbot is currently broken.
Comment #19
hass commentedIs this issue the fix for #2474353: Cannot replace placeholders with #post_render_cache?
Comment #20
fabianx commented#19: Yes, it is a part of it.
To really fix this, we will also need to render CSS and JS in its own "rendering context" instead of render it after all #post_render_cache / #placeholders had already been processed.
I am still working out the details on how this will work best.
Comment #21
wim leersTo get back into this, I tested all scenarios that are being updated in this patch. And found that the comment.module's node links don't work.
I'm removing the changes to the comment module's node links. Because that is broken in HEAD already. Being fixed at #2496399: Comment module's node links' front-end perf-optimizing drupalSettings are missing. This allows the patch to remain focused on the problem at hand, and not fixing already-broken things.
Comment #22
wim leers#17 moved
['#cache']['placeholders']to['#attached']['placeholders']. The unit test expectations weren't updated for that yet. This fixes that, which reduces the number of test failures to close to zero, but don't let yourself be fooled, this is nowhere near ready yet.In the next reroll, removing
#post_render_cachebubbling/support. I won't change the code restored in #21, but that won't hinder this patch, because that is effectively dead code at the moment anyway; once #2496399: Comment module's node links' front-end perf-optimizing drupalSettings are missing is fixed, we'll start seeing failures, and then we can fix it.Comment #24
wim leersHere's a first round of removing a fair bit of
#post_render_cache. NotablyBubbleableMetadatais now completely free of it.Keeping at "needs work" because surely the number of failures will increase. Just posting several patches before posting a "needs review" patch, so that the individual interdiffs are manageable.
Also: the
(get|add|set)Assets()->(get|add|set)Attachments()change and the deprecation of the former can and should happen in a child issue. That also includes the bug in HEAD inaddAssets(). It's a pre-existing problem that's just made more apparent now that#attachedcan contain placeholders.Continuing this work now.
Comment #25
wim leersIn HEAD,
#post_render_cachecallbacks are able to completely wipe the rendered markup. It is up to the#post_render_cachecallback itself to only replace the part that it must replace, i.e. the placeholders.With this patch, doing that becomes impossible: because the return value of the (
#pre_render_cache) callback now automatically replaces the placeholder in the rendered markup, and only the placeholder.Next: making
RendererPostRenderCacheTestpass. Then all Renderer unit test coverage is green again, and I'll move back to "needs review".Comment #26
Crell commentedAre you saying that as a good thing or a bad thing? It sounds like a good thing to me, but I want to make sure we're on the same page about that. :-)
Comment #27
wim leers#26: Good thing, absolutely :) It just made it quite mind-bending to update the tests to 1) pass, 2) still test what we want it to test :)
Comment #28
wim leersWorked all day long to get
RendererPostRenderCacheTestto pass as well, and now it does! :)The next tricky thing was this: in HEAD, the results of a
#post_render_cachecallback can never ever be render cached. But with this patch, that changes — intentionally.For example, imagine a block is rendering some static text and an entity. If the entity's access says that is can only be cached per user then we want to be able to render that entity as a placeholder, so that it can be replaced later. This means we can render cache the block, but have that block then at the last moment replace the placeholder with the entity. We're good so far, but here comes the new aspect.
We also want to be able to render cache that entity, on a per-user basis, so that we don't have to render it on every page load (we don't want to do this automatically though, we'll only do it if
['#cache']['keys']is set, so no need to worry there).In other words: we want the very render array that was rendered into a placeholder to also be render cacheable, so that when placeholders are replaced, that doesn't have to mean rendering from scratch every time.
Given the simple example of having 1000 pages and 1000 users: HEAD would create 1000*1000 variations. With this approach, we would create 1000+1000 variations. That's 500 times better (and thus assuming perfectly distributed requests, a 500 times better cache hit ratio).
The reason this was tricky: HEAD's test coverage relied on the fact that when the same element had both cache keys and a post-render cache callback, that the render cache item always contained the placeholder. Given the above, that's no longer true. But we can still achieve the same test coverage by doing something similar to the "block containing an entity" example: make the parent (block) have cache keys and the child (entity) use a placeholder, then test that the parent always contains a placeholder.
By using data providers and passing different ways of using placeholders (generated or manually created, and cacheable or uncacheable) to the various test scenarios, I was able to hugely simplify the unit test, and at the same time have better test coverage and already pave the way for auto-placeholdering's test coverage.
Thanks to this, this is now a net-negative patch (
22 files changed, 588 insertions(+), 695 deletions(-)), which is a nice benefit/side-effect :)In doing this work, I've also cleaned up the changes to
Renderer, and the placeholder processing logic should be in a near-final form. Thanks to added code comments, it should now be significantly easier to understand. The rest will still undergo clean-up. And of course, documentation needs to be improved.This is still not yet completely done/cleaned up, but all
Rendererunit tests do pass now! More work to follow. This is not yet in a reviewable state. (Except for Fabianx, perhaps — seeinterdiff-commits.txtfor the individual local commits that lead to this interdiff.) Keeping assigned to me.Comment #30
wim leersRemoving the exception that I've been using while rolling this patch should fix quite a few failures.
Now continuing the actual work again.
Comment #31
wim leersNot in its final form yet, but here's an initial update to the Filter module.
Comment #33
wim leersConvert all remaining
#post_render_cacheuses and references… with the exception of the ones for comment links that are broken in HEAD; those will be migrated once #2496399: Comment module's node links' front-end perf-optimizing drupalSettings are missing lands.This hence includes fixes for the last failing tests. This should be the first green patch!
Now we enter the final stages: the next patch will be a clean-up patch, to clean up oddities in the current patch. And then we begin the real finalization process: self-reviews and reviews will be done to get this patch to an RTBC state.
Comment #35
wim leersMost notably cleans up
\Drupal\Tests\Core\Render\RendererPlaceholdersTest(formerly called\Drupal\Tests\Core\Render\RendererPostRenderCacheTest). But really cleans up allRendererunit tests: not a single trace of#post_render_cacheleft in them!Comment #36
wim leersAnd now the removal of all references to
#post_render_cacheis done, with the exception of:drupal_render_cache_generate_placeholder()incommon.incandRendererInterfaceNext: addressing point 2, at which point the removal of all references will be complete for now.
Comment #37
wim leersAs promised, #36.2 now addressed: all references to
#post_render_cachenow removed!Changes in this reroll:
RendererInterface::generateCachePlaceholder()anddrupal_render_cache_generate_placeholder()because they are now useless.Renderer's creation of placeholders and invoking of#pre_render_cachecallbacks, to be more consistent with existing code.public function createRenderCacheArrayPlaceholder()toprotected function createPlaceholder(). As far as this patch is concerned, this does not need to be public. The auto-placeholdering/render strategy/BigPipe patch can still make this a public API, if truly necessary. No need to do that here.RendererInterfacedocs updatedFilterProcessResultno longer usesRendererInterface::generateCachePlaceholder()and therefore no longer has a hidden service dependency.This is now partially ready for review, now doing the work to make it completely ready for review :)
Comment #38
wim leersBased on discussions with @Fabianx, we concluded that "render placeholder" (
#render_placeholder) is too ambiguous. It's not clear if it means"render a placeholder, as in replace it", or
"render a placeholder, as in generate placeholder markup"
Hence the following improvements in this reroll:
#render_placeholder->#create_placeholderprotected function Renderer::processPlaceholders()->protected function Renderer::replacePlaceholders()#render_placeholder_strategy_hints, because the concept of "render strategies" are completely out of scope here; it's up to that issue to add themComment #39
wim leersMinimizing the changes in
StatusMessages. There were a bunch of unnecessary changes in there.Comment #40
wim leersIn this reroll:
#pre_render_cacheis renamed to#builder.Rationale: see the relevant portions of an IRC discussion with @Fabianx below:
Comment #41
wim leersAdded beta evaluation.
Comment #42
yched commentedHow does #builder play (or clashes) with #after_build ?
Comment #43
wim leers#42:
#after_buildis a pure Form API concept; it exists only inFormBuilder, not inRenderer. There's no clashing or playing.First,
FormBuilderdoes its thing, including executing#after_buildcallbacks. It returns a render array. That's the controller result. TheMainContentViewSubscriberdetects a render array and uses theRendererto render the form (and blocks etc) and turns it into a page.Comment #44
wim leersLOL, the patch + interdiff in #40 were the same as those in #39. So ignore the files in #40, but do read the comment.
Comment #45
effulgentsia commentedBut $form and form element arrays are also render arrays, so there might be a conceptual clash between #builder and the "build" terminology of FAPI: e.g. #after_build. What about renaming to #generator or similar?
Comment #46
effulgentsia commentedOr even, #lazy_builder / #lazy_generator?
Comment #47
berdirHaven't reviewed yet, just want to point out that entity_embed is not the only contrib module that uses #post_render_cache. Poll uses it as well since poll forms and especially the results (and whether the form or the results should be displayed) is very much per-user. And payment users it as well, to display a form in a field, very similar to comment.module.
And masquerade seems to use it as well.
Comment #48
wim leers#47: true. Masquerade is also switching away from it though. Sounds like Poll will indeed continue to use
#post_render_cachein the form of placeholders.I'm merely stressing in the IS that quite a few of the early D8 contrib modules can be made simpler, because they no longer have to use placeholders, they can just use cache contexts in quite a few cases.
Good feedback though, thanks :)
Comment #49
fabianx commentedI like #lazy_builder, it also works nice with the plan to have a PreparableLazyBuilderInterface (where you have prepare() and build() functions, so we can support placeholders with e.g. preloading of entities).
Comment #50
xanoI use
#post_render_cachein contrib to display a form in a field formatter.I have no time to go through the entire patch today (and maybe not tomorrow). Could someone post a small example (or point it out in the patch) of how calling code should implement this change?
Comment #51
wim leersI had a call with @effulgentsia yesterday, walking him through the patch. Remarks he had:
#buildercallback is for building a render array, whereas a#pre_rendercallback is for decorating an existing render array. The docs should stress this difference.#buildercallback. Otherwise, two#buildercallbacks might generate the same child, and we wouldn't know how to merge them. So, if a#builderstill wants to spread responsibilities across multiple functions, it should either invoke hooks, or use multiple decorating#pre_rendercallbacks.#buildercallback is encountered but there are children (Element::children()), then a helpful exception should be thrown.All implemented.
Comment #52
yched commentedFYI, #2496039: Formatter's #attached assets are not carried over by Views is introducing another mention of #post_render_cache (not an actual use, just an isset check), will need to be adjusted depending of which patch gets in first.
Also, comment #9 over there has a question/suggestion for @Wim / @Fabianx :-)
Comment #53
andypostquickly skimming a patch I'm wondering:
- reason for
#create_placeholder = true- is there a case then it false? better DX would be not require people to provide TRUE each time- name
#builder- post_render_builder looks more descriptiveOverall it looks like basic replacement for post render cache so why break all 8.x code?
Maybe there's other way to preserve BC or build that on top of existing "post_render_cache"?
explains the need in the key but maybe there's other way to check recursion?
so #builder checked twice?
looks copy/paste...
looks #builder is a requirement to create placeholder, so why not relay on it presence?
Example code shows that all implementations could be updated directly - for @Xano
Comment #54
wim leersOn that same call with @effulgentsia, we also discussed whether
#buildercallbacks should even receive$elements. I unfortunately don't remember the details/our joint conclusion of that. The main (but only!) trickiness is how to deal with the#cacheset on the original element, but that's easy enough to merge onto the render array returned by the#buildercallback!So, I tried (
failed-attempt1-to-stop-passing-render-array-to-builder-callbacks--interdiff.txt), and that's how I found out the hard way why this doesn't work: because restoring#cachemeans restoring#cache[keys], which means that it is impossible to render cache the results, because the cache keys will only be available *after* the render cache has been checked.So, instead, here's improved documentation.…
Having done written #51, and then having written the above made me realize something: because we still expect the original
#cacheto be persisted, in its current shape,#builderis only half-assed: because it persists#cache, it actually has part of the "decorating" aspect of#pre_render. And it is only for that bit that we're doing this.So, let's make this both simpler and more consistent: if the render array built by
#buildershould be cacheable, then#cacheshould just be set by#builder. Period.…
I then spent considerable timing doing this, only to notice that this too was flawed (
failed-attempt2-to-stop-passing-render-array-to-builder-callbacks--interdiff.txt): if we always call#builder, then we still cannot actually have render cache hits! Sigh :(Conclusion:
#pre_renderis decorating an existing render array, and#builderis building a render array. But#builderdoes start with#cacheset (and nothing else), precisely to allow for render cache hits to still happen.(Also fixed a small bit of duplication introduced in #51.)
Comment #55
wim leersPer #45, #46, #49: renaming
#builderto#lazy_builder.Other feedback: thanks! Will address all of it in my next comments!
Comment #56
dawehnerShould there be some example documentation somewhere in the patch to explain how to use #builder callbacks?
I'm curious is #attached really the right place for placeholder storage?
I like the term #builder here
It would be great to explain why we need a custom placeholder for filters ...
Comment #57
wim leers#50: answered by the CR that I just created: https://www.drupal.org/node/2498803
Comment #58
fabianx commentedTrain of thought
We have three cases of render arrays to distinguish:
I like that builder works in the way that it just returns the build render array, it does not even need to be able to change #cache in anyway.
If it _really_ needs a different cache layer, it should just use a child with a cache key, the same for more placeholders.
--
The advantage of one builder callback is:
- Can use services with injected dependencies and hence be called e.g. from a Middleware if no dependency on e.g. current request
- Service can have an interfaces to make multiple preparation possible (e.g. ->prepare() and ->build() on a list of items with the same builder)
#53:
Why is the API change needed?
Post Render Cache means you can do _everything_ you want to markup and that is a problem, because there is a world after Drupal, too.
e.g. if you want to cache things per role in Varnish and you have a post render cache that adds all unique user information, then that is pretty much impossible to resolve.
In essence you could still do anything that post render cache did in a custom response subscriber, but we encourage you to not do it.
While Post Render cache was mainly used to create placeholders in core, it can theoretically be used for anything and that is why its not possible to keep it around as else we lie over our goals.
Also with placeholders you give core much more possibilities to run the "placeholder callback" at the best suitable time.
See the demo at the beginning here:
https://events.drupal.org/losangeles2015/sessions/making-drupal-fly-fast...
Comment #59
wim leers#52: thanks for the note. Posted a review/suggestion on that issue, which would remove that mention of
#post_render_cache.#53: Basically all your questions point to the fact that the IS is severely lacking. The IS should be answering all of those questions. So, instead of answering your questions directly, I've updated the IS. You'll find the necessary answers there. It's a relatively long read, but I promise it will be worth it :)
!isset(#create_placeholer) || #create_placeholder == FALSE), this would run early during the rendering (same phase as#pre_rendercallbacks — since they're conceptually similar), if placeholdered (#create_placeholder = TRUE), this would run after the rendering of the HTML, using SingleFlush or BigPipe.Comment #60
effulgentsia commentedRaising to critical per #1744302: [meta] Resolve known performance regressions in Drupal 8's guideline of Over ~10ms savings with warm caches and would have to be deferred to 9.x. The necessity of the BC break of #post_render_cache satisfies the latter part, and #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts is very likely to achieve the former part but requires this issue first.
Comment #63
wim leers#56:
#lazy_builder! Also see IS update and recent comments about that for more rationale.#53.1: I actually already had a way to get rid of that, because during the code was refactored to be clearer/more consistent/simpler, and in doing so, I made it impossible to hit the infinite recursion case!
Details: when rendering the placeholder,
#create_placeholder === FALSEalways, which means we don't create a placeholder, then we execute the#lazy_builder, which even if it sets#create_placeholderat the root level is simply ineffective.Also cleaned up/improved other exceptions + associated tests.
Comment #64
wim leersThere are a few more small changes lined up, but that's just further clean-up. I will do so tomorrow.
This is basically ready for review now!
Comment #65
effulgentsia commentedThis patch looks great to me. Noticed some minor nits, but holding off on those pending #64. My only non-minor nits:
Why? Why not remove that arg and only pass $context?
Where is that enforced? If nowhere yet, can we add an exception?
Comment #66
wim leers#65
#cachebeing already set (and specifically,#cache[key]s), it's impossible for the contents of what is placeholdered itself to be cached. (And yes, that's super valuable: imagine the entire page being cached across users with roles A and B, but one block being per-user — it's valuable to be able to render cache that block per user, so that it doesn't need to be re-rendered for every page load — what matters is that the block didn't infect the entire page!)#post_render_cacheeither.Comment #68
effulgentsia commented#66.2: Per top of #58, can't we document and enforce that the builder callback is not allowed to return an element with
#cache['keys']set at the root level, and that instead that property may only be set on the original declaration? I'm still unclear on why the callback itself needs the original element declaration passed in. Meanwhile, I think cache tags and contexts would work just fine when set either (or both) in the original declaration and/or within the callback: can't the renderer merge those without needing to pass them to the builder callback?#66.3: Yes, but this issue is about breaking the BC of #post_render_cache. Let's not have to break it twice.
Comment #69
effulgentsia commented#66.1:
Since this is required to be single, what about changing it from array to
#lazy_builderas the callable and#lazy_builder_contextfor the context? Or maybe even drop the "context" terminology and turn it into#lazy_builder_argumentsand use call_user_func_array instead of call_user_func?After applying this patch, the codebase still has a bunch of "post_render_cache" terminology left in it, like here. Let's clean that up.
"pre_render_cache" is leftover terminology from earlier versions of this patch. Let's clean that up.
Comment #70
effulgentsia commentedWhy do we need/want the args (other than the uniqueness token) in the placeholder markup? AFAICT, we don't later parse it from there, so looks to just be extra useless bytes. If there's a reason, such as supporting replacement from a reverse proxy, or to aid debugging, please add a comment about that.
Comment #71
wim leersApplied @Fabianx's IS improvement suggestions.
Comment #72
wim leers#68
RE #66.2: answering this in my next comment.
RE #66.3: fair! Done. (Fun fact: PHP calls these "scalars", not "primitives", and strangely, NULL is not considered a scalar… so I ended up with
is_null() || is_scalar().)#69:
Not sure about "arguments" instead of "context". I discussed that with Fabian:
If we would be able to use
call_user_func(), then it'd make sense, yes, but how could we do that, without using reflection?Not yet addressing this, needs more discussion first. Added to IS to make sure we don't forget.
Over at #2451411-65: Add libraries-override to themes' *.info.yml, @alexpott said that we shouldn't use
SafeMarkup::format()for exception messages (despite many in core doing so), but usesprintf()instead. So changed that too.Comment #74
wim leers#68, RE #66.2:
#lazy_buildercallbacks when replacing placeholders. That is:#attached[placeholders][<placeholdermarkup>]just contains a super thin render array: one with#cache+#lazy_builder, nothing else. So, replacing the placeholder is a matter of simply rendering that super thin render array!Because rendering that super thin render array (with
#cache+#lazy_builder) means that the cache keys are already present, we can just call the renderer with that render array, and render caching works.I failed to get it to work in #54, but I've now found a way to make it work (I actually was very close in #54, I was probably too tired to see it back then). So, yay! :)
Thoughts on these changes? I like it a lot :) This was my biggest frustration/concern with the patch, so very glad to have that out of the way.
Next:
$contextintroduced there)Comment #75
wim leers#74:
<p>overridden</p>string used for testing placeholders didn't make sense anymore, because it's now impossible to just simply/stupidly override all markup. And I missedbubblingPreRenderCache, should've renamed that as part of addressing #69.2.)Comment #76
wim leers@Fabianx remarked that the way
Renderer::renderPlaceholder()is currently implemented makes it impossible to be used by the BigPipe render strategy, because BigPipe will not be able to merge the bubbleable metadata from the rendered placeholder with that of the rest of the HTML (which makes total sense) — it will e.g. have to load additional assets by including some JS to load those additional assets without loading the already-loaded assets again (i.e. something likeDrupal.loadAssetLibrary(), which takesdrupalSettings.ajaxPageStateinto account).But! That's not something we need to solve here. We can just keep that method protected for now, and make it public (and a public API) as part of #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts. That helps keep the scope of this issue well-defined.
Comment #77
wim leersComment #78
yched commentedVery minor and silly question about that sample in the IS :
Why not use a shorter snippet with a self-closing tag (
<drupal-render-cache-placeholder STUFF/>) ?Comment #79
wim leers#78: because self-closing tags are HTML5, not HTML4, and PHP's
DOMDocumentis HTML4-only. That's the only reason.PHP--
Comment #80
wim leersPer #2496399-5: Comment module's node links' front-end perf-optimizing drupalSettings are missing, not blocking this issue on that, and just merging the solution into this issue, since there'd be a lot of overlap anyway (and several of the changes needed there are obsolete with this issue! :)).
Which means we can solve this in a simpler, less silly way than that other issue was doing it. Because the sole remaining
#post_render_cachefunction (CommentLazyBuilders.php::attachNewCommentsLinkMetadata()) and#post_render_cacheattachments (inCommentLinkBuilder::buildCommentedEntityLinks()) is a pretty curious case, and is the reason it was broken without anybody noticing. That is, it's a recursive post-render cache callback. i.e. when a node is rendered and its links are built, that happens via#post_render_cache. But then HEAD's broken code, which is called by that#post_render_cachecallback, adds two more! This is pointless, because obviously it already is a placeholder.So the solution is very simple: don't attach additional
#post_render_cachecallbacks (which were only attachingdrupalSettings), but simply attach thosedrupalSettingsdirectly. This is fine, because we're already rendering a placeholder! :)Next: addressing #69.1.
Comment #84
wim leersNo remaining tasks! I consider this ready for final review :)
I discussed #69.1 with both @effulgentsia & @Fabianx. We agreed that it'd be better to change
and
function some_callback(array $context) {…}to:
and
function some_callback($foo, $bar) {…}The syntax then actually matches the architecture: the syntax only allows you to have a single
#lazy_buildercallback. And the syntax is identical to the one we have elsewhere for dealing with callbacks: an array containing two values: the first being the callback, the second being the arguments.This means that not only does the syntax now match one's expectations, it also means we can finally properly document these callbacks. Adding such documentation is a significant part of the changes you'll see in this interdiff.
And finally, thanks to this change, I needed to/was able to get rid of the "placeholder_token" stuff that was being generated, and generate a token automatically based on the hash of the serialized thin render array that is retained. (Just like @Fabianx's patch in #17.) Which means that a placeholder's markup depends solely on the thin render array associated with it (#cache keys + #lazy_builder callback + #lazy_builder arguments). Therefore, if the same combination of those 3 things exists multiple times on the page, they'll be de-duplicated automatically, and all such placeholders will be replaced in one go. Without having to think about it.
Hence you'll see a huge amount of simplification in
StatusMessages. (To convince yourself: place multiple "status messages" blocks, note how they all work, and each have the same placeholder markup.)IS & CR updated.
Comment #86
fabianx commentedQuick interdiff review:
+1000 to asserting that!
I thought about this again and hash() is enough.
A hash_hmac is not needed here, because if you have access to write markup like
<drupal-render-placeholderyourself, you can do way worse things with<scriptthan ever with using a placeholder markup, where there should be none.And collisions can only occur if someone re-uses the exact same markup in a help text or so, but that should be okay as they should change the token in that case ...
Are we sure we don't want to use arguments here as well?
This ternary operator looks wrong to me ...
Comment #87
wim leersFixed test failures.
Comment #88
wim leers#86:
Renderer::createPlaceholder(). That was my intention, but I forgot.Comment #89
fabianx commentedRTBC, This looks really fantastic now!
( Justification for RTBC: Wim has touched every single line I had in my original patch, so there is nothing left that was not verified by him, it also had several reviews in between by other people, so I feel like I can RTBC this even though I initially worked on this.)
Comment #94
catchOverall this is fantastic, I found a few minor issues and had a few questions:
Should this have a 'to be removed by' note on it?
dreditor cut off the bit of the function I was trying to paste, but nice diff here.
Feels like it'd be useful to move this logic into a helper on ControllerResolver - sure have this pattern elsewhere too, but just a niggle and off-topic here really.
Nit: We're replacing cache keys rather than merging them - i.e. we don't support that being part of $build. Comment suggests we merge the cache keys in a similar way.
ubernit: should this be elements' (plural possessive) to match the variable name?
Slightly off-topic again, but for any strategy other than single-flush, isn't this going to cause problems? If we've already sent most of the page before that exception gets thrown then it'll be too late - unless we somehow force the entire page to re-render via js or something? Or throwing 404/403 exceptions randomly might need to not happen any more.
+1.
nit: is present.
If a #lazy_builder callback isn't present, should this also throw an error?
It's not clear how the comment relates to the code here - there's no reference to pre_render - or is it just that this runs before #pre_render.
Also isn't #pre_render kept not just for backwards compatibility but also for different use cases?
So 'poor cacheability infection'. We've had this concept informally for a long time, but I think this is the first case of trying to put a name to it, at least in core.
I'm not sure about using infection terminology though - for a start it sounds like 'cache poisoning' which is a different concept. Infection suggests that the cache context is never wanted (i.e. a bacteria or a virus) - whereas cache contexts specifically protect us against infection/poisoning - that's the point of them.
Would 'cache context tainting' or 'cache context contamination' be any better?
Tainting/contamination suggests the context is OK in the correct context (no pun intended).
You don't want lead in water but you do want it around nuclear waste. You don't want sugar in petrol but you do want it in cakes. User and route cache contexts are fine as long as they're isolated.
1. Why are the arguments in the markup?
2. Why put them as query parameters?
I think I know where this is leading, but at minimum it needs a good comment explaining, or possibly remove from here and add back later.
Do we need to be concerned at all about people adding
<drupal_render_placehoder></drupal_render_placeholder>in text fields at all? Is there any test coverage that deals with an invalid render placeholder?Is this like SplObjectStorage? ;)
Comment #95
fabianx commentedMissed that in final review:
So why do we duplicate all of that from the renderer?
All that should be needed here is:
Comment #96
fabianx commentedSome more nits, two the isset() checks can be follow-up as adding the check is not an API change.
Lets check for && !isset($elements['#lazy_builder_built']);
Lets check for && !isset($elements['#lazy_builder_built']);
Lets use #lazy_builder_built please.
Comment #97
wim leersNotFoundHttpExceptionto force the page to 404, we're gonna have a problem. But shouldn't only the controller be able to do that? So not 100% certain this is really a problem.Comment added to document this.
<as976876dsflakjsdfasdfjkl>. The only way a placeholder is going to be replaced, is if there's associated#attached[placeholders]metadata.Similarly, there is no such thing as "invalid" placeholders: they're just markup, there's nothing to be valid or invalid. The only way placeholders can be invalid, is if they're invalid HTML. And we have test coverage for that. See
\Drupal\Tests\Core\Render\RendererPlaceholdersTest::testCacheableParent().testRenderChildrenPlaceholdersDifferentArguments()— a pretty crazy complex test.In addressing this feedback, I saw room for improvement in two areas:
Renderer::renderPlaceholder(), and move it toRenderer::render(), just like we had it in prior iterations — EDIT: this fixes #95'placeholders'attachments: #2471228: Optimize merging of attachments optimized that, and we can do much better than the default in this case: we can just merge at the top level: anything below#attached[placeholders][<unique placeholder markup>]is required to be the same anyway (i.e. "if same placeholder, then the same data to replace that placeholder")And I also fixed a few nitpicks/cleaned up a few small things.
EDIT: (after noticing I was cross-posting) also addressed #96. #96.1/#96.2: no need for that: once a
#lazy_builderis executed, it is removed from the render array. #96.3: done.Comment #98
fabianx commentedBack to RTBC, Interdiff looks great!
Comment #99
catchSorry #13 was a bad dad joke - SplObjectStorage lets you use objects as array keys, that parameter lets you use animals as array keys... Not actual feedback.
Interdiff and answers all look good, but getting late here so I'll look again and one more once-over tomorrow.
Comment #100
catchCommitted/pushed to 8.0.x, thanks!
Comment #102
dawehnerCongratulations!!
Comment #103
wim leersHurray! :) Coming back from vacation and seeing a big critical has landed, wonderful :)
Next step: #2499157: [meta] Auto-placeholdering.
Comment #104
wim leersJust updated all affected CRs: