as this code is still quite entirely pseudocode, i'm putting it over here rather than directly in the core queue. I'd like to get it there ASAP, but not just yet.

i've pushed up a new branch to this repo called 'booze'. it contains the first pass at putting together the panels-ish controller that connects WSCCI with Scotch. it also has the patch from #1787846-132: Themes should declare their layouts committed to it, and should have the latest from #1535868: Convert all blocks into plugins committed to it as well. b/c, yknow...that's basically blocks and layouts.

first, let me note that i've used names like 'Drunken' and 'Booze' throughout this code. i'm expecting these will change, of course, but in the absence of better, permanent names, it seemed appropriate to do since we're sort of bridging the two initiatives here.

the names do serve an important discussion purpose, too: it gives me an easy way to say "if we're serving a boozey/drunken page", which is more comfortable than "if we're using a blocks-and-layouts approach to serving the page."

there are three major components to note right now.

Displays

the Drupal\Core\Config\Entity\Display class. this class, entirely analogous to display objects from Panels, encapsulates all configuration for a given boozey route: the layout instance (so that's plugin name + conf, if any), all block instances (again, plugin name + conf) that are in use for the page, AND meta-conf that is not specific to any of these (e.g., which regions and in what order the blocks should be placed).

the Display class extends ConfigEntity. i went this route for two basic reasons: a) it's by far the easiest way to allow a specialized config class (which we need in order to provide certain data organization, abstraction, and lazy loading mechanisms) that can take over a certain config namespace prefix, and b) because ConfigEntity provides us with revisioning support. i'm far from settled on the decision to use ConfigEntity; i dislike not accessing config info through config(), and i'm not convinced that the revisioning provided will be useful for the task we need it for. there's already an issue that's roughly about this, and i just updated it with my additional thoughts on divergent config history: #1620140-5: Allow synchronizing config entities from default config when modules are updated.

the class is currently a shell with none of the methods implemented. there needs to be a lot more logic in there, and there's also some issues (can be discussed later) about ways we might want to mutate behavior by manipulating what's in the $display. most of the manipulations, however, should happen at compile-time, NOT at runtime. runtime modifications can easily destroy page predictability, break contracts suggested by the UI, and break caching. probably the biggest compile-time manipulation we'll need to make is managing globalish block injection driven by the replacement to admin/structure/blocks. that's a tricky question no matter what, and it gets into how we

i do not have a sample version of all the config i expect to be contained in one of these display objects written up yet. it's at the top of my list, as it obviously shapes what the interfaces will need to create.

i realize the term 'display' is fairly fraught. i'm open to coming up with something else, but it's tricky. Display is the term used in panels right now to describe this thing.

DrunkControllerSubscriber

this subscriber responds to KernelEvents::CONTROLLER, and is how we make the $display object available for use by the controller. its logic is shit right now, but this is the place where we make the foundational decision about what display object (and therefore what layout, blocks, etc.) is going to be used on for the current request. at the moment, the binding is very simple: it looks for "booze.display.$routename" - that means one display per route, and one route per display. this is probably not what we'll eventually do (we may not even settle on just one thing eventually), but it works for now. it's also the best answer to the basic being asked in #1787942: Allow assigning layouts to pages.

there's a small modification to the existing RouteProcessorSubscriber in order to point to the new controller. if i had just replaced HtmlPageController instead of created the new DrunkController, then that modification wouldn't have been necessary. we'll get there soon enough.

DrunkController

this is the core of the new rendering approach. what's in there right now is VERY rudimentary, but it illustrates the basic approach. roughly, that approach is:

  1. render all blocks, passing them whatever context they need (figuring out context is still a big giant gaping hole)
  2. render all regions, passing them the rendered blocks.
  3. render the whole layout, passing it the rendered regions.

there are a lot of additional switches & stuff from panels i haven't touched at all yet. styles are the most glaring thing that's missing, but caching also hasn't been touched (that's a wider issue anyway, and has to do with how we do the block rendering - subrequest or direct).

for those coming from #1787846: Themes should declare their layouts, this controller pulls out a fair bit of the logic that we had been placing directly in the Layout classes. this, i think, is a very good thing.

this sequence needs discussion. despite the fact that i just went with a simplified version of what Panels does right now, one of the things that took me the most time with this patch was pondering of the various different types of templates we might want to be able to interact with, and how to encapsulate those differences. this basic approach is good enough for Dec 1, but if we're going to deliver on the discussion we had in Munich (a generated template directory), it's gonna mean having more possibilities than just this one way.

there's plenty of other stuff, but this gets us a realistic start at bringing it all together, and hopefully provides a foundation for a bunch of other discussions.

getting this all finally working is also bound up with/blocked on at least the following, in addition to those linked above:

