Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently the LayoutBuilder Render element itself holds the key to the layout preparation, making it difficult for storage plugins or custom code to decide how to ouput a layout
Proposed resolution
Implement an Event for preparing a layout so subscribers can easily alter the sections.
Remaining tasks
Create an event/dispatcher for preparing layouts.Subscribe to the event to replicate current functionality.Deprecation notices for changed parameters.- Tests? Does this need to be tested outside of current test cases?
User interface changes
None
API changes
New LayoutBuilderEvents::PREPARE_LAYOUT
event for modules to interact without layouts during rendering.
Data model changes
None
Release notes snippet
New LayoutBuilderEvents::PREPARE_LAYOUT
event for modules to interact without layouts during rendering see https://www.drupal.org/node/3160587
Comment | File | Size | Author |
---|---|---|---|
#37 | interdiff-33_37.txt | 1.44 KB | b_sharpe |
#37 | 3143635-layout-event-9.1-combined-37.patch | 22.92 KB | b_sharpe |
#37 | 3143635-layout-event-9.1-tests-only-37.patch | 17.79 KB | b_sharpe |
#33 | interdiff-26_33.txt | 1.13 KB | b_sharpe |
#33 | 3143635-layout-event-9.1-combined-33.patch | 22.89 KB | b_sharpe |
Comments
Comment #2
b_sharpe CreditAttribution: b_sharpe at ImageX commentedNot sure if this warrants tests as functionality should not have changed. Please review.
NOTE: If using layout_builder_st (or any other module that overrides the LB Element class) then you will get an error since it is expecting different construct.
Comment #3
b_sharpe CreditAttribution: b_sharpe at ImageX commentedUpdated patch to allow BC for contrib as it seems there are a fair amount of known modules overriding the LB element due to the lack of being able to decorate plugins.
Comment #4
b_sharpe CreditAttribution: b_sharpe at ImageX commentedUpdated remaining tasks
Comment #5
mahmoud-zayed CreditAttribution: mahmoud-zayed as a volunteer and at ImageX for ImageX commented+1
Comment #6
tim.plunkettLooking good! Next up would be a test implementation of an event subscriber, maybe a couple or one with dynamic behavior. Run before core's, after, and one before that stops propagation.
Comment #7
b_sharpe CreditAttribution: b_sharpe at ImageX commentedAdded tests for before/after default event as well as stopping event propagation.
Comment #8
tim.plunkettTests look good!
This should be something like this
Then in 10.0, it'd get the typehint.
For example, see this evolution over 3 releases:
https://git.drupalcode.org/project/drupal/-/blob/8.6.x/core/modules/cont...
https://git.drupalcode.org/project/drupal/-/blob/8.7.x/core/modules/cont...
https://git.drupalcode.org/project/drupal/-/blob/9.0.x/core/modules/cont...
Then this could drop the NULLs
I think this should set a specific priority higher than 0, since this is our expected behavior. Then any 0 (default) priority subscribers will consistently after this one, unless they purposefully specify
Great! (Though very odd that it is a string, not an int, not what I expected 😅)
Comment #9
b_sharpe CreditAttribution: b_sharpe at ImageX commentedComment #12
b_sharpe CreditAttribution: b_sharpe at ImageX commentedWhoops, I accidentally reverted the test change before patch. Let's try this again.
Comment #14
tim.plunkettAll of the implementation and tests look great, into the home stretch which means very nitpicky coding standards fixes :)
The event dispatcher.
Lower case
null
(optional) The messenger service. This is no longer used and will be removed in drupal:10.0.0.
I believe these need a link to a change record. See other trigger_errors in core.
This NULL can be removed
"when during" in the first docblock
"LayoutBuilder Element" should be
\Drupal\layout_builder\Element\LayoutBuilder
to not be ambiguous. That can also be added to the @seeThe capitalization on preRender() is inconsistent, as are the parens
Also while it is a method name, it's only coincidentally the same. I think this should be
#pre_render
in all cases, as that's where it's actually run.This seems copy/pasted
The section storage plugin.
Constructs a new _____.
And make sure it's the right class name
Not any more!
More preRender
spell out Layout Builder
Comment #15
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedComment #16
b_sharpe CreditAttribution: b_sharpe at ImageX commented8.9.x changes here, will post 9.1.x if all looks good.
Comment #17
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedHi @tim.plunkett
updated patch please review.
Comment #19
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedComment #20
WebbehWith the repeat of work between #16 and #17, and #16 passing automated testing while #17 fails, marking Needs Review with work of #16.
Comment #21
b_sharpe CreditAttribution: b_sharpe at ImageX commentedAssigning to me to not muddy waters here...
I will repost a proper 9.1.x and a backport for original 8.9.x
Comment #22
b_sharpe CreditAttribution: b_sharpe at ImageX commentedSummary of changes to keep things in order here:
Comment #24
tim.plunkettOne more pass on the nits should do it.
Also it occurs to me that this is supposed to test the deprecation. You can pass in the things that aren't supposed to be passed anymore, make sure it still all works, and use
@expectedDeprecation
on that test method.The CR link should be in both
Missed one
no U in collaborate, and missed a #pre_render. Also didn't make the change about not calling LayoutBuilder Element.
Not sure what this is supposed to be :D
Comment #25
pyrello CreditAttribution: pyrello at The University of Iowa commentedVery nice work on this update. I think this is going to be a big improvement to the LB system.
I wanted to note that the event, as currently designed, only allows for alterations to the sections and components. The administrative links, such as "Add block" or "Add section" get added after this fires, so it is still necessary to use a '#pre_render' function to alter behavior related to that.
Comment #26
b_sharpe CreditAttribution: b_sharpe at ImageX commentedComment #28
tim.plunkettThis is a welcome addition, it has great test coverage, and to the best of my knowledge is up to par with how deprecations should be done. Great work @b_sharpe!
Comment #30
tim.plunkettRandom fail in CKEditorIntegrationTest. Requeued/reRTBC'd
Comment #32
tim.plunkettBroken by #3055198: [Symfony 5] Symfony/Component/EventDispatcher/Event is deprecated in Symfony 4.3 use Symfony/Contracts/EventDispatcher/Event instead, see https://www.drupal.org/node/3159012
tl;dr the arguments to ::dispatch() need to be swapped
Comment #33
b_sharpe CreditAttribution: b_sharpe at ImageX commentedUpdate per change record.
Comment #35
tim.plunkettThanks!
Comment #36
alexpottCan use a return typehint in new code.
typo Chack
typo: get's
Comment #37
b_sharpe CreditAttribution: b_sharpe at ImageX commentedfixes from last comment.
Comment #38
tim.plunkettIt was news to me that the policy is to add strict return types to code now, and that's not in the coding standards yet.
But as @alexpott clarified on slack, it's much easier to add them now than later...
Also I don't understand cspell, is that not integrated with testbot?
Either way, the patch is good.
Comment #40
alexpottI'm sorry on my last review I missed that we don't have a change record describing the new event and when to use it. Once the change record exists this issue can be set back to rtbc.
Comment #41
b_sharpe CreditAttribution: b_sharpe commentedAdded https://www.drupal.org/node/3160587
Comment #42
tim.plunkettper #40
Comment #43
alexpottCommitted e77ca8b and pushed to 9.1.x. Thanks!
Comment #45
vselivanovPatch from #26 works with Drupal 8.9.2.
Comment #47
quietone CreditAttribution: quietone at PreviousNext commentedPublished change record