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 onBlockPluginInterface
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.
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff.txt | 8.53 KB | dawehner |
#53 | adressable_blocks-2028749-53.patch | 65.81 KB | dawehner |
Comments
Comment #1
sdboyer CreditAttribution: sdboyer commentedadditional 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.Comment #2
Crell CreditAttribution: Crell commentedTagging.
Comment #4
Crell CreditAttribution: Crell commentedSo, is this something we're going to get back to? Anyone else want to pick it up? It's kinda important. :-)
Comment #5
katbailey CreditAttribution: katbailey commentedYep, 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.
Comment #6
sdboyer CreditAttribution: sdboyer commentedfinally 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:
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 returnsResponse::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 subclassesResponse
. failure to do so will cause an exception.a subclass of
Response
being the hard requirement, we basically have two options:Response
subclasses.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 theblock_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 callsResponse::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:HtmlFragment
from the block controller to a dumb Response and sending it back up.HtmlFragment
into the correct HTML...or whatever...drops that in withResponse::setContent()
, meaning that ultimately be sent back to the browser.there's a bunch to be rooted through there, but i think this is a good starting point.
Comment #7
Crell CreditAttribution: Crell commentedAddendum 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.
Comment #8
sdboyer CreditAttribution: sdboyer commentedyep, 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 byBlockRenderController
and returning anHtmlFragment
. or, the definition of it that i have back in princess.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...
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.
Comment #9
Crell CreditAttribution: Crell commentedI was thinking more something like this (totally made up code):
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...
Comment #10
katbailey CreditAttribution: katbailey commentedOK, 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.
^^ 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.
Comment #11
Crell CreditAttribution: Crell commentedMy knee-jerk there is something along the lines of:
That may or may not make any sense in practice; I've not looked at the patch itself yet.
Comment #12
sdboyer CreditAttribution: sdboyer commentedi 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.
Comment #13
katbailey CreditAttribution: katbailey commentedActually, 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.
Comment #14
sdboyer CreditAttribution: sdboyer commentedohhhh - 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.Comment #15
katbailey CreditAttribution: katbailey commentedYeah, 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.
Comment #16
katbailey CreditAttribution: katbailey commentedDerp. New d.o is hard.
Comment #17
Crell CreditAttribution: Crell commentedI think this function is slated for removal in #1959574: Remove the deprecated Drupal 7 Ajax API anyway.
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.
Should be injected.
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...
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.
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.
I'm pretty sure this is injectable by now.
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.
If drupal_merge_attached() is a simple enough function, can it just be duplicated as a protected method here to eliminate the global dependency?
Comment #18
katbailey CreditAttribution: katbailey commentedThanks 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! :)
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 ;)
Comment #19
katbailey CreditAttribution: katbailey commentedI 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.
Comment #21
dawehnerJust another rerole.
Comment #23
dawehnerPotentially a lot of fixes.
Comment #25
katbailey CreditAttribution: katbailey commentedComment #26
dawehnerRerolled.
Comment #28
dawehnerAll 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).
Comment #29
dawehnerhere is the interdiff
Comment #31
dawehnerMaybe this works.
Comment #33
dawehnerPhp 5.3, come on!
Comment #35
dawehnerThis was a fun one, and should fix all the ajax related issues.
Comment #37
dawehnerAssign to myself, just to be sure.
Comment #38
dawehnerFixed 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.
Comment #39
dawehner.
Comment #43
dawehnerThis could be green
Comment #44
tim.plunkettThe actual changes here seem sane. I might have to look again, but I was too distracted by nitpicky things.
Missing docblock
Both of these should be marked (optional), and specify the default values.
Missing docblock
Does this return something?
No extra line needed
Missing docblock
Ewwwwwww
Docblock
@todo!
Instead of the inline @var, just fix the @return on block_list()
docblock
YAY
Missing oneline doc
Missing line at the top, extra after namespace, missing at end of file
Should be FQCN
Could be oneline
doooccccblockkk
docblock
Comment #45
sdboyer CreditAttribution: sdboyer commentedtalked 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.
Comment #46
sdboyer CreditAttribution: sdboyer commentedactually, 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.
interesting. well, if we've already inlined block_page_build(), then maybe we CAN get rid of hook_page_build()/alter() in this patch.
obv agree with tim.plunkett, this is terrible. any way we can inject?
do we actually need this? this whole description feels like exactly the sort of fuzziness that i'd like to get rid of.
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 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.
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.
Comment #47
andypostAnd 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
Comment #48
dawehnerSadly noone had a look at #2167635: Move the theme initialization into its own service. at all :(
Good idea!
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 just hope this is okay now.
We can research how to get rid of this hack in a followup.
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.
Comment #50
yched CreditAttribution: yched commentedJust a note that field_page_build() is going away in #2201693: Field output supporting code should move out of field.module
Comment #51
sdboyer CreditAttribution: sdboyer commentedi am so, so very horrified by that. but clearly, yes, that means punting.
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.
Comment #52
sdboyer CreditAttribution: sdboyer commentedi'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
Comment #53
dawehnerSeriously node tests
Comment #55
dawehner53: adressable_blocks-2028749-53.patch queued for re-testing.
Comment #58
msonnabaum CreditAttribution: msonnabaum commentedForgive 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.
Why does a page renderer know how to do this? This seems out of place.
Still don't see what we gain from this. Especially when we're passing a loaded block into the attributes.
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.
Again, this does not seem like the right place to move this logic.
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.
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.
Is this a HtmlFragmentRenderer or a HtmlBodyFragmentRenderer?
Is this a fragment of a body or just the body? Naming doesn't really work.
Still unclear what noblocks is for, but if this class exists, what is the conditional logic doing in the other one?
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.
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.
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.
Comment #59
dawehnerWhatever.
Comment #60
Crell CreditAttribution: Crell commentedThat issue just got in, so...
Comment #61
mgiffordComment #62
Crell CreditAttribution: Crell commentedIn 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.
Comment #63
msonnabaum CreditAttribution: msonnabaum commentedPerhaps a new issue is warranted, but surely we can still explore addressable blocks after the removal of HtmlPage…
Comment #64
mgifford@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.
Comment #65
Wim LeersI 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 indrupal_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: .
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.
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.
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. ButPageDisplayVariant
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:
$page
render array still exists, but only aPageDisplayVariant
can manipulate it, nothing else, so the crazy, dynamic, deep page altering of the past is no morePageDisplayVariant
HtmlFragment
is returned (for blocks or main content), but just a render array, for which we have a VIEW subscriber that prepares it$page
array, you can only alter page-level attachments. In #2362987: Remove hook_page_build() and hook_page_alter() we're completely removinghook_page_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.
Comment #66
Crell CreditAttribution: Crell commentedWim: 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.
Comment #67
andypostPlease 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
Comment #68
Crell CreditAttribution: Crell commentedIsolated 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.
Comment #69
Wim LeersThanks 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?
Comment #70
Crell CreditAttribution: Crell commentedI 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.
Comment #71
Wim LeersOk, let me know if something is unclear!
Comment #72
Fabianx CreditAttribution: Fabianx commentedOkay, 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!
Comment #73
catchMarking 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.