Files: 
CommentFileSizeAuthor
#40 booze_test.tar_.gz890 bytesfabsor
#39 controllers-1812720-39.patch35.66 KBfabsor
FAILED: [[SimpleTest]]: [MySQL] 50,509 pass(es), 275 fail(s), and 40,144 exception(s).
[ View ]
#31 interdiff.txt17.68 KBsdboyer
#31 controllers-1812720-31.patch35.88 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 51,040 pass(es), 182 fail(s), and 36 exception(s).
[ View ]
#31 booze_test.tar_.gz900 bytessdboyer
#31 ITS.TIME_.FOR_.SCOTCH.png67.1 KBsdboyer
#30 interdiff.txt33.72 KBsdboyer
#30 controllers-1812720-30.patch32.13 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 51,030 pass(es), 182 fail(s), and 42 exception(s).
[ View ]
#28 controllers-1812720_28.patch28.36 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#28 interdiff_1812720_28.patch10.3 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#25 interdiff.txt12.1 KBsdboyer
#25 controllers-1812720-25.patch18.14 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#22 boozey-1812720_22.patch6.03 KBLetharion
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 boozey-1812720.patch44.04 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

sdboyer’s picture

Assigned:Unassigned» sdboyer
StatusFileSize
new44.04 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

and, here's a patch with the latest state of the branch.

sdboyer’s picture

one other important note: while this is still pseudocode-y, the lack of drupal_render_page() is intentional. with this approach, hook_page_alter(), the topmost-level render array, and content by committee, is dead.

yched’s picture

"subscribe" :-p.

One long-standing goal with Field API is to move the display settings for entity components from $instance['display'] and the 'pseudo-fields' variables where they are currently scattered, into standalone, entity-level, per-view-mode, "display" objects - probably tying into #1026616: Implement an entity render controller . This will probably be my primary focus when I get out of afk mode early November.

Those entity-level display object (whatever name they eventually get) might or might not include notions of layouts and regions (aka panelizer / DS in code), but we'll definitely need to keep a close look on how the "page-level" displays are structured.

[edit: this is more of a FYI - I do not intend to derail this issue on non-page layouts]

sdboyer’s picture

Priority:Normal» Major

yeah, i noticed the entity render controller discussion only recently after finally reading Lin's comment on the g.d.o post i put up about the anatomy of a complex path. i need to delve into it further, but this is very much one of those things we need to resolve - if B&L is to be *the* way we do html responses (and i do believe it should be, clearly), how does that interact with concepts like different ways of viewing entities? i think there's a not-too-tough solution out there for this question, if we talk it out for a bit.

my next step here is to articulate the internals of the Display object in greater detail. i think getting that more fleshed out is probably the biggest thing that'll block other folks being able to move forward with this.

Stalski’s picture

I'll help with the display object as said before. That will be a major improvement for field api.

Crell’s picture

For rendering view modes, it should be trivially simple, I'd think. Something like:

<?php
class MyControllers extends Whatever {
  public function
showNode(Node $node, $view_mode) {
    return
$this->container->get('entity.renderer.node')->render($node, $view_mode);
  }
}
?>

(Or something along those lines.) Controllers are supposed to be really simple glue code.

sdboyer’s picture

@Crell - it's not that simple of a case.

i'm not sure whose intention it is that controllers are "simple glue code," what the criteria for "simple" is, or why it bears on what we're discussing here, it's unquestionably the case that the selection of one control vs. another is not simple. we have to write controller selection logic that allows the different layers we're building to coexist in a scalable and consistent way, and that is not a simple problem.

stalski and i chatted about the entity rendering case in irc earlier, pasting it here for reference (edited for relevance, there were like four conversations happening):

