after the WSCCI-ish call yesterday, i offered to pull out some of the relevant segments of code from princess that deal with addressable rendering. these are those pieces. hang on, this could be a bit of a rough ride, because there are key differences in how these systems are used by princess vs. core.

let's start with flow. in princess, the subrequesting process is managed by the DisplayController. it pulls up the current display's list of blocks to be rendered, gets each block ready to go (instantiate the block plugin, inject the block config, inject any contexts that are needed by the block), then passes that fully-loaded block object to another object, one that implements FragmentRendererInterface. in the patch, this is InlineBlockFragmentRenderer, specifically the render() method.

this fragment renderer is responsible for getting the content back - in princess, we represent this response as an HtmlFragment. currently, this logic forks in two - either it issues a subrequest by asking the BlockAddressGenerator for the appropriate addressed path, or it renders directly right on the spot, depending on whether or not the block has indicated that it is "addressable-safe". either way, though, it ends up being the responsibility of the DefaultBlockController to create the HtmlFragment. so, that's five-ish different systems with different scopes of responsibility, working in relative harmony to produce a subrequest response.

so in asciiart, it's something like this:

DisplayController --> FragmentRenderer --> HttpKernel --> DefaultBlockController
                                                                                \
                                                                                 \
                                                                             HtmlFragment
                                                                                 /
                                                                                /
                      DisplayController <-- FragmentRenderer <-- HttpKernel <---

most of the same rules apply to how we'd need to do this in core. we'd need:

  • some addressing routes routes. the 'standalone' one in the block.routing.yml i included should be sufficient.
  • some new config options on blocks that determine the rendering strategy to use. these may or may not have to be pushed to the interface.
  • a block controller...maybe. this is interesting, i'll come back to this.
  • a way to get MORE back than just a rendered string from the subrequest. there are options for this, as well, but it's tricky.

the function we'd target (or at least begin by targeting) is _block_get_renderable_region(). roughly speaking, i'd say that this logic would need to fork somewhat similarly to how the InlineBlockFragmentRenderer::render() logic does - either it uses a subrequest, or it doesn't. if it does, then it needs to initiate the subrequest, which it should probably do by asking the FragmentHandler (this does have issues; i'll get to that) to render. it'll need to pass along the config value that indicates the particular rendering strategy to use. it'll also need to make a choice about the controller to use. Symfony handles this with a ControllerReference concept...which maybe will work for us, maybe not.

