Problem/Motivation
Layout builder has a relatively naive context implementation that uses Core's normal context provider approach. This is problematic because Layout Builder itself thinks in terms of entities generically while core's context providers attempt to provide very specific entities. This is especially problematic with the NodeRouteContext which provides a node context at all times regardless of the presence of an actual node. This is purposeful in order to support the core block module's approach to block placement which is largely context-ignorant.
Proposed resolution
Get core's context provider list, ignore NodeRouteContext's provided context and create a "current_entity" context from the provided entity being "laid out". That context can be provided as part of the layout field's build process and needn't come directly from any context provider in core. This approach will allow us to support both embedded laid out entities with contextual blocks within other entities as well as things like views rendering entity layouts.
Remaining tasks
Write the code.
User interface changes
It would add new context for contextual blocks. The ignoring of the provided NodeRouteContext would typically prevent any UI changes except for during User entity layouts since both a "current user" and a "current entity" context would compete in that case. TBD how we want to handle that but it's pretty edge-casey.
API changes
Existing layout builder methods, especially those on the layout field, probably need to pass the context stack around rather than obtaining it late in the build process as happens now.
Data model changes
No expected changes.
Comment | File | Size | Author |
---|---|---|---|
#45 | 2921626-layout-45.patch | 15.23 KB | tim.plunkett |
#45 | 2921626-layout-45-interdiff.txt | 571 bytes | tim.plunkett |
#41 | 2921626-layout-41.patch | 14.98 KB | tim.plunkett |
#41 | 2921626-layout-41-interdiff.txt | 756 bytes | tim.plunkett |
#36 | 2921626-layout-36.patch | 14.24 KB | tim.plunkett |
Comments
Comment #2
EclipseGc CreditAttribution: EclipseGc at Acquia commentedAn initial attempt at what the code should probably look similar to. No additional test, just want to see what blows up and how.
Eclipse
Comment #3
EclipseGc CreditAttribution: EclipseGc at Acquia commentedbah... phpstorm.
Comment #5
EclipseGc CreditAttribution: EclipseGc at Acquia commentedLet's see how this fairs. I had to rewrite it from scratch after the layout_builder changes, so no interdiff. It's a small patch anyway.
Eclipse
Comment #6
EclipseGc CreditAttribution: EclipseGc at Acquia commentedoops
Comment #8
EclipseGc CreditAttribution: EclipseGc at Acquia commentedClearly, I completely screwed up the previous patch. Sorry.
Eclipse
Comment #9
EclipseGc CreditAttribution: EclipseGc at Acquia commentedSo this patch is provided warts and all. With the move to some calls happening as part of hook_entity_view_alter(), that meant I had to repeat some code. Obviously this could be remedied by creating a new service. I opted not to for the moment. I'm pretty sure that's the right solution long term.
Eclipse
Comment #11
tim.plunkettHere it is built on top of #2922033: Use the Layout Builder for EntityViewDisplays
Comment #12
tim.plunkettComment #13
tim.plunkettComment #14
xjmComment #15
tim.plunkettHere's a patch NOT built on all the other changes, since this is actually important now that FieldBlock is in.
Writing a test now.
Comment #16
tim.plunkettUh whoops
Comment #19
tim.plunkettFixes the above fail, and adds tests of its own.
Comment #21
EclipseGc CreditAttribution: EclipseGc at Acquia commentedThis looks really good. I've not applied the patch yet (which I will do momentarily) but from a pure code-review perspective, I'm very ++
Eclipse
Comment #22
tim.plunkettHere's a version that is dependent on #2927349: Decouple the Layout Builder UI from entities going in. Marking NW for now so as to not trigger the testbot.
Comment #23
tim.plunkettBlocker is in!
Comment #24
larowlanAny reason not to implement ContextProviderInterface here and add a new tagged service.
e.g. see NodeRouteContext etc
https://gist.github.com/larowlan/dc88c7e2cd2d816fe80d99a8c92160b3 was something I'd used before
Comment #25
larowlanI think if we plan to remove this after #2932462: Add EntityContextDefinition for the 80% use case, we should avoid adding it in the first place.
It's a couple of lines - we could just inline it.
Then when we have ECD, we can simplify it.
Comment #26
EclipseGc CreditAttribution: EclipseGc at Acquia commentedSo, the ContextProviders really are an adapter mechanism which extract context values from globally available services. Things like current user and language are all driven by the services in core, and the node is driven by the current request object. Unfortunately, that's the only scope at which context providers are really useful, and as we begin to delve into these more contextually-sensitive use cases and plugins, it becomes increasingly important that we have no only the global contexts, but also local ones.
The context object we're dealing with most commonly here is actually the entity which is reference by the field we're rendering during an entity override. So literally $field->getEntity(). This will change from one entity to the next, and in use cases like a view rendering multiple laid-out entities, we can't get the entity as context from the route. And even if we could, we didn't layout all the entities on the view page together, so they're all going to reference "node" incorrectly. (rather than something like node1, node2, etc). So the introduction of the current entity context in this patch allows us to pass around a context that's relevant for each entity being rendered because the field being rendered already knows what entity it's attached to, we just need to provide that entity as a context which can be mapped by the plugins.
TL:DR; ContextProviders weren't meant for this.
Eclipse
Comment #27
tim.plunkettImplementations of ContextProviderInterface rely on global state or preconfigured information.
We can rely on neither.
Imagine a view of nodes, each laid out with Sections.
When rendering each node, the context must be derived from that entity at that time. There's nothing in the routeMatch to help us there.
Inlined _layout_builder_get_contexts_for_entity()
Comment #28
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI'm totally on board with this last patch. Can we get an RTBC?
Eclipse
Comment #29
samuel.mortensonLooks good to me. 👍
Comment #30
EclipseGc CreditAttribution: EclipseGc at Acquia commentedTim, Sam and I were discussing this and realized that the text we're using is "Current @entity" so for users it'd be "Current User" which is the exact label that the current user context uses, so we should change that. Tim suggested'@label being viewed which I fully support.
Relevant sections are:
Eclipse
Comment #31
tim.plunkettActually, the suggestion was from @dead_arm, she deserves the credit there :)
Here's the after screenshot, before it said "Current user" twice
Comment #32
EclipseGc CreditAttribution: EclipseGc at Acquia commentedYay @dead_arm.
No reason I can't re-rtbc given the scope of the interdiff.
Eclipse
Comment #34
webchickAdding @dead_arm to issue credit. :)
Comment #35
larowlanI manually tested this.
Steps taken
Test site -> https://due6v.ply.st/node/1/layout
Comment #36
tim.plunkettIf we don't have any contexts passed, we shouldn't run them through the ContextHandler.
Simple fix, really nasty side effects without it.
Comment #37
EclipseGc CreditAttribution: EclipseGc at Acquia commentedTim and I discussed this together, and I am still totally on board.
I double checked add/edit/delete of a field block. Given the scope of the interdiff, I'm going to re-rtbc it pending tests passing.
Eclipse
Comment #38
larowlanIdeally we'd have tests here, but we're blocked on #2924201: Resolve random failure in LayoutBuilderTest so that it can be added to HEAD, which is already tagged as a beta blocker
Comment #39
larowlanComment #41
tim.plunkett😩
Comment #42
EclipseGc CreditAttribution: EclipseGc at Acquia commentedStill on the RTBC bandwagon over here (pending tests).
Eclipse
Comment #43
webchickOk, Tim walked me through this patch, as well as demoing the before/after behaviour. We hit a snag at one point with some PHP errors, but tracked it down to applying the patch mid-demo and some old cruft that was exhibiting the bugs this patch is fixing leaking into the new stuff. Upon clearing that out and starting over, the patch worked great.
It's really nice that this patch limits the number of options available to people to stick in their layouts to only those that are relevant. Huge +1 to that from a UX POV.
The code itself is really straight-forward. There's one bit of ugliness which is some manual string concatenation ickiness in a couple of places, but there's a follow-up to make a nice API method for that at #2932462: Add EntityContextDefinition for the 80% use case. There's a method that gets removed, and a couple of others that get their signatures adjusted for new (optional) parameters, but all of these are limited to the layout builder module itself, not anywhere else in core. Additionally, there's test coverage for the new behaviour.
So, in sum: no complaints here! :)
Committed and pushed to 8.5.x. Thanks!
Comment #45
tim.plunkettStupid freaking PHP 5.
Thanks PHP 7 for fixing this asinine rule for traits.
Comment #46
EclipseGc CreditAttribution: EclipseGc at Acquia commentedblah
Eclipse
Comment #49
webchickOk, take two. :)
Committed and pushed to 8.6.x (oooh!) and cherry-picked to 8.5.x.
Comment #53
tim.plunkettComment #54
vitalie CreditAttribution: vitalie commentedHi, looks like whatever was done here, was undone without much explanation in later commits. How could I pass the current entity to a Views block placed using Layout Builder?
Comment #55
yonailo CreditAttribution: yonailo commentedYes I've got the same problem, I have a contextual filter and when I add the views block to the layout builder, I do not see the option to choose the contextual parameter. Any pointers to help ?
Comment #56
maskedjellybeanSame problem for me as unknownguy. I suspect this is not really what this issue was about, but could we look to https://www.drupal.org/project/viewsreference as an example of how to provide an Argument form field which a user could use to enter arguments that are passed to the view?
Comment #57
eworwa CreditAttribution: eworwa at Cyber-Duck commentedSame here, I've been trying different combinations but I'm still not able to 'expose' contextual filters when including views (block displays) using Layout Builder, any hint on this?
Comment #58
maskedjellybeanHey @eworwa I was able to pass arguments to a view block via a field in Layout Builder by applying this patch to ctools:
https://www.drupal.org/project/ctools/issues/2759445
Pay attention to comment #55. You have to do a tiny amount of configuring within the view itself for the field to appear.
Good luck!
Comment #59
super_romeo CreditAttribution: super_romeo as a volunteer commentedContexts _without_ data not visible in block context selector.
I created https://www.drupal.org/project/drupal/issues/3194198