<sdboyer> stalski: so, about the display object
<stalski> yes
<sdboyer> i'm not sure how applicable it will be to field api, at least directly
<stalski> sdboyer: I have no idea either atm
<sdboyer> we did have some discussions about the multiple layers of applicability for this pattern in munich, but
<sdboyer> stalski: i'm definitely prioritizing the whole-page-level stuff first
<stalski> sdboyer: I was hoping to have a braindump with yched and swentel, and others at the field api sprint in Ghent
<sdboyer> stalski: but what sort of thing sprung to mind for you that you made the comment?
<sdboyer> ah ok
<stalski> sdboyer: agreed
<stalski> sdboyer: well, I'll try to keep it short
<sdboyer> stalski: yeah, display modes, while certainly a useful thing...well if we have a block-centric rendering model, then we can't have an entity-centric rendering model coexist next to that. but it's quite easy to nest one level in, i think
<stalski> sdboyer: it would be nice to have a display object, containing "things" (fields, properties, fieldgroups, fieldcollections) or referenced to these fields in one display object. That display object would be tied to the view modes
<sdboyer> stalski: i did respond to lin with more or less the same thought on g.d.o - http://groups.drupal.org/node/256028#comment-835848
<Druplicon> http://groups.drupal.org/node/256028 => Anatomy of a complex path => 2 comments, 4 IRC mentions
<stalski> sdboyer: the big question for me is how the fields are configured and how this fits in the display object
<sdboyer> stalski: sure, that makes a ton of sense to me - bundle up a bunch of fields & display conf together into a single object that you can reuse
<stalski> sdboyer: exactly
<sdboyer> stalski: now, i think that COULD be readily represented as an individual block
<sdboyer> stalski: but it does depend on the use case, too
<stalski> you mean how people display fields?
<sdboyer> yep
<sdboyer> e.g. "teaser" vs. "full"
<stalski> I kinda always forget the panels>fields
<sdboyer> :)
<EclipseGc> sdboyer: layout block + configuration information for field blocks
<sdboyer> stalski: i think the other approach WOULD be having a first-class display, as in, a bunch of diff blocks each containing some set of fields, arranged into regions
<sdboyer> stalski: which is basically what EclipseGc just said, yeah
<stalski> sdboyer: yes, the second is what I have in mind I think
<sdboyer> stalski: yeah ok, cool
-*- EclipseGc already did a proof of concept earlier
<sdboyer> so that's also entirely feasible
<sdboyer> and actually, it fits in really nicely within the paradigm i've set up
<sdboyer> so
<stalski> EclipseGc: sdboyer : well, how will we approach this?
<EclipseGc> and I have a layout block plugin in the layouts issue (not the code the was committed)
<sdboyer> see DrunkControllerSubscriber::onDrunkenKernelController()
<sdboyer> stalski: it seems to me that this is basically a question of WHICH display object we choose to use
<stalski> and others. Should we make one issue for it now or are there others that already claiming this behavior (I know there are a bunch that will be linked to it)
<stalski> sdboyer: in fact, yes :)
<sdboyer> stalski: i discuss this is a little bit in the issue summary i wrote, but that listener is where the decision is made about it
<sdboyer> stalski: ja - so if you look in there, you'll see that right now there's super-simple logic that just grabs a display base on the route name that's been selected. that logic can easily be made more complex to suit our needs
<stalski> sdboyer: where can I see that paradigm?
<sdboyer> stalski: sec, i'll link
<sdboyer> stalski: http://drupalcode.org/sandbox/eclipsegc/1441840.git/blob/46b25b019b656bbbcee84570a1495b36f94069b3:/core/lib/Drupal/Core/EventSubscriber/DrunkControllerSubscriber.php#l43
<sdboyer> stalski: that entity_load() is for ConfigEntity, so it's more like a call to config()
<stalski> sdboyer: trying to understand :)
<sdboyer> stalski: ok so, the sequence goes like this. first that method checks that we're using the DrunkController (which is what needs a Display object in the first place)
<EclipseGc> webchick: are not
<sdboyer> stalski: then, if we are, it reads the name of the route that's been matched. this is all happening AFTER route-matching, so there's just one route
<sdboyer> stalski: have you followed the new routing system?
<stalski> sdboyer: I used too, but I am far behind now apparently
<stalski> so that's my job for tonight/tomorrow
<stalski> sdboyer: although I understand what you just explained ;)
<sdboyer> stalski: ok, not that big a deal. wrt the new routing system, what matters here is that routes are no longer keyed on path, but on a unique name we give them
<sdboyer> stalski: cool. so $request->attributes->get('_route') returns us the name of the route that we're currently using
<stalski> sdboyer: yes, I had that. And the thing with the weird namings you are working on will be the ones to work with the display object
<sdboyer> stalski: and then it basically just makes a call to config("booze.display.$route") in order to get back the display we want
<stalski> sdboyer: well actually that's what is in comment
<sdboyer> stalski: yup yup
<sdboyer> stalski: so yeah, everything is tightly route-bound right now: one display per route, one route per display. but there's no reason it HAS to be
40 comments, 3 IRC mentions
<sdboyer> stalski: if we want to involve additional criteria in display selection, that's totally cool
<stalski> sdboyer: ok, great
<stalski> sdboyer: new try with better words: I mean, when you render a node in teaser, how is this nested inside the page display rendering?
<sdboyer> stalski: ok so the big shift is that we're no longer doing any of the runtime injection of shit, like blocks (at least that's my goal). so each $display object has to have all the info about everything to be rendered, in every region
<stalski> sdboyer: jihaa! I hoped for that to happen
<sdboyer> yay :)
<stalski> well it's the goal .. the battle is not over yet, I persume
<sdboyer> stalski: chances are, there's only a small set of stuff that's different between teaser mode and other modes, but
<cweagans> :)
<sdboyer> no, battle not over yet at all. definitely still some dragons there
<sdboyer> but at least i don't feel like there are many unknown unknowns...mostly just known unknowns. which is good :)
<stalski> sdboyer: well, I not sure I agree on that view mode thing. I mostly use very different fields/configurations on different view modes
<sdboyer> stalski: ok sure, that's me just generalizing unnecessarily
<stalski> that's why I think it's a difficult storage to design
<sdboyer> stalski: it's just that in the context of the $display object, those differences between modes are tucked in right next to something like, say, admin_menu
<sdboyer> yep, it's gonna be a bit tricky
<sdboyer> fortunately we only have to load one of them per page, ever :)
<stalski> sdboyer: it could lead to a fields being put in yamls in 10 different ways, all linked to other view modes
<sdboyer> stalski: yeah, and that's def part of my concern as well
<stalski> sdboyer: so imo, it should be considered to have a display object with the configured fields in it, and maybe not a reference to fields
<stalski> or both … who knows
<sdboyer> stalski: the only thing that display objects have references to are blocks
<sdboyer> blocks & layouts
<sdboyer> that's part of how we simplify this
<stalski> atm you mean
<stalski> yeah
<sdboyer> yes
<sdboyer> and i would oppose injecting anything else directly in there
<sdboyer> if we keep the block wrapper thin, then it's easy to have passthrus to other things, like fields
<stalski> sdboyer: true
<sdboyer> stalski: but by keeping the block wrapper layer there, we have a consistent level at which we make decisions about things like, say, caching
<stalski> sdboyer: in stead of always forgetting fields as panes, I should start thinking that way as the default ;)
<sdboyer> stalski: that is the idea :)