here's where it gets a little tricky, though. entity_view() is the "controller" we rely on in the current code - that ultimately arrives at BlockRenderController. we need to make something else, though, because BlockRenderController isn't gonna cut it here as-is. the issue is a fundamental disconnect between what Symfony expects to come back out of a subrequest (a Response - and when it passes through Symfony's native FragmentHandler, that is further reduced to only the string body of the response) versus what we need (the response body, but also out-of-band information like assets and titles). this is the reason i came up with HtmlFragment in the first place - it gave us a place to piggyback data.

msonnabaum has often pointed out that HtmlFragment is not significantly different from a render array. this is true. but there are two differences that matter:

  • Symfony requires a Response be passed back from the controller targeted by the subrequest.
  • Currently, this render array is created via a monolithic method, BlockPluginInterface::build(). Which means we're currently all-or-nothing in our ability to get back a block's output, out-of-band or otherwise. note - the 'exception' is the clusterfuck we've made out of block titles where they're *sort of* gotten out of the configuration entity, which effectively makes them out of band (i'm trying to fix this in #2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues). princess has introduced methods on BlockPluginInterface at least in part to deal with these issues.

the DefaultBlockController in this patch won't help a lot, as it's designed around many of these additional methods.

what we need to do for core, i think, is cheat. that is, we need a block controller for core that takes the render array output of entity_view(), strips out-of-band info out of it, calls drupal_render(), stores the resulting string on a Response or subclass, send the response back up to the main request. THEN, upon receiving that response string back up in _block_get_renderable_regions(), we call entity_view() again, then go through and strip out the content-related stuff from the render array, leaving only the out-of-band information.

is this terrible? ohhh, yes. i'm pretty sure dante designated a special circle of hell for people who do things like this. but it fits the constraints. i think.

CommentFileSizeAuthor
#53 interdiff.txt8.53 KBdawehner
#53 adressable_blocks-2028749-53.patch65.81 KBdawehner
#48 interdiff.txt16.31 KBdawehner
#48 adressable_blocks-2028749-47.patch64.55 KBdawehner
#43 interdiff.txt6.33 KBdawehner
#43 adressable_block-2028749-43.patch59.89 KBdawehner
#38 adressable_block-2028749-38.patch56.94 KBdawehner
#35 addressable_blocks-2028749-35.patch43.72 KBdawehner
#35 interdiff.txt1.77 KBdawehner
#33 interdiff.txt1.52 KBdawehner
#33 adressable_blocks-2028749-33.patch44.26 KBdawehner
#31 interdiff.txt1.29 KBdawehner
#31 adressable_block-2028749-31.patch44.42 KBdawehner
#29 interdiff.txt18.06 KBdawehner
#28 adressable_block-2028749-28.patch44.38 KBdawehner
#26 adressable_block-2028749-26.patch36.78 KBdawehner
#23 interdiff.txt681 bytesdawehner
#23 addressable_block.patch37.13 KBdawehner
#21 addressable_block-2028749.patch37.04 KBdawehner
#19 addressable-blocks-do-not-test.patch36.93 KBkatbailey
#19 addressable-blocks-htmlpage-combined.patch154.01 KBkatbailey
#16 addressable-blocks-do-not-test.patch28.88 KBkatbailey
#10 2028749.addressable-blocks.10.patch28.62 KBkatbailey
demo-addressing.patch23.16 KBsdboyer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

additional note - i am proposing to add most of these methods that princess needs directly to BlockPluginInterface. there are a few issues:

there's also a question of meta tags - i haven't really dealt with it, but they deserve a slot on HtmlFragment as well, which means blocks should probably have something to say about them.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Active » Needs review
Issue tags: +WSCCI, +scotch, +D8 cacheability

Tagging.

Status: Needs review » Needs work

The last submitted patch, demo-addressing.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell commented

So, is this something we're going to get back to? Anyone else want to pick it up? It's kinda important. :-)

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Assigned: Unassigned » katbailey

Yep, had a call with Sam about this today, though it was cut short before I could get clarification on how I should actually proceed here - the latest plan is heavily reliant on #2068471: Normalize Controller/View-listener behavior with a Page object. Will sync up with Sam again shortly, after I get caught up on that other issue.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

finally returning to update this a bit.

first, and most important: none of this is ACTUALLY that hard, unless we try to use symfony's FragmentRenderer. it is not useful to us, period. put it out of your mind.

i think there are two two stages of things to think about here:

  1. how do we handle the addressed rendering of blocks, and reintegration of their render arrays into the main page, from within a normal request?
  2. how do we make that *also* work for external systems that are addressing those blocks?

the first is easier, and helps familiarize with the challenges at hand, so let's deal with that first, and cheat as we may.

Basic subrequesting & reintegration

so, the icky re-rendering i described in the summary is only necessary if we try to use Symfony's FragmentRenderer. that's the thing that forces the return value coming back from the subrequest to just be a string (it ultimately returns Response::getContent()). if we don't use that, then the only requirement for traversing the kernel boundary (that is, HttpKernel::handle()) for the subrequest is that the subrequest produces something that subclasses Response. failure to do so will cause an exception.

a subclass of Response being the hard requirement, we basically have two options:

let's assume the latter case, as it's simpler for now; we'll return to the former in the second half of the overall question.

to make this work, the subrequest route will need to resolve to a controller whose sole responsibility is performing a block render. at that point, we are confronted with a very similar question as the one dealt with in #2068471-97: Normalize Controller/View-listener behavior with a Page object: how much work do we do in the controller (central control) vs. leaving for VIEW event subscribers (inverted, horizontal control)? let's keep it simple to start with - we'll create our stupid Response subclass directly in the controller, attaching the render array to it, and returning back to the http kernel. that Response will now pass directly (well... RESPONSE subscribers will still get a bite at it. gotta be careful, there) back into the master request. grab the render array from that response, chuck the rest, and you're off to the races.

now, this could be done from within _block_get_renderable_region(), but i think it's probably better to simply remove the block_page_build() implementation and shift that logic entirely into a VIEW subscriber that expects to further decorate one of the objects created by #2068471: Normalize Controller/View-listener behavior with a Page object. maybe. that does fragment our page building logic to part of it happening in that hook and part of it happening in an event, but...well, we're never gonna get rid of this page array unless we start chipping away at what uses it. doing it in the event does let us at least inject the kernel through which we're subrequesting, rather than reaching out to \Drupal::service() from proceduraldom.

in all of this, it's implicit that the block representing the main content is not retrieved via a subrequest. that output is built by the main controller, and is acted upon in our VIEW subscriber. (the main content block, btw, is by far the trickiest part of this system to deal with)

Public accessibility of subrequest routes

this is where it gets more fun.

first, the obvious - real routes need to be generated for each block. subrequests bypass routing (let's assume - though that itself is maybe not ideal), so it's technically not needed in the earlier case, but it's definitely needed for an external request to directly target the addressable route.

next change: our super-dumb Response subclass is no longer good enough. we can't send one of those out from the controller on the addressable block route, because the super-dumb one isn't actually a real, functioning response: the render array sits unrendered, and the client would just get an empty string back when our index.php eventually calls Response::send().

the obvious alternative is to have the addressable block controller return a thinger from #2068471: Normalize Controller/View-listener behavior with a Page object - an HtmlFragment, most likely. which means we're now pushing the work of preparing the thing back to the VIEW subscriber, our second option from above. clearly, this will also change how we have to handle things in the subrequesting case - really, we need a rather smart VIEW subscriber. it needs to know how to differentiate the following cases:

  • A subrequest that is addressing a block. this is most like our dumb case - maybe we can still just get away with attaching the HtmlFragment from the block controller to a dumb Response and sending it back up.
  • A main request that is addressing a block - so, where the block is being directly addressed from the outside. in this case, the VIEW subscriber should turn that HtmlFragment into the correct HTML...or whatever...drops that in with Response::setContent(), meaning that ultimately be sent back to the browser.
  • A subrequest that is not addressing a block - weird case that we have no instance of in core, but here, we do nothing.
  • A main request that is not addressing a block (but is doing an HTML page) - the original case already discussed - unless we want to put this on a totally separate class for clarity's sake, this is where we need to fire subrequests in order to collect all the block output.

there's a bunch to be rooted through there, but i think this is a good starting point.

Crell’s picture

Crell CreditAttribution: Crell commented

Addendum to #6: In light of #2068471: Normalize Controller/View-listener behavior with a Page object, my intent was to have blocks always construct and return an HtmlFragment (just as a controller does). The HtmlPageRender service (I think that's what I called it) would then ask each block for its HtmlFragment and assemble them together with the Controller's HtmlFragment into an HtmlPage. (The precise method by which it does so is left undefined, specifically because that's the logic that SCOTCH/Panels TNG would replace in contrib.) The interesting part here would be, IMO, automagic handling of cache tags for the HtmlFragment objects so that we can skip the block partially or entirely as appropriate.

As Sam notes, the fun part is block-on-its-own requests. But actually, I don't think that's as tricky as Sam makes it out to be. Rather, we can subclass HtmlFragment to HtmlBodyFragment, which Controllers would return. The listener that the HtmlPage issue introduces (which calls the renderer) checks for that class instead of HtmlFragment. Other listeners can then listen for a controller result of HtmlFragment and, based on other information like the Accept header, convert that into whatever Response is appropriate. (String, Drupal Json command, SVG, morse code, etc.)

Along the way, hook_page_alter() gets removed from the renderer as there's no page array for it to even modify anymore.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

my intent was to have blocks always construct and return an HtmlFragment (just as a controller does). The HtmlPageRender service (I think that's what I called it) would then ask each block for its HtmlFragment and assemble them together with the Controller's HtmlFragment into an HtmlPage. (The precise method by which it does so is left undefined, specifically because that's the logic that SCOTCH/Panels TNG would replace in contrib.)

yep, total agreement there. the DefaultBlockController included in the patch i attached to the original issue does exactly that - it's based around supplanting the work normally done by BlockRenderController and returning an HtmlFragment. or, the definition of it that i have back in princess.

The interesting part here would be, IMO, automagic handling of cache tags for the HtmlFragment objects so that we can skip the block partially or entirely as appropriate.

it'd be nice for sure - still not as good as being able to do it without entering the block at all based entirely on injected context (creating that render array isn't necessarily cheap). but hey, context injection has languished for months. soo...

As Sam notes, the fun part is block-on-its-own requests. But actually, I don't think that's as tricky as Sam makes it out to be. Rather, we can subclass HtmlFragment to HtmlBodyFragment, which Controllers would return. The listener that the HtmlPage issue introduces (which calls the renderer) checks for that class instead of HtmlFragment. Other listeners can then listen for a controller result of HtmlFragment and, based on other information like the Accept header, convert that into whatever Response is appropriate. (String, Drupal Json command, SVG, morse code, etc.)

mm, that could work well. subclassing further would indeed help with one aspect of the disambiguation - it would clearly demarcate the "main request to non-block" and "main request to block" case.

converting the response into some other format has always seemed like a tough problem to me unless the block is providing some capability for it directly. this isn't like an entity that has view modes. and please, let's not be fooled by the block entity - its view modes cannot accomplish this, and we should continue pretending like they do not exist. that said, i really know next to nothing about how we do type conversions on Accept headers in the 'normal' case. but if it has anything to do with the intrinsic capabilities of the underlying, then there's nothing like that built for blocks.

Crell’s picture

Crell CreditAttribution: Crell commented

I was thinking more something like this (totally made up code):

$html = new HtmlFragment();
$block = get_a_block();
$renderable = $block->build();
// We have code for this in the HtmlPage issue, but I don't feel like looking it up right now.
$html->addCss(get_the_css_out_of_the_renderable($renderable));
$html->addJs(get_the_js_out_of_the_renderable($renderable));
$html->addMetatag(get_the_metas_out_of_the_renderable($renderable));
$html->setContent(render($renderable));
return $html;

HtmlPageRenderer would do that for each block to be displayed, then dissect the return value and merge it into the HtmlPage. An individually addressed block would convert the HtmlFragment directly into a response object in some fashion.

I do not know how we can generically cache that at the string level unless the assets are accessed via a separate methods rather than through build(). (That's why I've been trying to get that data out of the render array since we talked about this at BADCamp 2012. :-) )

But anyway... the important thing here is addressing the block. Kat, have at it. We'll stop talking so you can work...

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Issue summary: View changes
FileSize
2028749.addressable-blocks.10.patch28.62 KB

OK, I am throwing this up here a) to prove I am not a complete layabout and b) to ask for help with part of it. With regard to b, I have absolutely no idea what to do with the block fragments gathered up by the page renderer.

