Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered)

Note: this will fail at least BlockViewBuilderTest. I felt it was more important to show the key parts of the patch soon, than to post a perfect patch.

This should fix the 11 failures in BlockViewBuilderTest.

• The fact that we now use a #lazy_builder no longer allows us to easily unset #theme, which means we cannot easily avoid the theme system executing just to simplify tests, therefore… as of this patch, we don't do that anymore.
• The above makes it much harder to test the $build['#suffix'] = '<br>Goodbye!'; test case, so I've changed it to $build['#attributes']['foo'] = 'bar'; — we're still testing the same modifiability.
• Then… what is surely going to be the most contentious change: this patch no longer allows cache keys to be altered in. We explicitly kept this (in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags), but I don't think it makes sense. If you are using a specific block in a very narrow, optimized way, and you're smart enough to alter its cache keys (well, that should actually be cache contexts, but anyway), then you're also going to be just fine with subclassing the existing class, and overriding the getCacheContexts() method. If we really turn out to have a need for such an alter hook, we should add a separate alter hook.

And apparently the help and statistics blocks also aren't placeholderable, because both carry state: they store data in properties in their respective classes in their blockAccess() implementations.

So this is quite an excellent way to detect stateful blocks.

This should be green.

Fixed the statistics block to not be stateful instead.

(It was written in that way because it used to be the case that to not render a block, you had to have the block's access result not grant access, even if the user actually had access. Because otherwise it'd always render the block.)

#5: Excellent points. You're right regarding ESI & BigPipe — that comment is a tad wrong. And agreed on the concern that cache contexts should already do this, but there's a difference here: these blocks carry state. And that is the real problem here: if a block is relying on $this->something, that got set either in a blockAccess() method (like Help & Statistics), or that is injected because it is a very special block (the main content block has a ->setMainContent() method that is necessary for the rendering system to be able to get the main content into the block without relying on state).

So, I've been thinking that StatefulBlockPluginInterface would actually be a better name. Thoughts?

Also fixed the Help block to not be stateful.

#5/@yched: Please look at the #6 & #8 interdiffs. I think they quite clearly demonstrate what the "problem" is with these blocks.

99% of core's blocks don't do this. So, I'd think that only in rare cases, you have to think about this.

Oops, I was slightly too fast: forgot to update the Help block to no longer use that interface.

Attached is also an interdiff that shows all changes for #6 -> #11 (this comment). It is that diff that most clearly answers @yched's questions in #5.

Fail caused by fixing the help block. Now the help block's url cache context bubbles always, like it should. Fixed test expectations.

Also simplified HelpBlock code that I already touched slightly more: I was able to just delete it because it now was identical to the base implementation :)

#14.1: renamed to StatefulBlockPluginInterface.

#14.4: I'd prefer to not implement any of those suggestions in this issue, because:

[…] this patch no longer allows cache keys to be altered in. We explicitly kept this (in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags), but I don't think it makes sense. If you are using a specific block in a very narrow, optimized way, and you're smart enough to alter its cache keys (well, that should actually be cache contexts, but anyway), then you're also going to be just fine with subclassing the existing class, and overriding the getCacheContexts() method. If we really turn out to have a need for such an alter hook, we should add a separate alter hook.

#14.5: We need to again load the block config entity, but that's statically cached already anyway. It uses a static method because BlockViewBuilder is not a service, and you can only point to an object if it's also a service. Hence static method.

#14.6: Yep, see #15 :)

Fabian asked me to do the fixes for the Help & Statistics blocks in a separate issue, to keep this issue focused. Opened #2543554: Clean up Help & Statistics blocks for that (patch already posted!).

• RE: 14.4: So how can you change the cacheability of a block from an outside module then? — I already answered this:
  

  If you are using a specific block in a very narrow, optimized way, and you're smart enough to alter its cache keys (well, that should actually be cache contexts, but anyway), then you're also going to be just fine with subclassing the existing class, and overriding the getCacheContexts() method

  Why is that not sufficient?