so right now, it sounds like when it comes to text/html 'display modes' for entities, we're talking about them defining a display object somewhere, saving that and then we have a more complex equivalent to the DrunkControllerSubscriber that can determine which of those displays should be used.

this does start making me a little itchy, though as we're starting to do something that looks a bit like routing after routing proper is done. i would like to avoid that, if possible, but we'll see what the best approach ends up being.

yched’s picture

The way I was envisioning this, entity rendering would still be triggered by some module code somewhere doing an entity_view($entity, $view_mode).

This means that there is no strict / predictable nesting relation between "page" Displays and "entity" Displays. For node/[nid], it so happens that the "main content" block is directly a "node view in 'full' mode", but basically any block could display any arbitrary entity in any arbitrary view mode, at any arbitrary place within its own HTML. So it's hard to present page layouts and entity layouts properly nested in an edit UI, for instance.
Some view modes are typically used outside of a regular "html laid-out page", BTW : rss, print, search index (the latter wouldn't need "layouts", but definitely needs a "display" in the sense of a collection of fields with their display settings)

Page-level displays are leveraged during the HTTP routing process, and trigger the rendering of a series of blocks in a layout. The code for some of those blocks happen to call entity_view($some_entity, $some_view_mode), which then leverages entity-level displays that renders a series of entity components (fields, properties...), possibly in a layout of its own.

Both displays would be edited in different locations: page displays in the new UI that is being worked on, and entity displays basically in Field UI's "Manage display" more or less as we know it - one screen per view mode.

In this scenario, there's no easy way to put a single field of your node in the "right sidebar" region of your *page* layout. The two levels just don't communicate.

yched’s picture

so imo, it should be considered to have a display object with the configured fields in it, and maybe not a reference to fields

That makes me tick quite a bit. Field and instance definitions will get their own config files, I definitely wouldn't want to duplicate those definitions in "entity display" config files, too much chances of clashes / staleness (manual edits...).
In my mind An "entity display" is a list of field names with a formatter and its settings. The actual $field and $instance structs, if needed, need to be fetched from Field API (field_info_field() or whatever this will end up being in D8).

sdboyer’s picture

In this scenario, there's no easy way to put a single field of your node in the "right sidebar" region of your *page* layout. The two levels just don't communicate.

i tend to agree with this. in that chat with stalski in #7, i presented two options:

stalski: sure, that makes a ton of sense to me - bundle up a bunch of fields & display conf together into a single object that you can reuse
stalski: now, i think that COULD be readily represented as an individual block

vs.

stalski: i think the other approach WOULD be having a first-class display, as in, a bunch of diff blocks each containing some set of fields, arranged into regions

he went for the second option, which is definitely less like what we have now (although a bit more akin to what Display Suite does). it's one possible approach, though it is not my preferred one. it makes more sense to me that we have a designated place for making page-level layout decisions, and that we not mix it with data organization & presentation considerations. that line does get blurred a lot, of course, but i think it benefits us to keep it clear. and have good UIs that are tailored to each case :)

as for the rendering of other, non-html entity view modes, i entirely agree that that really has nothing to do with HTML page generation, and therefore pretty much any of the system here. i outlined my thoughts on that in my response to Lin on g.d.o..

there is a caveat, though. with the way that people tend to compose Drupal sites - and in fact, the way we tend to consider "good" architecture, we very often interweave several entities together for the purposes of creating a particular single visual. historically, there have been a number of approaches to solving this that involve manipulating/injecting stuff into the output of some view mode on one of the entities. i think that approach is terrible: it destroys the clean separation between data and presentation, and leads to chimaera interfaces for managing the "data."

to that end, i do think we're better served by making a clear decision to reserve entity view modes for the simpler cases of single-entity viewing, or aggregate listings (views). it should be out of scope for view modes to deal with page composition.

andypost’s picture

This means that there is no strict / predictable nesting relation between "page" Displays and "entity" Displays

Actually if we could have internal route/uri for each kind of plugin (that could produce output) then I see no difference between them

sdboyer’s picture

page displays compose blocks into layouts.

entity displays compose fields/fieldgroups into...render arrays, i guess.

per my comment in #10, i believe we create significant problems for ourselves when we try to treat these like the same thing. yes, there are some basic similarities in how they function, but the types of systems they connect, and how those systems are driven, is quite different.

yched’s picture

Pretty much agreed with #10, although i'm not sure I see what the paragraph refers to.

All in all, even if that would be a cool thing, I don't really expect entity displays and page displays to share actual code. If we do things right, though, we might want to keep close-ish, maybe share some concepts and architecture, or at best some base class or interface.

An interesting point would be for layouts to be able to be used by page and entity displays indifferently. Even though in actual use cases, layouts that you'd use for entity displays would be simpler than those that you'd use for page displays, so maybe the intersection is actually close to empty. At leat the mechanism to expose entity layouts should be the same than for page layouts. But that's probably outside the scope of this specific issue.

swentel’s picture

The way I was envisioning this, entity rendering would still be triggered by some module code somewhere doing an entity_view($entity, $view_mode).

+1 I don't see any other sane option to make this possible before feature freeze. entity_view() asks for display object, which maybe has a layout and some fields. That's it. With layouts being able to be declared in core now, we can easily add this to the field ui manage display screens. How and where it's called is the main question then, at least imo. But basically, we should be able to override the default node.tpl very easily now anyway.

imo, if someone wants to move a single field from a node into a page layout, or a complete node, a different block should be available, pretty much like the entity_field.inc/node.inc plugins in ctools. It's easy and we don't kill ourselves in the process trying to find a working solution before 1 dec, because it pretty much already exists.

Given that, let's not derail this to much, let's make it work for the page level first and see if that's going to work. We already have the 'display object' issue open somewhere (forgot the link), maybe we should try and create a POC there on say entity_view($node, 'full');

sdboyer’s picture

i've pushed an update to the branch that expands on the properties to be contained on the Display object. i'll put some of the actual massaging logic in place next.

sdboyer’s picture

wfter today's discussion, we're separating a bit of this out: Displays, at the very least, are getting their own issue: #1817044: Implement Display, a type of config for use by layouts, et. all. we should be able to get that in pretty quick.

sdboyer’s picture

now that #1817044: Implement Display, a type of config for use by layouts, et. all is substantively done, i'm moving back onto this.

effulgentsia’s picture

Is there a reason this issue is in the sandbox queue? When can it be moved to the core queue?

sdboyer’s picture

it's only here because it was the first one i posted, and it was a really provisional patch at the time. now that this is all going forward, i intend to move it to the core queue after posting the first cleaned-up patch.

effulgentsia’s picture

Any update on a core queue issue for this?

sdboyer’s picture

yeah, this is next on my list to hit as soon as i get a simple responses patch up. this last week's been murder for me, but i'm shooting for the end of this weekend.

the initial goal is just to reroll to accommodate new stuff that's gone into core since it was originally put together, and move it to the core queue.

Letharion’s picture

Assigned:sdboyer» Unassigned
StatusFileSize
new6.03 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

After chatting with sdboyer, we agreed we'd start by re-rolling the patch with only the DrunkController in it, both because we can split things up into smaller pieces and because a lot has changed anyway.

Crell’s picture

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,169 @@
+        // this routes rendering through a subrequest.
+        // @todo When we have a Generator, we can replace the forward() call with
+        // a render() call, which would handle ESI and hInclude as well.  That will
+        // require an _internal route.  For examples, see:
+        // https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/routing/internal.xml
+        // https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/InternalController.php
+        $response = $this->container->get('http_kernel')->forward(array($instance, 'render'), $attributes, $request->query->all());
+        $this->renderedBlocks[$instance] = $response->getContent();

Note that _internal paths are gone in Symfony 2.2 (at least as of the last update). It's now specific to one particular rendering strategy. I thought we'd already discussed leveraging those directly here?

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,169 @@
+      $to_render = array();
+      foreach ($this->display->getBlocksByRegion($region) as $block) {
+        $to_render[] = $this->renderedBlocks[$block];

We should not make the blocks stateful. Rather, renderLayout() should be responsible for calling renderBlocks() for the appropriate blocks, which will return them without saving them to an object property. That likely means renderBlocks() gets called multiple times, once per region. That's OK.

sdboyer’s picture

Project:Drupal 8 Blocks Everywhere» Drupal core
Version:» 8.x-dev
Component:Code» base system

thanks for the prelim review.

Note that _internal paths are gone in Symfony 2.2 (at least as of the last update). It's now specific to one particular rendering strategy. I thought we'd already discussed leveraging those directly here?

yeah, that's just a direct copy-over from what's in the HtmlPageController right now. safe to disregard...and remove.

We should not make the blocks stateful. Rather, renderLayout() should be responsible for calling renderBlocks() for the appropriate blocks, which will return them without saving them to an object property. That likely means renderBlocks() gets called multiple times, once per region. That's OK.

it's a nice goal to shoot for, but we may or may not be able to live up to it. blocks, as part of their rendering process, may mutate some wider state that other blocks are also dependent upon. one case, off the top of my head: a block that contains messages (like, drupal_set_message()) will need to be rendered after pretty much everything else. the case still obtains even if we get rid of the global function.

really, this just reflects the reality that "source" order does not necessarily correspond with render order.

also...moving this into the core queue. it's bobsled time.

sdboyer’s picture

StatusFileSize
new18.14 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new12.1 KB

new patch, moving in the direction of getting the overall system working by starting at the beginning: installation-time. i've started in on a UnboundDisplayStorageController to manage the install-time process of making a configured display object actually usable. to that end, i've also written a (tentative) conversion of Bartik's page.tpl.php over to a Layout, so that it can be used. hunkered down on that one for a while, and answered a lot of knotty questions (though hardly all...).

tomorrow i'm probably gonna end up skipping the unbound stuff and just tightly couple for now by shipping plain bound display configs to start with.

also, i'm now tracking work on this in the SCOTCH sandbox on the 1812720-controllers branch.

Crell’s picture

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,169 @@
+/**
+ * Default controller for handling text/html responses.
+ *
+ * It's called DrunkController in order to absolutely ensure we rename it later.
+ *
+ * ...right?
+ */
+class DrunkController implements ContainerAwareInterface {

I'm not taking bets on that... :-)

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,169 @@
+    $attributes = $request->attributes;
+    // Strip off stuff that can't go through to subrequests.
+    $attributes->remove('system_path');
+    $attributes->remove('_content');

Since $attributes is an object, this will modify the original request object. I don't think you want to do that. We probably need a clone() in here.

I read through the rest, but I'm having a hard time figuring out how it fits into the bigger picture.

sdboyer’s picture

yeah, clone of attributes is needed there. i'll add that when i get to it (which is in a bit, stepping through to it now).

the rest of what's in there is how we get the necessary shit lined up and into place to actually render something that looks like a drupal page. it'll make more sense with the next patch, i think; i'll be putting my progress up before i go to bed, a few hours off.

Letharion’s picture

StatusFileSize
new10.3 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new28.36 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

While I'm still trying to work this out, here's a patch combining sdboyers, mine and dysramas work. Posting to avoid work duplication.

Crell’s picture

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,169 @@
+      //$instance = block_manager()->getInstance(array('config' => $name));
+      $instance = entity_load('block', $name);

I'm confused by this obvious WIP. :-)

+++ b/core/modules/layout/lib/Drupal/layout/Config/UnboundDisplayStorageController.php
@@ -0,0 +1,36 @@
+class UnboundDisplayStorageController extends ConfigStorageController {

This class needs a docblock explaining WTH an UnboundDisplay is.

I think I need to just apply this patch and spend 40 minutes in XDebug...

sdboyer’s picture

StatusFileSize
new32.13 KB
FAILED: [[SimpleTest]]: [MySQL] 51,030 pass(es), 182 fail(s), and 42 exception(s).
[ View ]
new33.72 KB

ok, posting up another patch - mammoth progress. we now have blocks rendering, including the main content block (which is the most tricky one for this initial pass). what remains is:

  1. doing a hacky hook_theme_registry_alter() to make the bartik layout pick up all the normal page preprocess functions.
  2. probably moving the bartik layout into core - it really doesn't make a ton of sense to have in there. doing so means significantly reducing the number of regions, of course, so i'll probably just add a second one.
  3. futzing with various global state as needed so that the various blocks in there that do horrible encapsulation-breaky things get the data they expect to have. we can make those blocks smarter later.
  4. of course, we also very much need #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox] to finalize this. i'm switching gears back to that now rq, to get the problems raised there squared away (shouldn't take long).
  5. finally, some tests. just some basics, i already have a test module written (though i'm actually using it in a real spot, so it's not in the patch yet).

i'll keep on barreling along here, and we should have the rest of these things done shortly - tomorrow morning, methinks.

interdiff is against my last (#25), but incorporates the relevant bits from Letharion's #28.

sdboyer’s picture

StatusFileSize
new67.1 KB
new900 bytes
new35.88 KB
FAILED: [[SimpleTest]]: [MySQL] 51,040 pass(es), 182 fail(s), and 36 exception(s).
[ View ]
new17.68 KB

aaaand, we've got liftoff. attached is the first screenie of a page rendered fully through the blocks and layouts system. woohoo!

of the issues listed in #30, 1 and 4 are taken care of. 1 turned out to be a simple matter of defining 'base hook' for all layouts as being 'page'; that causes those calls to inherit the preprocess & process of the base hook. exactly what we want. 4 is taken care of by copying in the meat of the PartialResponse patch in and simply dropping it into the disposable shim DrunkController::wrapHtml method.

i've gotten a start on 3, in a sense, with the weird logic in DrunkController::wrapHtml to handle sidebar counting - template_preprocess_html inspects the page variable to see if it has sidebars and sets classes accordingly, so we needed to mimic that. for the most part, those kinds of fixes are ones that should be taken care of in followups on #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox].

2 remains outstanding; it really kinda merits some further discussion.

5 is also outstanding, but in the meantime if you'd like to see this on your local, grab the booze_test tarball i also attached and install that, then just visit /booze.

at Crell's suggestion, here's a list of suggested breakpoints to set if you'd like to step through this to see WTF is going on. These are given in the order in which you'll hit them when stepping through the code:

  1. First, have a look at booze_test.routing.yml to see the base route properties we're working with. the expectation is that we will be injecting display properties directly onto routes like this, at router build time.
  2. Drupal\Core\Booze\DisplayRouteEnhancer::enhance() at line 29. This is where the _display property on the route gets turned into a proper Display object.
  3. Drupal\Core\Booze\ContentRouteEnhancer::enhance()at line 31. This is where the _content property on the route gets bound into a closure, for injection into and execution by the SystemMainBlock much later.
  4. Drupal\Core\EventSubscriber\RouteProcessorSubscriber::onRequestSetController() at line 41. This is where we inspect the current request, and determine that it's an HTML request without an explicitly specified controller (our long-agreed upon criteria for using the SCOTCH rendering approach), and then set the DrunkController for use out of the container.
  5. Drupal\Core\EventSubscriber\DrunkControllerSubscriber::onDrunkenKernelController() at line 38. This is an alternate approach to adding displays vs. what I described with the DisplayRouteEnhancer, where instead of explicitly registering them on the routes, we inject them with this event. We may utilize this approach initially with this patch, as it's a bit of a handy shortcut vs. doing route altering, but it probably won't stay in long-term. Anyway, its logic is also pretty clear: it only acts if the DrunkController is the active controller, and if a real Display object hasn't already been set on the request (i.e., by the DisplayRouteEnhancer).
  6. Drupal\Core\Booze\DrunkController::respond() at line 109. Being that this is the main event, it's worth stepping through pretty much everything here. However, some of the calls out to other stuff can get circuitous, so, two more breakpoints for ya.
  7. Drupal\block\BlockRenderController::render() at line 98. This is where we make the direct calls to go in and render the block. Line 77 is where you want to move that breakpoint when you get tired of tapping through to the actual call that goes to the respective block plugin.
  8. Drupal\system\Plugin\block\block\SystemMainBlock::build() at line 34. This is where we use that magic closure that we made waaay back up in ContentRouteEnhancer and proxy off to that code. The injection mechanism for this is horrible (there's a @todo about that in the DrunkController, as we still need to work out a good, abstracted way for blocks to report the data they need, and then for the controller to prepare it for injection.

thassabout it. this code is *clearly* still very much in progress (i'm not even setting it to needs review...though, please do :P), but it's worth being excited about: it's the that key bridge between our new base system and the HTML rendering system we've all been discussing for so long.

webchick’s picture

Status:Active» Needs review

Marking needs review to get some eyeballs on this.

Status:Needs review» Needs work

The last submitted patch, controllers-1812720-31.patch, failed testing.

jthorson’s picture

Status:Needs work» Needs review

#30: controllers-1812720-30.patch queued for re-testing.

Crell’s picture

Status:Needs review» Needs work

I'm not convinced that DisplayRouteEnhancer should be an Enhancer and not a ParamConverter. Subtle difference, yes, but I think it ought to be a converter.

Other comments:

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,240 @@
+
+    // Create a Response object right away that we can easily decorate as we go.
+    $this->response = new Response();

Except we don't actually do anything with it in the sub-calls, do we? I don't think so. Don't make it an object property unless you absolutely have to.

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,240 @@
+    $this->response->setContent($this->wrapHtml($content));

This would be a bit easier to debug if this was broken into 2 lines. I know it's tidy this way, but for now it's easier on the debugger if it's 2 lines. Just sayin'.

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,240 @@
+      $instance = entity_load('block', $name);

This needs to be an entity_load_multiple(), otherwise we get into a SELECT N+1 problem.

Also, for the love of god don't call this variable $instance. That's too abstract to mean anything, especially since it is guaranteed to get confused with plugin instances on the VERY NEXT LINE. Just call it $block.

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,240 @@
+      if ($plugin instanceof SystemMainBlock) {
+        $instance->set('_content_closure', $request->attributes->get('_content_closure'));

I don't like this at all. We're setting _content_closure on the request, so that we can move it to an entity, so that we can pass it to a block, so that we can take it back off again and call it? WAT?

+++ b/core/lib/Drupal/Core/Booze/DrunkController.php
@@ -0,0 +1,240 @@
+        $this->renderedBlocks[$name] = $this->entityManager
+          ->getRenderController($instance->entityType())
+          ->render($instance);

If I'm reading this correctly, we'll always get the same render controller since it's always the same entity type, no? (Block) So can't we just look up the renderer once and not have to re-fetch it every time?

Overall, yeah, still rough. :-) I think my biggest concern is with the handoff from _content to SystemMainBlock. That process feels way too convoluted to me.

sdboyer’s picture

awesome. thanks for the review.

Except we don't actually do anything with [Response] in the sub-calls, do we? I don't think so. Don't make it an object property unless you absolutely have to.

nope, not at the moment. we will once we start dealing with PartialResponse, as we'll be stacking the PartialResponse from each block up onto the main Response that's attached to the object. it is code cruft though, and could easily be removed for right now.

This would be a bit easier to debug if this was broken into 2 lines. I know it's tidy this way, but for now it's easier on the debugger if it's 2 lines. Just sayin'.

ok done, np.

This needs to be an entity_load_multiple(), otherwise we get into a SELECT N+1 problem.

strongly agreed. i'd been pestering heyrocker about multiloading of config where we can't rely on prefixing since...september?. there are some possible approaches, but the thought was that we get it working first.

and yeah, i think i just called it $instance b/c i copied something over from the BlockRenderController. however, i don't like calling it $block, since it's the block entity, but not the block plugin, and if i had to choose (since our current system delightfully forces us to)...i'd call the latter the "block," not the former.

If I'm reading this correctly, we'll always get the same render controller since it's always the same entity type, no? (Block) So can't we just look up the renderer once and not have to re-fetch it every time?

yeah, i thought i'd actually taken that extra step. guess i forgot to, but certainly no reason not to.

I don't like this at all. We're setting _content_closure on the request, so that we can move it to an entity, so that we can pass it to a block, so that we can take it back off again and call it? WAT?

yep. i was planning on writing the logic for the passthrough to the main content block inside the controller, until i realized that it would be way easier to figure out back before anything's been mucked with on the route. and that the contract for how _content works should be enforced somewhere other than the controller that's pulling these elements together.

the fact that it's dropped on the entity in order to inject it into the block is utter insanity though, i agree. THAT is symptomatic of the fact that we haven't dealt with context/injection into blocks at all yet. for which there is a @todo right above what you selected :)

as for rendering it again? ugh, i suppose that might happen, since we end up calling it when we have to invoke hook_page_build(). that's already marked "fuck off as soon as possible."

msonnabaum’s picture

Will have more comments later, but for now:

Drupal\layout\Config\DisplayBase is an odd namespace. It's exposing unnecessary implementation details. Let's treat it like a first class model and hide that stuff.

Although I like Booze, we should at least try to name these classes. It's pretty hard to figure out what the roles are of some classes with the current naming.

webchick’s picture

Issue tags:+Blocks-Layouts

As far as I can tell, this is currently the #1 issue to resolve for SCOTCH, so tagging accordingly.

fabsor’s picture

StatusFileSize
new35.66 KB
FAILED: [[SimpleTest]]: [MySQL] 50,509 pass(es), 275 fail(s), and 40,144 exception(s).
[ View ]

Reroll after various changes.

fabsor’s picture

StatusFileSize
new890 bytes

And here is an updated version of the booze_test module.

Crell’s picture

Just an FYI, this patch is going to affect this issue, too: #1938980: Fix Ajax system; the last remnants of the old API must be swept away

But I think in a good way.

Edit: Oh wait, the latest patch is already doing things the new way! Nifty!

andyceo’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, controllers-1812720-39.patch, failed testing.

sdboyer’s picture

Status:Needs work» Postponed

per our discussions last week with Dries et. all, we're postponing this and working on it in a sandbox.

no more artificial breaking this stuff up in a way that no one can comprehend. WOOHOO!

sdboyer’s picture

Title:Implement the new panels-ish controller» Implement the new panels-ish controller [it's a good purple]

at neclimdul's suggestion, which i thought was good :)

webchick’s picture

LOL nice. :)

xjm’s picture

@sdboyer, part of what we discussed on the call was that in order for a sandbox to be workable, we would have actively maintained issues in the core queue with rebased, interdiffed milestone patches every week or two so that core reviewers could track progress and give input. I don't think that the goal of working in a sandbox was to skip the review process--if that's the "benefit" you and @EclipseGc see here, I'm really concerned. :( If we go that route, we are just going to end up in another situation where we have another unreviewable monster 400 K diff against core, and this time I have a full-time job so I'm not going to be able to invest 120 hours in reviewing it.

"Postponed" also hides them from thresholds, which is a lie. Hopefully we can all get on the same page about this.

EclipseGc’s picture

xjm,

The benefit isn't that we skip the core issue queue, but that the patch we supply actually shows the whole picture of what we're doing, so that issues like this one that don't obviously stand on their own have some context in which to judge and understand them. No one is skipping the review process, but Dries did specifically say that it doesn't matter if the patch is 200-300K he'd review it. So we are going to have to have another discussion if the perception is that we're still maintaining all of these componentized issues that don't do anything on their own, because that doesn't solve any of the vision selling/communication issues we've had thus far.

That being said, there are still other things I think we believe should go into core on their own. The entity wizard, the block improvements, condition improvements, condition groups, etc are all examples of things that can be developed the way we want in the sandbox and maintained in their own issues since they leave core "shippable" and provide exposure of existing features or function as cleanups.

Hopefully we're on the same page here, if not we should take the conversation out of the issue queue to somewhere we can communicate more directly.

Eclipse

Cottser’s picture

Version:8.x-dev» 9.x-dev
Issue summary:View changes

This is counting towards thresholds so it can't sit here postponed forever :) Bumping to 9.x for now.

Wim Leers’s picture

Version:9.x-dev» 8.1.x-dev
Status:Postponed» Active

I read the entire issue.

AFAICT the overarching goal of this issue is: make it easy to implement alternative ways of rendering pages (i.e. main content with some decoration, like we have Blocks by default). And hence ensure that in Drupal 8, Panels and other will be cleanly implementable.

On top of that, this issue more or less wants to standardize how essential parts of alternative ways of rendering pages are implemented:

  1. a Display object that contains the entire configuration for something that is renderable (displayable). Panels would have its own subclass of this object, as would DS, as would others.
  2. setting _display on routes to ensure a certain display is used
  3. a KernelEvents::CONTROLLER subscriber to load the Display object associated with _display
  4. a controller per module (Panels, DS…) that does the actual rendering work

Well, AFAICT, the overarching goal was achieved with the combination of #2286357: Introduce Display Variants, use for the block rendering flow and the introduction of the RenderEvents::SELECT_PAGE_DISPLAY_VARIANT event in #2352155: Remove HtmlFragment/HtmlPage. Panels, page manager, DS and others would each implement a page display variant. That's how they can choose to render pages differently.

The things on top of that are merely nice to haves, except for us needing a way to select a page display variant. In this issue, that was being done through a _display route attribute. In HEAD, we now do that using the RenderEvents::SELECT_PAGE_DISPLAY_VARIANT event.

In other words: PageDisplayVariants solve the must-have of this issue, everything else are nice-to-haves that potentially might have limited innovation in PageDisplayVariants, because they add more strict requirements. Now there is a lot of freedom to innovate and experiment in D8 contrib.

Therefore I think we can close this.

Wim Leers’s picture

Version:8.1.x-dev» 8.0.x-dev
catch’s picture

Status:Active» Closed (works as designed)

I'm going to go ahead and close this. Whether we tie display variant selection to a global event or the route seems like it's worth discussing (if only due to issues elsewhere with global events and performance), but that hasn't come up in profiling at all and the rest of the discussion is something that could be picked up anew in 8.1.x or later.

Closing as 'work as designed' since the variant selection event covers the main use case here.