HtmlPageRenderer would [...] dissect the return value and merge it into the HtmlPage

^^ Yeah, that bit. Once we get to dealing with bits of html output, I haz no skillz. Halp.

It is all extremely rough around the edges to say the least. Patch won't apply as it was done on top of #2068471: Normalize Controller/View-listener behavior with a Page object.

Crell’s picture

Crell CreditAttribution: Crell commented

My knee-jerk there is something along the lines of:

foreach ($block_fragment->getCss() as $css) {
  $page->addCss($css);
}
// ...
foreach ($block_fragments as $id => $fragment) {
  $render[$id] = $fragment->getContent();
}
$page->setContent(drupal_render('html' , $render));

That may or may not make any sense in practice; I've not looked at the patch itself yet.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

i basically agree with #11, but no such method currently exists on blocks to collect their assets, nor is it considered an easy problem to (always) separate out the assets into a discrete call like that. (also, there's no reason to to separate css vs. js, just one call should be fine for all assets).

the method i introduce in #1762204: Introduce Assetic compatibility layer for core's internal handling of assets is BlockInterface::declareAssets(), which is a little different in that it uses a concept called a collector to do the work. it's designed to fit really easily into what's being done here, though, so when time comes, slotting it in will be easy. however, it still doesn't actually solve the problem of blocks not necessarily being able to accurately declare their assets separately.

given this constraint on asset declaration, for the moment, it's best to still just assume that they'll come in directly as part of the render array produced by the block's builder.

katbailey’s picture

katbailey CreditAttribution: katbailey commented

Actually, the assets part wasn't where I was stumped - for now I have a function that detaches the assets from the build array so we can set them as the HtmlFragment's assets property. The bit I'm most confused about is how the html will get smushed together.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

for now I have a function that detaches the assets from the build array so we can set them as the HtmlFragment's assets property

ohhhh - badass. i missed that in glancing through the patch. i've been thinking we'd need just such a thing for quite some time.

is Larry's answer sufficient re: smushing together the HTML? i'd say the initial goal should just be to roughly mimic the render array output produced by block_page_build(), but from this new context. it's been long enough since i've looked at it specifically that that might actually be a little tricky, but...seems like the right starting point.

katbailey’s picture

katbailey CreditAttribution: katbailey commented

Yeah, I think I was imagining a problem where the was none. This now outputs the blocks, and although things kinda look funny, that's not actually due to this patch - dawehner points it out over in #2068471-133: Normalize Controller/View-listener behavior with a Page object.

Anyway, still rough, but I did test it by making an ajax request for a block that had some js settings attached to it and it worked.

katbailey’s picture

katbailey CreditAttribution: katbailey commented
FileSize
addressable-blocks-do-not-test.patch28.88 KB

Derp. New d.o is hard.

Crell’s picture

Crell CreditAttribution: Crell commented 
  1. +++ b/core/includes/ajax.inc
@@ -230,72 +230,104 @@
+function ajax_render($commands = array(), $assets = array()) {

    I think this function is slated for removal in #1959574: Remove the deprecated Drupal 7 Ajax API anyway. 

  2. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php
@@ -44,6 +47,9 @@ public function render(HtmlFragment $fragment, $status_code = 200) {
+    if ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) != 'block.admin_demo') {
+      $page_array += $this->getBlocks($request);
+    }

    Making this service request aware just for that check seems very fugly. That's tightly coupling this service to a route name, and the request. Neither of those is OK, IMO.

    What is the reason for this block? Perhaps there's another way around it, like an alternate service for the blocks page. 

  3. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php
@@ -57,6 +63,51 @@ public function render(HtmlFragment $fragment, $status_code = 200) {
+        $not_cacheable = \Drupal::currentUser()->id() == 1 ||

    Should be injected. 

  4. +++ b/core/lib/Drupal/Core/Page/HtmlBodyFragment.php
@@ -14,19 +14,9 @@
+class HtmlBodyFragment extends HtmlFragment {

    I know the name HtmlBodyFragment is entirely my fault, but on further consideration I think it's a poor name. It implies "the body tag", when it very much is not the body tag. It's the special fragment for the content region. Of course, HtmlContentFragment seems dumb since "content" is such an overloaded term. HtmlMainFragment, maybe? Sam, help me bikeshed this... 

  5. +++ b/core/modules/block/lib/Drupal/block/DefaultBlockController.php
@@ -0,0 +1,26 @@
+  public function respond(BlockInterface $block) {
+    $fragment = $block->getHtmlFragment(TRUE);
+    return $fragment;
+  }

    Purdy.

    If we just return HtmlFragment, though, is there now a view subscriber that will know what to do with it? If I read this correctly the old subscriber is now listening for HtmlPageFragment (or whatever it becomes), so there's nothing that knows what to do with HtmlFragment. We should decide what to do with that... Possibly it will be Accept-dependent. 

  6. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -172,4 +175,66 @@ public static function sort($a, $b) {
+  public function getHtmlFragment($not_cacheable) {

    The caching logic in this method feels quite wrong to me, including the $not_cacheable parameter. But I'm assuming that's simply a port of existing logic.

    At the very least, can we invert the parameter to make it $cacheable rather than $not_cacheable? Although I would still much rather have a better way of handling that than a boolean param. 

  7. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -172,4 +175,66 @@ public static function sort($a, $b) {
+        $build = entity_view($this, 'block');

    I'm pretty sure this is injectable by now. 

  8. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -172,4 +175,66 @@ public static function sort($a, $b) {
+    // Add contextual links for this block; skip the main content block, since
+    // contextual links are basically output as tabs/local tasks already. Also
+    // skip the help block, since we assume that most users do not need or want
+    // to perform contextual actions on the help block, and the links needlessly
+    // draw attention on it.
+    if (!empty($build) && !in_array($this->get('plugin'), array('system_help_block', 'system_main_block'))) {

    I know it's out of scope here, but again, hard-coding that assumption here seems very wrong. The cleaner approach is probably for Blocks to identify if they support contextual links, with a default of true. 

  9. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -172,4 +175,66 @@ public static function sort($a, $b) {
+      $this->attached = drupal_merge_attached($this->attached, $element['#attached']);

    If drupal_merge_attached() is a simple enough function, can it just be duplicated as a protected method here to eliminate the global dependency?

katbailey’s picture

katbailey CreditAttribution: katbailey commented

Thanks for the review @Crell! To be sure, most of this is a horrendous mish-mash of procedural and OO code as I just copied a bunch of logic from places and stuck it in methods. I wanted to get the pieces in place and doing what they need to do before refining it to actual half-decent code. Great to have a list of points to work from though! :)

If we just return HtmlFragment, though, is there now a view subscriber that will know what to do with it?

Yep, for now I added that in the existing ViewSubscriber class.

Anyway, lots more work to do, will try and make the next patch significantly less horrendous ;)

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Status: Needs work » Needs review
FileSize
addressable-blocks-htmlpage-combined.patch154.01 KB
addressable-blocks-do-not-test.patch36.93 KB

I haven't finished addressing Crell's points in #17 but figured I should put up a fresh patch at this stage. I have added support for per-route renderers, so we can use a different renderer for the block layout demo page.

This is rolled on top of the latest htmlpage patch, so attaching a combined patch for testbot.

The last submitted patch, 19: addressable-blocks-htmlpage-combined.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
addressable_block-2028749.patch37.04 KB

Just another rerole.

Status: Needs review » Needs work

The last submitted patch, 21: addressable_block-2028749.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
addressable_block.patch37.13 KB
interdiff.txt681 bytes

Potentially a lot of fixes.

The last submitted patch, 23: addressable_block.patch, failed testing.

katbailey’s picture

katbailey CreditAttribution: katbailey commented
Assigned: katbailey » Unassigned
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
adressable_block-2028749-26.patch36.78 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 26: adressable_block-2028749-26.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
adressable_block-2028749-28.patch44.38 KB

All the failures are basically around the problematic architecture how we render the main content block, in case we have regions.
The current version of the patch ignores other blocks in that region (sadly I wasn't able to fix it properly, so i used the version with the least failures).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt18.06 KB

here is the interdiff

Status: Needs review » Needs work

The last submitted patch, 28: adressable_block-2028749-28.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
adressable_block-2028749-31.patch44.42 KB
interdiff.txt1.29 KB

Maybe this works.

Status: Needs review » Needs work

The last submitted patch, 31: adressable_block-2028749-31.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
adressable_blocks-2028749-33.patch44.26 KB
interdiff.txt1.52 KB

Php 5.3, come on!

Status: Needs review » Needs work

The last submitted patch, 33: adressable_blocks-2028749-33.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.77 KB
addressable_blocks-2028749-35.patch43.72 KB

This was a fun one, and should fix all the ajax related issues.

Status: Needs review » Needs work

The last submitted patch, 35: addressable_blocks-2028749-35.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: Unassigned » dawehner

Assign to myself, just to be sure.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: dawehner » Unassigned
FileSize
adressable_block-2028749-38.patch56.94 KB

Fixed a lot of the failures, though the block rendering in the main region is still broken, as the current hack of drupal_set_page_content cannot longer be applied.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

.

The last submitted patch, 10: 2028749.addressable-blocks.10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 38: adressable_block-2028749-38.patch, failed testing.

The last submitted patch, 38: adressable_block-2028749-38.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
adressable_block-2028749-43.patch59.89 KB
interdiff.txt6.33 KB

This could be green

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

The actual changes here seem sane. I might have to look again, but I was too distracted by nitpicky things.

  1. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -23,6 +23,16 @@ class AjaxResponse extends JsonResponse {
+  protected $needsAssets = TRUE;

    Missing docblock 

  2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -47,6 +57,74 @@ public function addCommand(CommandInterface $command, $prepend = FALSE) {
+   *   An array of assets keyed by 'js' and 'css' and the list of assets.
...
+   *   If is_ajax is TRUE some JS settings are removed automatically.

    Both of these should be marked (optional), and specify the default values. 

  3. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseRenderer.php
@@ -72,6 +72,21 @@ public function render($content) {
+  protected function createAjaxResponse() {
+    return new AjaxResponse();
+  }

    Missing docblock 

  4. +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php
@@ -28,6 +28,13 @@ public function getCacheKeys();
+   * Controls the granularity of the cache entry.
+   *
+   * A typical example is DRUPAL_CACHE_PER_ROLE.
+   */
+  public function getCacheGranularity();

    Does this return something? 

  5. +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php
@@ -51,4 +58,5 @@ public function getCacheMaxAge();
   public function isCacheable();
 
+
 }

    No extra line needed 

  6. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php
@@ -92,6 +106,15 @@ public function onHtmlPage(GetResponseForControllerResultEvent $event) {
+  protected function detectRendererFromRoute(Route $route) {

    Missing docblock 

  7. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
+    global $theme;

    Ewwwwwww 

  8. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
+  protected function buildPage(HtmlBodyFragment $fragment, $status_code = 200, $with_blocks = TRUE) {

    Docblock 

  9. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -93,4 +218,47 @@ public function preparePage(HtmlPage $page, &$page_array) {
+   * @TODO

    @todo! 

  10. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -93,4 +218,47 @@ public function preparePage(HtmlPage $page, &$page_array) {
+        /** @var \Drupal\block\BlockInterface $block */

    Instead of the inline @var, just fix the @return on block_list() 

  11. +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php
@@ -28,6 +25,8 @@ class HtmlFragment implements CacheableInterface {
+  protected $assets;

@@ -81,63 +86,23 @@ public function setContent($content) {
+  public function setAssets($assets) {
...
+  public function getAssets() {

    docblock 

  12. +++ b/core/modules/block/block.module
@@ -181,69 +163,6 @@ function block_page_build(&$page) {
-function _block_get_renderable_region($list = array()) {

    YAY 

  13. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.php
@@ -32,4 +32,12 @@
+   * @param bool $not_cacheable
+   *   Is the block not cachable.
+   *
+   * @return \Drupal\Core\Page\HtmlFragment
+   */
+  public function getHtmlFragment($not_cacheable);

    Missing oneline doc 

  14. +++ b/core/modules/block/lib/Drupal/block/DefaultBlockController.php
@@ -0,0 +1,26 @@
+<?php
+/**
+ * @file
+ * Contains Drupal\block\DefaultBlockController.
+ */
+
+namespace Drupal\block;
+
+
+/**
...
+  }
+}
\ No newline at end of file

    Missing line at the top, extra after namespace, missing at end of file 

  15. +++ b/core/modules/block/lib/Drupal/block/DefaultBlockController.php
@@ -0,0 +1,26 @@
+   * @param BlockInterface $block

    Should be FQCN 

  16. +++ b/core/modules/block/lib/Drupal/block/DefaultBlockController.php
@@ -0,0 +1,26 @@
+    $fragment = $block->getHtmlFragment(TRUE);
+    return $fragment;

    Could be oneline 

  17. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -64,6 +66,8 @@ class Block extends ConfigEntityBase implements BlockInterface {
+  protected $attached = array();

@@ -177,4 +181,78 @@ public static function sort($a, $b) {
+  public function getHtmlFragment($cacheable) {
...
+  protected function detachAssets(&$element) {

    doooccccblockkk 

  18. +++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxResponseRendererTest.php
@@ -141,4 +160,12 @@ protected function elementInfo($type) {
+  protected function createAjaxResponse() {
...
+  public function setAjaxResponse(AjaxResponse $ajax_response) {

    docblock

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented
Status: Needs review » Needs work

talked with Crell about this in IRC the other day. much though i appreciate @dawehner pushing on this, this issue is not at a point where just pushing to get it green really helps solve a problem. it's just rearranging deck chairs on the titanic. we need to resolve the question of the correct relationship between the fragments and render arrays, and operate on some kind of clear principle with it. otherwise we've just made this whole area far worse and more confusing.

i'm gonna step through this patch today, get a handle on the flow again, then post more thoughts. sorry to parade-rain.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

actually, not as problematic as i thought. i'm not the biggest fan of how we've composed some of these interfaces, but they're still improvements. the only issue i would consider actually a blocker on this is the last one, re: subrequests...as that was the whole original goal of this issue, and without it we have not done ANYTHING like addressable block rendering.

  1. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
+  protected function getBlocks() {

    interesting. well, if we've already inlined block_page_build(), then maybe we CAN get rid of hook_page_build()/alter() in this patch. 

  2. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
+    global $theme;

    obv agree with tim.plunkett, this is terrible. any way we can inject? 

  3. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
+    // Once we've finished attaching all blocks to the page, clear the static
+    // cache to allow modules to alter the block list differently in different
+    // contexts. For example, any code that triggers hook_page_build() more
+    // than once in the same page request may need to alter the block list
+    // differently each time, so that only certain parts of the page are
+    // actually built. We do not clear the cache any earlier than this, though,
+    // because it is used each time block_get_blocks_by_region() gets called
+    // above.
+    drupal_static_reset('block_list');

    do we actually need this? this whole description feels like exactly the sort of fuzziness that i'd like to get rid of. 

  4. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
+        drupal_set_page_content($page_content);

    why is this global still needed? there are things that still call it, but this patch should be able to obviate the need for this handshake-through-a-global-function - we should be able to encapsulate it all in this class. 

  5. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -18,35 +21,157 @@ class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {
     $page_array = drupal_prepare_page($page_content);

    we should not use this, at all. while actually getting rid of hook_page_build() and hook_page_alter() are probably for the next patch, we should at least inline this function as a step towards recognizing that it needs to be gotten rid of, and that this is the only place in which we should *actually* be "preparing" a page. 

  6. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
@@ -93,4 +218,47 @@ public function preparePage(HtmlPage $page, &$page_array) {
+  protected function getBlocksByRegion($region) {
+    // Assign blocks to region.
+    $content = array();
+    if ($list = block_list($region)) {
+      $empty = TRUE;
+      foreach ($list as $key => $block) {
+        /** @var \Drupal\block\BlockInterface $block */
+        $fragment = $block->getHtmlFragment($this->isBlockCachingEnabled());
+        if ($block_content = $fragment->getContent()) {
+          $content[$key] = array(
+            '#markup' => $block_content,
+          );
+          $empty = FALSE;
+        }
+        if ($tags = $fragment->getCacheTags()) {
+          $content[$key]['#cache']['tags'] = $tags;
+        }
+        if ($keys = $fragment->getCacheKeys()) {
+          $content[$key]['#cache']['keys'] = $keys;
+        }
+        if ($granularity = $fragment->getCacheGranularity()) {
+          $content[$key]['#cache']['granularity'] = $granularity;
+        }
+        if ($bin = $fragment->getCacheBin()) {
+          $content[$key]['#cache']['bin'] = $bin;
+        }
+        if ($max_age = $fragment->getCacheMaxAge()) {
+          $content[$key]['#cache']['cache_max_age'] = $max_age;
+        }
+      }
+
+      if (!$empty) {
+        // Add a theme wrapper that allows them to be consistently themed.
+        $content['#theme_wrappers'][] = 'region';
+        $content['#region'] = $region;
+      }
+    }
+    return $content;

    this is where the subrequests should be built and sent, ultimately arriving in the same sort of handling on the other side of the subrequest. that was the entire original goal of this issue.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

And here's still issue with forms that placed via block plugin (login at least) that core require to build a whole page to execute submit in some block form... main block content is only one bound to called route.
So having a block addressable we will be able to properly pass submit to needed block without building others

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
adressable_blocks-2028749-47.patch64.55 KB
interdiff.txt16.31 KB

Ewwwwwww

Sadly noone had a look at #2167635: Move the theme initialization into its own service. at all :(

Instead of the inline @var, just fix the @return on block_list()

Good idea!

interesting. well, if we've already inlined block_page_build(), then maybe we CAN get rid of hook_page_build()/alter() in this patch.

The following modules still rely on on the hook: book (for the demo page), contextual, dblog, edit, field, filter, system, toolbar, update, user. Most of them want to attach some assets in there.

do we actually need this? this whole description feels like exactly the sort of fuzziness that i'd like to get rid of.

I just hope this is okay now.

why is this global still needed? there are things that still call it, but this patch should be able to obviate the need for this handshake-through-a-global-function - we should be able to encapsulate it all in this class.

We can research how to get rid of this hack in a followup.

we should not use this, at all. while actually getting rid of hook_page_build() and hook_page_alter() are probably for the next patch, we should at least inline this function as a step towards recognizing that it needs to be gotten rid of, and that this is the only place in which we should *actually* be "preparing" a page.

Right, things are always simple. Sadly we have other places in which we call this method. I doubt you want to add that to the html fragment render interface. On the longrun we should get rid of the other users
of which one is batch! fun!

#2028749-6: Explore addressable block rendering should be merged into the issue summary.

At least blocks are rendered ... and login works.

Status: Needs review » Needs work

The last submitted patch, 48: adressable_blocks-2028749-47.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented

Just a note that field_page_build() is going away in #2201693: Field output supporting code should move out of field.module

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

The following modules still rely on on the hook: book (for the demo page), contextual, dblog, edit, field, filter, system, toolbar, update, user. Most of them want to attach some assets in there.

i am so, so very horrified by that. but clearly, yes, that means punting.

Right, things are always simple. Sadly we have other places in which we call this method. I doubt you want to add that to the html fragment render interface. On the longrun we should get rid of the other users
of which one is batch! fun!

i guess we can delay it. i'm sure we'll get to fixing the stuff that's actually broken just this side of never.
also, lemme note that i *think* the asset-related methods you've added should be fine and easy to integrate with #1762204: Introduce Assetic compatibility layer for core's internal handling of assets.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented 
+++ b/core/modules/block/block.routing.yml
@@ -56,8 +56,9 @@ block.category_autocomplete:
+  path: '/block-view/{block}/{caching}'

i've clearly missed something crucial - why do we need to encode the caching option as part of the URL? isn't there a different aspect of state from which we should be able to get this? putting it in the URL seems wrong - caching shouldn't be controllable externally by a formal parameter.

let's clarify this in IRC

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
adressable_blocks-2028749-53.patch65.81 KB
interdiff.txt8.53 KB

Seriously node tests

Status: Needs review » Needs work

The last submitted patch, 53: adressable_blocks-2028749-53.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

53: adressable_blocks-2028749-53.patch queued for re-testing.

The last submitted patch, 53: adressable_blocks-2028749-53.patch, failed testing.

The last submitted patch, 53: adressable_blocks-2028749-53.patch, failed testing.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Forgive me if some of these questions are naive, I admit I haven't read the entire issue, I'm just trying to make sense of the code for now.


class DefaultHtmlFragmentRenderer implements HtmlFragmentRendererInterface {

  /**
   * Gathers all blocks to be added to the page array.
   *
   * @return array
   *   An array of block arrays keyed by page region.
   */
  protected function getBlocks() {
    $content = array();
    // @TODO Replace with the service coming from https://drupal.org/node/2167635
    global $theme;

    // The theme system might not yet be initialized. We need $theme.
    drupal_theme_initialize();

    // Fetch a list of regions for the current theme.
    $all_regions = system_region_list($theme);

    // Load all region content assigned via blocks.
    foreach (array_keys($all_regions) as $region) {
      $content[$region] = $this->getBlocksByRegion($region);
    }

Why does a page renderer know how to do this? This seems out of place.

        try {
          /** @var \Drupal\Core\Page\HtmlFragmentResponse $response */
          // @todo: Maybe we want to create a full subrequest instead of the forward
          // call, but we need to setup the request like forward does.
          $controller = 'Drupal\block\DefaultBlockController::respond';
          $attributes = $this->request->attributes->all();
          $attributes['block'] = $block;

          $response = $this->httpKernel->forward($controller, $attributes, $this->request->query->all());
          // Ensure that the subrequest returned what we expected.
          if ($response->isOk() && $response instanceof HtmlFragmentResponse) {
            $fragment = $response->getHtmlFragment();
          }
        }

Still don't see what we gain from this. Especially when we're passing a loaded block into the attributes.


    $blocks = array();
    if ($with_blocks) {
      // Set the current page content element, so that drupal_prepare_page
      // and the getBlocks() don't conflict.
      if (is_string($page_content) || (is_array($page_content) && (!isset($page_content['#type']) || ($page_content['#type'] != 'page')))) {
        drupal_set_page_content($page_content);
      }
      $blocks = $this->getBlocks();
    }
    $page_array = drupal_prepare_page($page_content);
    // Merge in the rendered blocks.
    $page_array = NestedArray::mergeDeep($page_array, $blocks);

What is the case where $with_blocks is false? If this is something that we need to support, it should at least be moved into a method so we can just assign $blocks to the result of that method.


 protected function getBlocksByRegion($region) {
    // Assign blocks to region.
    $content = array();
    if ($list = block_list($region)) {
      $empty = TRUE;
      foreach ($list as $key => $block) {
        $fragment = NULL;

Again, this does not seem like the right place to move this logic.

        if ($tags = $fragment->getCacheTags()) {
          $content[$key]['#cache']['tags'] = $tags;
         }
        if ($keys = $fragment->getCacheKeys()) {
          $content[$key]['#cache']['keys'] = $keys;
        }
        if ($granularity = $fragment->getCacheGranularity()) {
          $content[$key]['#cache']['granularity'] = $granularity;
        }
        if ($bin = $fragment->getCacheBin()) {
          $content[$key]['#cache']['bin'] = $bin;
        }
        if ($max_age = $fragment->getCacheMaxAge()) {
          $content[$key]['#cache']['cache_max_age'] = $max_age;
        }

All of those methods should return sensible defaults. Let's not put all these guards around them. In #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags I moved it to block's entity view builder, which is awkward for other reasons, but it at least consolidated all block-2-render array logic in one place.


class HtmlFragment implements CacheableInterface {

  /**
   * Sets the used assets of the fragment.
   *
   * @param mixed $assets
   *   The asset information.
   */
  public function setAssets($assets) {
    $this->assets = $assets;
  }

  /**
   * Gets the used assets of the fragment.
   *
   * @return mixed
   */
  public function getAssets() {
    return $this->assets;
  }

This should be done with an additional interface, similar to CacheableInterface, with more granular methods. Requiring a structured "asset" array is knowledge of render api leaking into this class. The $cache_info argument has the same problem, but I realize that got in elsewhere.

+++ b/core/lib/Drupal/Core/Page/HtmlFragmentRendererInterface.php
-  public function render(HtmlFragment $fragment, $status_code = 200);
+  public function render(HtmlBodyFragment $fragment, $status_code = 200);

Is this a HtmlFragmentRenderer or a HtmlBodyFragmentRenderer?

class HtmlBodyFragment extends HtmlFragment {

Is this a fragment of a body or just the body? Naming doesn't really work.

class HtmlFragmentRendererNoBlocks extends DefaultHtmlFragmentRenderer {

Still unclear what noblocks is for, but if this class exists, what is the conditional logic doing in the other one?

-class HtmlPage extends HtmlFragment {
+class HtmlPage extends HtmlBodyFragment {

Is this a page or a body? The methods suggest this is just an HtmlBody class, but the distinction between the two is very confusing. It also doesn't really make sense for an HtmlBody to have a response code.

--- a/core/modules/block/lib/Drupal/block/BlockInterface.php
+++ b/core/modules/block/lib/Drupal/block/BlockInterface.php
@@ -32,4 +32,14 @@
    */
   public function getPlugin();
 
+  /**
+   * Returns the block as HTML fragment.
+   *
+   * @param bool $not_cacheable
+   *   Is the block not cachable.
+   *
+   * @return \Drupal\Core\Page\HtmlFragment
+   */
+  public function getHtmlFragment($not_cacheable);
+

This does not feel like the right place for this logic. It's certainly confusing considering that blocks don't have an identity outside of plugins and entities, but I think we should avoid putting anything on the entity that isn't directly related to it's job as a map of settings that get saved to a yaml file.

Again, in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags I moved much of this to BlockViewBuilder, which is awkward because it's on the entity side, but it already has logic to convert a block to a render array, so that logic should stay there as long as we're using the entity system to render blocks.


class HtmlViewSubscriber extends ContainerAware implements EventSubscriberInterface {
  public function onHtmlFragment(GetResponseForControllerResultEvent $event) {
    $fragment = $event->getControllerResult();
    if ($fragment instanceof HtmlBodyFragment && !$fragment instanceof HtmlPage) {
      if (!$this->pageRenderer) {
        $request = $event->getRequest();
        $this->detectRendererFromRoute($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT));
      }
      $page = $this->fragmentRenderer->render($fragment, 200, $event->getRequest());
      $event->setControllerResult($page);
      return;
    }

    // If the subrequest returns an HtmlFragment we need to convert it to some
    // portable response object which can hold that object.
    if ($event->getRequestType() == HttpKernelInterface::SUB_REQUEST && $fragment instanceof HtmlFragment) {
      $event->setResponse(new HtmlFragmentResponse($fragment));
    }
  }

}

Not sure if we got this from symfony or what, but it's really confusing to set a controller result in one case and then a response in the other. Also, is it a mistake that it checks for $this->pageRenderer (which looks required in the constructor), but then goes on to use $this->fragmentRenderer? I'm having a very difficult time making sense of this code.

Overall, this feels far from where we want to be. The naming issues around Html* are suggesting to me that this isn't modeling our domain well as is. Also, the special casing around when we're working with a page, fragment, body, response, etc, make the sub-request abstraction a hard sell.

My preference would be for this issue to wait on #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags since the scope of that one is clearer and we can figure out block-to-render-array handling in one place.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Postponed

My preference would be for this issue to wait on #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags since the scope of that one is clearer and we can figure out block-to-render-array handling in one place.

Whatever.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Postponed » Needs review

That issue just got in, so...

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Related issues: +#1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]
Crell’s picture

Crell CreditAttribution: Crell commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed

In light of #2352155: Remove HtmlFragment/HtmlPage, I don't think the work here is going to be viable. Let's revisit this post-release and see what happens.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Postponed » Needs work

Perhaps a new issue is warranted, but surely we can still explore addressable blocks after the removal of HtmlPage…

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs work » Postponed

@xjm told me that all items bumped to 8.1 are being marked postponed for now.

@msonnabaum - Did you want this to be brought in 8.0? It's marked as a task, not a feature... Not sure if that's reasonable.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

I have re-read this entire issue. And in trying to formulate a complete answer, what follows is more rambling than I'd like. Apologies.

Overall, I think the goal was flawed. I think we can summarize the goal as "render blocks in subrequests".

Note that I write "render" (which in D8 means "render array to HTML string"), not "build" (which in D8 means "generate a render array"), because in order to be able to use subrequests to … build/render blocks, we need to know the attached assets, and to know the attached assets, we must render (as in drupal_render()) the blocks. After all, blocks may be constructing their actual contents in #pre_render callbacks, and based on some conditional logic, some assets may be attached and others may not be. In other words: to know which assets are associated, we must render (as in drupal_render()) the blocks.
Consequently, something like BlockPluginInterface::declareAssets() as suggested in #12 is an impossibility.

The IS presents addressable block rendering and "blocks built/rendered in subrequests" as a universally-understood-to-be-better mechanism. It doesn't even bother justifying why we do this. This probably made sense at the time, but looking back on things, this seems bizarre. Is that just me?

I think we were all convinced that it'd be great if D8 would have ESI support, because that'd allow us to make Drupal 8 much faster for complex, authenticated (personalized) pages.

It almost seems like we started moving in this direction as soon as http://symfony.com/blog/new-in-symfony-2-2-the-new-fragment-sub-framework was published. That blog post announces Symfony 2.2, with one major change: a refactoring of the subrequests management in Symfony. As of Symfony 2.2, you can transparently choose to render fragments using multiple strategies. No code changes necessary in the code that generates the fragments. Sounds nice in theory, right?
Well, Symfony does one crucial thing very differently from Drupal, and that one thing is asset (CSS/JS) management. AFAIK, Symfony is designed to work with one static set of assets. It is not designed for sites where the set of assets to be loaded varies from page to page. That's a fine choice. It's simpler. But it's not the choice Drupal has made (a long time ago, Drupal 4.7 or before). Drupal chooses to load only the assets that are actually needed on the current page. And if parts (blocks) of your page are rendered (partial page rendering using blocks) through subrequests, ESI, SSI or HInclude… then the assets associated with those blocks need to be loaded. Otherwise the CSS (styling) or JS (interactivity) might be missing. For sub-requests, we can make this work. But ESI and SSI are — AFAICT — explicitly not designed to support this. They're designed to only operate on markup. For HInclude, there's no need to doubt, it's explicitly mentioned: You can include any HTML markup that would be valid in context..
Now, of course, you might argue that it is okay for us to just use <script> tags in rendered blocks, but it's not okay to put <link rel=stylesheet> in there; they belong in <head> and nowhere else (see the spec).

So then the only way to make this work is by requiring JS when using ESI (it's used on most sites anyway, so that's ok), and having each block declare which assets it needs, and having JS load those assets plus its dependencies. But Drupal 8 doesn't support JS loading assets directly, so you'd need another round trip to Drupal first.

If we'd have Shadow DOM… it'd be a different story.

The fact that this wasn't discussed at all, or perhaps handwaved, suggests that this issue wasn't fully thought through. Well, the name says "explore" so I guess that was the point.

  • What is the point of using subrequests? It seems to me like there are two key reasons:
    1. the ability to easily implement something like BigPipe or Ajax Blocks in Drupal 8
    2. using more of Symfony's abstractions… either for the sake of using more Symfony abstractions or because some feel like they make things more understandable

    Not a single time during this issue was the performance aspect even mentioned. Rendering blocks using subrequests comes with an inherent cost. Especially when there are typically dozens of blocks that need to be evaluated (access checking) and rendered on each page.

  • What is the point of addressable block (rendering)?
    AFAICT it is to implement something like BigPipe or Ajax Blocks with ease in Drupal 8. (And to use subrequests/ESI/SSI/HInclude with ease.)
    But what that really points to, AFAICT, is a problem that is now an artefact, a problem that no longer exists in Drupal 8. The problem in D7 was that it was a relative pain to render a block. But in D8, one could argue that a block's address is defined by a block config entity (i.e. "a placed block"). Once you know the Block's config entity, it's trivial to render the block: load the block config entity and pass it into BlockViewBuilder::view(). That's it. So if it's actually trivial to implement addressable block rendering, then why should Drupal core provide it if it doesn't use it itself?
    And even if Drupal core would use this (let's assume for a moment there wouldn't be a performance regression when performing dozens of subrequests per page load), then how would direct (external, non-sub) requests be rendered? How can you meaningfully render an individual block in a generic way? Until we have something like Shadow DOM, I can't see a format that would universal sense. If it were just HTML, it'd be simple. But because we also need to bubble up assets from a block to the entire HTML document, I don't see how that could work? And ideally, we'd return the exact same response when doing a direct (external, non-sub) request or a subrequest, otherwise we'd have double the number of cache items. If so, then cache tags also need to be bubbled (because when doing subrequests, the client gets a response with the final HTML, so the blocks' cache tags must be included also in the X-Drupal-Cache-Tags header). Which is also not addressed at all anywhere in this issue.
    Sure, we could argue that rendering blocks into formats other than HTML/Drupal AJAX would be useful… but would it really be? AFAICT it would be some kind of poor man's REST API: what's the point of getting a JSON representation of a "Who's online" block or a Menu block? Wouldn't that be better done through a REST API? Blocks are inherently tied to building HTML pages. They're islands of information potentially related to the main content, but always inherently designed to become HTML. If you want semantical data related to the main content, you'd want to use a REST API. So I'm left wondering whether there really is any value to having addressable blocks in the first place? Besides ESI, of course, but I've already explained how supporting ESI in Drupal 8 is almost a fool's errand anyway. Hence I think wondering what's the remaining value of addressable block rendering is a valid question.
    In HEAD, if you want to build a different page variant, that decorates/renders the main content in a different way than Block module does, then implement a different PageDisplayVariant. That code would still be able to use "placed block" config entities and render them in its own way if it wanted to. But PageDisplayVariant is inherently tied to HTML responses (and the HTML main content renderer) for a reason! Page displays are about displaying pages — HTML pages!

Overall, what is the goal of this issue? It seems like at the very beginning, there was focus, but that was lost over time? Many of the problems described in the comments in this issue outline goals that are not mentioned in the IS at all. And in fact, most of those goals have been solved by other issues since, or by #2352155: Remove HtmlFragment/HtmlPage.

Things already solved:

  • #6: i think it's probably better to simply remove the block_page_build() implementation — now gone
  • #6: we're never gonna get rid of this page array unless we start chipping away at what uses it — the $page render array still exists, but only a PageDisplayVariant can manipulate it, nothing else, so the crazy, dynamic, deep page altering of the past is no more
  • #6: in all of this, it's implicit that the block representing the main content is not retrieved via a subrequest. that output is built by the main controller, and is acted upon in our VIEW subscriber. — is still the same, but now different page display variants can choose to deal with "main content" in different ways, also see PageDisplayVariant
  • #6: next change: our super-dumb Response subclass is no longer good enough […] the obvious alternative is to have the addressable block controller return a […] HtmlFragment, […] which means we're now pushing the work of preparing the thing back to the VIEW subscriber — this is exactly how HEAD now works, with only one difference: no HtmlFragment is returned (for blocks or main content), but just a render array, for which we have a VIEW subscriber that prepares it
  • #7: Along the way, hook_page_alter() gets removed — done: as of #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter(), you can no longer alter the $page array, you can only alter page-level attachments. In #2362987: Remove hook_page_build() and hook_page_alter() we're completely removing hook_page_alter().
  • #46: [talking about drupal_set_content()] why is this global still needed? — it's now gone :)
  • #46: [talking about drupal_prepare_page()] we should not use this, at all. […] we should at least inline this function as a step towards recognizing that it needs to be gotten rid of — we got rid of it :)
  • #48: The following modules still rely on on the hook: book (for the demo page), contextual, dblog, edit, field, filter, system, toolbar, update, user. Most of them want to attach some assets in there. — fixed by #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter()

So much what was discussed here, has already been implemented! :)

In conclusion, I think we should close this issue in favor of a new one with the exact same title. Much of what was discussed here was not focused on the main task at hand: addressable blocks. Probably because Drupal 8 was still so much in flux. Probably because it depended on many other related issues. Much of the discussed/proposed implementation has already been implemented. But the actual "addressable" part is not yet fixed, and still has significant hurdles to overcome. Hence my proposal to open a new issue for that.

Crell’s picture

Crell CreditAttribution: Crell commented

Wim: Two points of background that I think are relevant:

1) Symfony's new subrequest system was added in part at our prodding, because the previous version hard-coded support for 3 different mechanisms only. However, you're correct that it only supported strings, not strings-and-assets, as you mention. That's why we, around that time or shortly after, abandoned the idea of using Symfony subrequests directly. The terminology stuck around for a while, though, for better or worse.

2) Attached assets were not simply ignored; I think you're misunderstanding the use case. The CSS that the Whose Online block (for instance) needs is, realistically, not going to change between requests although its HTML may. Therefore when rendering the whole page we DO want its CSS/JS, which would then be included in the whole page along with its ESI tag. Then Varnish would call back to the server and get just the markup of that block (as it already has its CSS), inject that, and be done. Subsequent requests would just do the second part, render the Whose Online block's markup (remember, it already has the CSS), inject it into the cached page again, and we're done.

If we cannot rely on the CSS/JS of a block not changing between subsequent requests, then that block is un-ESI-able. (I would argue it's also stupidly written, but that's beside the point.)

The use case of serializing the markup *and* assets together in some JSON envelope or something would be for other non-ESI use cases, which would involve client-side Javascript anyway. (Eg, Facebook BigPipe-style, or a lazy tabbed setup, or...) Those would require a separate controller (via accept header or not), which is fine; If we enable isolated block rendering then we enable that experimentation. If we don't, we can't.

All that said, I think given how far core has drifted since this was last touched that a new issue that references back to this one for context is probably wise.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: +Needs issue summary update

If we enable isolated block rendering then we enable that experimentation

Please point what "isolation" means?
- we can't render block in isolation of current theme and enabled modules.
- blocks can use forms inside so more assets and data can be dependency for block rendering

Crell’s picture

Crell CreditAttribution: Crell commented

Isolated from the rest of the page. Ie, If a page has 5 blocks we conceptually want them all to be renderable in different PHP processes.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Thanks for the extra background info Crell :)

Crell, I think you're on of the most qualified people to open a new issue that frames addressable block rendering in the current D8 reality. Would you be willing to do so?

Crell’s picture

Crell CreditAttribution: Crell commented

I need to get back up to speed on the post-fragment rendering world first. I mostly sat out that issue so I have to get myself back up to speed.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Ok, let me know if something is unclear!

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

Okay, so technically isolated block rendering is not needed and we can do what Symfony does with subrequests with render array properties as well and using #post_render_cache.

Realistically currently blocks might depend on the main page request, but then they should be using a context or cache context to be renderable PER page, which means they would never be ESI'ed or AJAX'ed anyway.

However a block might still be different per USER and have different CSS - and that is a very realistic use case - just display a list of teasers and some of those teasers have data that needs some CSS (like flags), but others have not ...

So you need a way to render the assets, but with the to-library transition, we can use /js and /css loaders, so that will be solved in some way.

The main request might still predict libraries it needs for the ESI or AJAX block, but it can't rely on it.

Render Arrays now only contain #pre_render and #cache, which is information that is perfectly suited to push to a sub-request as it is usually isolated data enough. (Big Pipe, etc).

All we really need is a way to push #pre_render -> #post_render_cache and replace with a placeholder and for BIG data like objects in the #pre_render array we might need a serialize or create placeholder method or such to ensure the data is as minimal as possible.

Once we have that structure and such we can use #post_render_cache internally we can also push that optionally to a sub-request, because the data is suited for e.g. a POST. If we have no global state, that works well.

Another reason why the symfony fragment framework is not suitable is that we have strong cache contexts, which we need to transfer over the wire as well. e.g. for a per user cached block.

AND ESI / AJAX caching is so much more than just doing an authorized HTTP request (that is the easy part), but you want to have special expiration headers (max-age / downstream-ttl), cache contexts, cache tags, etc. on them ...

Because you want Varnish to cache the ESI, not Drupal be called again and again and you want the browser to cache the AJAX request for a moment (which he can if the user is in the URL).

My own strategy (in D7) is to use ESI/AJAX/... in knowledge of the original request, e.g. via a special http header like:

/fragment/1234-5678
X-Drupal-Uid: 1
X-Drupal-Request-Path: /node/1

where 1234-5678 is the cache id of the block without the cache contexts replaced.

( and for ajax: /fragment/1234-5678?uid=1 )

e.g. {1234-5678} -> CID: block:whos_online:@cache_context.user

So blocks _do_ have cache context agnostic addresses already.

In the fast path the cache contexts are resolved and then the item is retrieved from cache.

In the slow path, the main page request is executed by setting the path and executing the route normally, but then only retrieving _this_ fragment instead of all blocks.

OR

the data can be stored in the UUID, which is just the #pre_render, #cache part of the render array (or in #post_render_cache).

Please see:

http://wimleers.com/talk-render-caching-drupal-7-and-8/#/4/7

(start of that section is here: http://wimleers.com/talk-render-caching-drupal-7-and-8/#/4)

for a lot more information on how the big picture for ESI / AJAX / etc. needs to work.

Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Postponed » Closed (duplicate)

Marking duplicate of #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts and related auto-placeholdering/placeholder strategy issues.