• RE: 14.5: we could make it a service in the issue where we introduce that LazyBuilderPreparableInterface, but right now, it is hard to justify.

#21.4:

#21.5: I wouldn't know how to word that issue; it's a proposal you made — that makes sense at a high level — but I think it'd be better if you create that issue. You'll want that issue anyway, regardless of this one, so :)

#22: many thanks for your review! It's much appreciated!

1. I agree that "Stateful" is a less than great name. :( It was "NonLazyBuildable" before that. Fabianx preferred "Stateful". Also see next point.
2. RE: now that I think about it […] properly written […]: totally agreed. That's why we have #2543554: Clean up Help & Statistics blocks, to fix the broken cases. But, there are legitimate use cases too, like the main content block. I already explained in #7 why it was done that way for the main content block: the main content block has a ->setMainContent() method that is necessary for the rendering system to be able to get the main content into the block without relying on state. It also doesn't make sense to placeholder the entire main content.
   This is why I originally called it "NonLazyBuildable".

   RE: your proposed alternative interface docblock: I didn't go with that, because the "out of sequence" part doesn't capture what the problem is. It really is about a block plugin instance that is carrying state. i.e. this line:

     public static function preRender($build) {
       $content = $build['#block']->getPlugin()->build();
   

   When that is called immediately (like we do for stateful block plugins in the current patch), then $build['#block'] is set to the entity that has the plugin instance that contains the state (e.g. the main content injected into the main content block plugin). But, if it is lazily built, then a new block plugin instance is created, which hence doesn't have the state.

   So, to make a first step to address your feedback (I'm sure we'll hash it out further in subsequent comments), I already moved the docs you liked better from BlockViewBuilder to StatefulBlockPluginInterface.

3. Sorry, yes. Fixed. PHP--
4. Also fixed. Why static? Already answered in #17, but repeated for your convience: It uses a static method because BlockViewBuilder is not a service, and you can only point to an object if it's also a service. Hence static method.
5. See previous point. Also: http://drupal4hu.com/node/416.html
6. It does, because: interface MainContentBlockPluginInterface extends StatefulBlockPluginInterface.

#2543332: Auto-placeholdering for #lazy_builder without bubbling landed. And soft-blocker #2543554: Clean up Help & Statistics blocks landed also!

Lets' get this moving forward again? This has been blocked on reviews for almost a week now. (EDIT: by that I meant: gently nudging Fabianx & Crell :P — whose feedback I replied to in my previous comment and am waiting for their further thoughts so we can work towards consensus.)

First, a straight reroll now that #2543554: Clean up Help & Statistics blocks has landed, because #23 still contained that. So this patch is now smaller again, like it originally was.

1. Agreed.
2. I was merely mimicking other code in core, but sure, done.
3. Done. Much better :) Thanks!
4. Filed #2548817: Remove SafeMarkup::checkPlain() in BlockViewbuilder.
5. ?
6. Fixed in #2543554: Clean up Help & Statistics blocks.
7. Good question. We've always used Views' exposed filters block as the canonical example of "a block that depends on the main request, which therefore cannot be rendered via ESI". Perhaps that's no longer true? Can you point to a good test/STR to step through how Views does this, so I can verify the details myself? Documented based on my understanding of how Views deals with this, but likely inaccurate.

Will do the profiling and add the alter hook.

Before doing so, can you clarify this:

Can we somehow ensure that all blocks that use contexts from e.g. URL extend that class?

I have no idea what you mean by that.

From AggregatorFeedBlock:

public function build() {
    // Load the selected feed.
    if ($feed = $this->feedStorage->load($this->configuration['feed'])) {

So this is not at all using contexts.

In fact, the only one that does AFAICT is \Drupal\plugin_test\Plugin\plugin_test\mock_block\MockComplexContextBlock, which exists only for testing purposes. Given that, I'm not entirely sure what you want me to do.

Tim Plunkett pointed me to \Drupal\block_test\Plugin\Block\TestContextAwareBlock, which is the one used in "contexts + blocks" tests.

And he also pointed me to #2377757: Expose Block Context mapping in the UI, which makes the feed block use contexts indeed. I bet that's why you thought that was the case :)

Working on this next.

#29.1: Blocked on Crell, added to remaining tasks in IS.

#29.2 + 36: postponing handling that to a next comment, until Fabian and I get a chance to chat so we can get on the same page

#29.3: Done. Also vastly expanded the test coverage: now tests not only cache keys and tags, but also contexts and max-age, plus all in combination (keys, contexts, tags, max-age), and also tests #create_placeholder. Added API docs. Ensured the wording is consistent with the existing docs for hook_block_view_alter().

#28, #29.4, #35: Up next.

d.o--

#28: done.

#35: done.

#29.4: done. In doing so, I discovered two small things that unnecessarily worsened performance:

1. using \Drupal::service('module_handler') in BlockViewBuilder::viewMultiple()instead of injecting that service; fixed
2. using Block::load($id) instead of entity_load('block', $id) in the #lazy_builder callback. (Block::load()required a fair bit of magic, and a single call to it resulted in ~70 additional function calls

I profiled both with and without the breadcrumb block marked as cacheable, since #2483183: Make breadcrumb block cacheable is very close, at which point all blocks are cacheable and none are auto-placeholdered.

Note that the wall time is effectively meaningless, there's a lot of variance on there, it is essentially equivalent. In none of the cases, the I/O differs, memory usage is equivalent, so only number of function calls matter.

/contact before vs. after

	Run #contact-HEAD	Run #contact-patch	Diff	Diff%
Number of Function Calls	39,289	39,528	239	0.6%
Incl. Wall Time (microsec)	103,833	103,124	-709	-0.7%
Incl. MemUse (bytes)	17,998,176	17,972,256	-25,920	-0.1%
Incl. PeakMemUse (bytes)	18,133,664	18,096,960	-36,704	-0.2%

/contact before vs. after, with cacheable breadcrumb block

	Run #contact-cacheable_breadcrumb-HEAD	Run #contact-cacheable_breadcrumb-patch	Diff	Diff%
Number of Function Calls	38,751	38,622	-129	-0.3%
Incl. Wall Time (microsec)	102,416	100,928	-1,488	-1.5%
Incl. MemUse (bytes)	17,830,688	17,825,176	-5,512	-0.0%
Incl. PeakMemUse (bytes)	17,996,352	17,950,240	-46,112	-0.3%

/node/1 before vs. after

	Run #node1-HEAD	Run #node1-patch	Diff	Diff%
Number of Function Calls	58,852	59,101	249	0.4%
Incl. Wall Time (microsec)	148,409	143,943	-4,466	-3.0%
Incl. MemUse (bytes)	22,044,952	22,113,360	68,408	0.3%
Incl. PeakMemUse (bytes)	22,157,432	22,209,560	52,128	0.2%

/node/1 before vs. after, with cacheable breadcrumb block

	Run #node1-cacheable_breadcrumb-HEAD	Run #node1-cacheable_breadcrumb-patch	Diff	Diff%
Number of Function Calls	58,310	58,181	-129	-0.2%
Incl. Wall Time (microsec)	141,880	142,577	697	0.5%
Incl. MemUse (bytes)	21,963,888	21,972,432	8,544	0.0%
Incl. PeakMemUse (bytes)	22,071,208	22,070,032	-1,176	-0.0%

Conclusion

Building the basic render array for a block is cheaper now. (As expected.)

When all blocks are cacheable (i.e. none are placeholdered), then this has slightly better performance. Because less work needs to be done on a cache hit: see previous point.

When not all blocks are cacheable (i.e. some are placeholdered), then this has slightly worse performance. Because render placeholders means rendering the placeholdered blocks in isolation, which has a small amount of overhead.

Note that with #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, that small amount of overhead only occurs after the majority (cacheable parts) of the page has already been sent. This is also what even allows us to render those blocks later, after the initial page is sent.

Effectively, performance remains unchanged.

So, this issue completely failed to address one part: Contexts, i.e. context-aware blocks. This is what @Fabianx was getting at in #29.2 & #36.

A. Discussion to find a better name for StatefulBlockPluginInterface

And while discussing #29.1 with Crell — i.e. determining the final name for what is currently called StatefulBlockPluginInteface — that came up too. We were exploring the reasons why a block plugin would need to implement that interface, to find the best possible name. And when we got to the point where we said block plugins that depend on more than purely what they get injected (services & configuration), we arrived at the natural conclusion/question: well, what about contexts, because we get the context manager injected, not specific contexts.
That brought us back to #29.2, and is where we got stuck.

B. Context-aware blocks are a problem

The reason it was easy to forget about Contexts/context-aware blocks is simple: core has zero actual blocks that are context-aware. (There are a few test-only blocks, but those are only tested in non-rendering ways.) That's being fixed slowly in #2377757: Expose Block Context mapping in the UI + #2550199: Add a ContextProvider for Feeds.

The problem with Contexts/context-aware blocks are manifold:

1. if it is possible to determine for any given blocks what the entire set of contexts is that a block depends on, it's not clear how that is determined (again, because no block actually uses contexts)
2. AFAIK we have the concept of both required and optional contexts. Knowing just the required contexts won't be sufficient. We need to know all input data for a block, so that includes optional contexts.
3. Assuming the two points above are not actually a problem, then the next question is: how do we serialize the actual contexts for a given request? The Context API, does not yet support that rather crucial feature. Berdir's proposal:
   

   optional SerializableContextInterface (because not all context can be serialized, not sure about the name as it could be mixed up with php serialize()), if present, use that and pass it along. if not, no lazy-building for you

   — but then we'd of course also need a way to deserialize the serialized Context back into an actual Context.

(I can't answer points 1 and 2 myself because I can find neither documentation nor test coverage. I'm sure tests exist, but not in the form of integration tests for blocks.)

Conclusion: the Context system is not designed to run in isolation, because if it were, we'd be able to both capture a specific context and recreate it. Neither is currently supported.

This is why @Fabianx said this in #36:

I am neither expecting full support for blocks using context here, nor full coverage, etc. This patch is great as is and way within scope!

I just want to make sure that we either exclude those blocks by default or give an easy way to exclude. And especially that we keep those in mind.

Similar to how we used max-age=0 for some uncacheable things, then later worked on the cacheability in a follow-up.

C. Context-aware blocks are actually not a problem

… at least not in the SingleFlush and BigPipe render strategies. (SingleFlush is the render strategy that Drupal has used forever: build the entire HTML response and then send it when it's completely ready.)

Because when we replace placeholders in both SingleFlush and BigPipe render strategies, we are still operating within the actual/original request context. Which means that blocks will actually be able to access the Contexts they need.

For an ESI render strategy, however, this breaks down. Because with ESI, the rendering of the placeholder happens in an ESI request, not in the actual/original request. Which means Contexts won't be available.

Conclusion: in the current situation wrt Contexts + Blocks, we are explicitly relying on a leaky abstraction, on imperfect isolation. Because the context system is not designed to be able to run in isolation (see earlier). And, that's even fine for SingleFlush and BigPipe, because those only render blocks in the actual/original request context.

D. Context-aware blocks: able to render them in isolation when using ESI

We need two things to truly be able to render context-aware blocks in isolation:

1. Ability to serialize and deserialize Contexts.
2. If the block itself varies by route or url (or url.*, e.g. url.query_args:foo), then the only way to render it in isolation is to do routing. Which means the ESI client needs to send the actual request URL to the origin, so that the origin (Drupal) can run routing. Drupal would get a X-Original-Request-Uri header.

This can be done in a follow-up.

Proposed next steps

(Issue title updated according to the proposed next steps.)

• In this issue: If a block has contexts (detect using !empty($block->getPlugin()->getPluginDefinition()['context'])) => make it non-placeholderable by setting #create_placeholder => FALSE.
• In a follow-up issue, only set #create_placeholder => FALSE if not all contexts can be serialized.

This comment and reroll are merely an unfortunate distraction. The next reroll will simplify the patch.

In a testing issue where I was trying something to simplify this patch further, I discovered that the last green patch (in #40) is no longer passing in HEAD. Specifically, \Drupal\system\Tests\Form\RedirectTest::testRedirectFromErrorPages() is failing (that test was introduced by #2206909: Regression: Form submit redirects from 403/404 pages are no longer possible, >1.5 year ago).

It's failing because it is testing that a form in a block (as opposed to a block in the main content area) is able to do a redirect on a 403/404 page (i.e. on a page where the kernel chooses what to render based on a kernel exception), and this patch automatically placeholders that block, which causes the redirect to occur later.

The root cause of the problem is that forms do redirects like this:

    // If the form returns a response, skip subsequent page construction by
    // throwing an exception.
    // @see Drupal\Core\EventSubscriber\EnforcedFormResponseSubscriber
    //
    // @todo Exceptions should not be used for code flow control. However, the
    //   Form API does not integrate with the HTTP Kernel based architecture of
    //   Drupal 8. In order to resolve this issue properly it is necessary to
    //   completely separate form submission from rendering.
    //   @see https://www.drupal.org/node/2367555
    if ($response instanceof Response) {
      throw new EnforcedResponseException($response);
    }

i.e. a pre-existing bit of nastiness in HEAD.

The reason it was passing in #40, is because #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor got committed in the mean time, which causes blocks to be rendered later, during the kernels' RESPONSE event. Which means that when the EnforcedResponseException exception finishes bubbling, it does not trigger a kernel exception, because we're already in the response event.

Whereas in #40/earlier HEAD, a nested kernel exception occurs (first a 403/404, then the EnforcedResponseException within the handling of that 403/404 exception).

In other words, this small interdiff fixes a bug in #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor, which only is an actual problem because Form API works in this crazy backward way :/

Attached is a reuploaded (identical, just rebased) #40 patch that is expected to fail, and a new patch with the fix (see interdiff).

Alright, the fail patch failed as expected, the other patch passed as expected. Great.

Now on to the next reroll, which will simplify this patch again.

So, a key part that we got stuck on earlier, is the naming of this new StatefulBlockPluginInterface. But, the only examples we have are the main content block, and the Views exposed filters block.

We always thought the latter is directly dependent on "the main View" for the page (i.e. the one that's the main content, i.e. generated by the route's controller). But a closer look at ViewsExposedFilterBlock made me doubt that. So, I stepped through it. And, indeed, the Views exposed filters block in Drupal 8 no longer depends on the main content at all :) It has its own, non-executed ViewExecutable that is constructed independently. So, hurray! This is great!

That means the only block with special needs is the main content block. And that block already is special.

Conclusion: we can remove StatefulBlockPluginInterface! A whole layer of complexity is avoided. When the need arises in contrib, we can still add something like this, with more wisdom and experience based on those real-world examples that we encounter — if any.

Patch & IS updated accordingly.

#45 contains a random fail. Easy fix.

So, now that we have a simpler, green patch, we can focus on the remaining problem: context-aware blocks. See #41 for a full analysis.

First, I needed my understanding/analysis of context-aware blocks confirmed. Tim Plunkett confirmed in chat that:

1. A block's context annotation indeed lists all contexts it may ever use:
   

   Tim Plunkett [21:26] those could be optional, those could include ones that are only used in certain code paths, but yes they would be in there
   Wim Leers [21:27] So a Block's `context` annotation lists all contexts it may *ever* consume, even if it does not consume them always? (edited)
   Wim Leers [21:27] (I want to make 100% sure I understand it correctly.)
   Tim Plunkett [21:28]  yes
   

2. One can thus find a block's contexts by doing !empty($block->getPlugin()->getPluginDefinition()['context'])?
   

   Tim Plunkett [21:29] ContextAwarePluginInterface::getContextDefinitions does
      $definition = $this->getPluginDefinition();
      return !empty($definition['context']) ? $definition['context'] : array();
   Tim Plunkett [21:29] so if ($block->getPlugin() instanceof ContextAwarePluginInterface && $block->getPlugin()->getContextDefinitions())
   

   … so it can be done even more elegantly than I anticipated :)

3. One thing I failed to mention in #41, but realized later, is the case of "configured contexts" or "static contexts" (not sure what the official term is, I think I've read both): if all contexts that a block could ever consume are specified in context_mapping, then the block effectively is being rendered in full isolation.
   

   Tim Plunkett [21:37]  it's basically like $used_contexts =  getContextMapping() + getContextDefinitions(). if everything in the definitions is in the mapping, then the mapping is the entire resulting set
   Tim Plunkett [21:37] see \Drupal\Core\Plugin\Context\ContextHandler::applyContextMapping
   

Conclusion: we can easily make sure that blocks which depend on contexts that aren't defined in context_mapping will get #create_placeholder = FALSE set, to prevent them from being rendered outside the main request.

I forgot to set #47 to NR. Such noob.

(Before reading this comment, please read #41.)

1. #48 explains/confirms that we can in fact make context-aware blocks opt out.
2. Drupal core only supports SingleFlush, and may support BigPipe (see #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts). Drupal core can only support those two render strategies/HTML delivery mechanisms, because they both don't require additional infrastructure. ESI does require additional infrastructure.
3. As #41's section C explains, in both of those cases (SingleFlush & BigPipe), any and all rendering does happen in the original request context. Which means that Contexts work just fine.
4. Therefore, there is in fact no need for Drupal 8 core to prevent Context-aware blocks from being placeholdered. That will only be necessary when ESI is used — which will happen on a tiny minority of Drupal sites. Therefore we can put the responsibility on the contrib ESI module to prevent Context-aware blocks from being placeholdered, by letting its hook_block_build_alter() set '#create_placeholder' => FALSE.
5. The expanded test coverage in this patch already asserts that hook_block_build_alter() implementations can set or override #create_placeholder (but currently it only tests the TRUE case, for absolute peace of mind it should also test the FALSE case; done in this reroll).

Conclusion: this patch is ready; it doesn't need to do any further work to deal with Contexts/Context-aware blocks. Placeholdering of Context-aware blocks works fine for >99% of sites (no ESI), and for the remaining <1% (ESI) there is test coverage to ensure that the contrib ESI module can prevent Context-aware blocks from being placeholdered. The ability to serialize and unserialize Contexts is therefore only necessary for this <1% of sites, and can easily be an optional interface added in Drupal 8.x.0.

Let's explain why BigPipe is sufficient, and ESI is not a great fit for most sites anyway.

BigPipe will be sufficient for 99%, because ESI will only be actually useful on pretty advanced infrastructure setups: the setup must cache the ESI fragments in the ESI client (Varnish or something else), otherwise they'll end up bootstrapping Drupal for every ESI request. Not to mention that Varnish (and other, but not all) ESI clients in fact perform ESI requests serially, not in parallel. So your page that is cached in the ESI client and which just needs N blocks rendered in Drupal is then waiting for N Drupal bootstraps plus rendering, plus network overhead/latency.

That's the beauty of BigPipe: a single bootstrap, no network involved, no additional infrastructure required.

For profiling, see #40. :)

YAY!

See you next in #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags.