Change Layout Preparation into an Event to allow proper alterations

+1

Looking 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.

Tests look good!

1. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -53,15 +62,35 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   -  public function __construct(array $configuration, $plugin_id, $plugin_definition, LayoutTempstoreRepositoryInterface $layout_tempstore_repository, MessengerInterface $messenger) {
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, LayoutTempstoreRepositoryInterface $layout_tempstore_repository = NULL, MessengerInterface $messenger = NULL, EventDispatcherInterface $event_dispatcher = NULL) {
   ...
   +    if ($layout_tempstore_repository) {
   +      @trigger_error('Calling LayoutBuilder::__construct() with the $layout_tempstore_repository argument is deprecated in drupal:8.9.0 and will be removed in drupal:10.0.0', E_USER_DEPRECATED);
   +    }
   +
   +    if ($messenger) {
   +      @trigger_error('Calling LayoutBuilder::__construct() with the $messenger argument is deprecated in drupal:8.9.0 and will be removed in drupal:10.0.0', E_USER_DEPRECATED);
   +    }
   +
   +    if (!$event_dispatcher) {
   +      @trigger_error('Calling LayoutBuilder::__construct() without the $event_dispatcher argument is deprecated in drupal:8.9.0. The $event_dispatcher argument will be required in drupal:10.0.0.', E_USER_DEPRECATED);
   +      $event_dispatcher = \Drupal::service('event_dispatcher');
   +    }
   

   This should be something like this

   public function __construct(array $configuration, $plugin_id, $plugin_definition, $event_dispatcher, $messenger = NULL) {
   	parent::__construct($configuration, $plugin_id, $plugin_definition);
   	if (!($event_dispatcher instanceof EventDispatcherInterface)) {
   		@trigger_error('The event_dispatcher service should be passed to LayoutBuilder::__construct() instead of the layout_builder.tempstore_repository service since 9.1.0. This will be required in Drupal 10.0.0. See https://www.drupal.org/node/XXXXX.', E_USER_DEPRECATED);
   		$event_dispatcher = \Drupal::service('event_dispatcher');
   	}
   	$this->eventDispatcher = $event_dispatcher;
   	if ($messenger) {
   		@trigger_error('Calling LayoutBuilder::__construct() with the $messenger argument is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0', E_USER_DEPRECATED);
   	}
   }
   

   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...

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -72,8 +101,9 @@ public static function create(ContainerInterface $container, array $configuratio
   -      $container->get('layout_builder.tempstore_repository'),
   -      $container->get('messenger')
   +      NULL,
   +      NULL,
   +      $container->get('event_dispatcher')
   

   Then this could drop the NULLs

3. +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
   @@ -0,0 +1,79 @@
   +    $events[LayoutBuilderEvents::PREPARE_LAYOUT][] = ['onPrepareLayout'];
   

   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

4. +++ b/core/modules/layout_builder/tests/modules/layout_builder_element_test/src/EventSubscriber/TestPrepareLayout.php
   @@ -0,0 +1,118 @@
   +      if ($entity->id() === '2') {
   +        $event->stopPropagation();
   +      }
   

   Great! (Though very odd that it is a string, not an int, not what I expected 😅)

All of the implementation and tests look great, into the home stretch which means very nitpicky coding standards fixes :)

1. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -31,18 +31,11 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +   * The Event Dispatcher.
   

   The event dispatcher.

2. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -53,15 +46,24 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +   * @param \Drupal\Core\Messenger\MessengerInterface|NULL $messenger
   

   Lower case null

3. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -53,15 +46,24 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +   *   The messenger service. This is left optional for BC reasons and
   +   *   will be deprecated in drupal:9.1.0.
   

   (optional) The messenger service. This is no longer used and will be removed in drupal:10.0.0.

4. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -53,15 +46,24 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +      @trigger_error('The event_dispatcher service should be passed to LayoutBuilder::__construct() instead of the layout_builder.tempstore_repository service since 9.1.0. This will be required in Drupal 10.0.0.', E_USER_DEPRECATED);
   ...
   +      @trigger_error('Calling LayoutBuilder::__construct() with the $messenger argument is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0', E_USER_DEPRECATED);
   

   I believe these need a link to a change record. See other trigger_errors in core.

5. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -72,8 +74,8 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $container->get('event_dispatcher'),
   +      NULL
   

   This NULL can be removed

6. +++ b/core/modules/layout_builder/src/Event/PrepareLayoutEvent.php
   @@ -0,0 +1,45 @@
   + * Event fired when during preRender() of a LayoutBuilder Element.
   
   +++ b/core/modules/layout_builder/src/LayoutBuilderEvents.php
   @@ -26,4 +26,19 @@ final class LayoutBuilderEvents {
   +   * Name of the event fired when preparing a LayoutBuilder Element.
   ...
   +   * This event allows modules to collabourate on creating the sections used in
   +   * a LayoutBuilder Element during PreRender().
   

   "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 @see

   The 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.

7. +++ b/core/modules/layout_builder/src/Event/PrepareLayoutEvent.php
   @@ -0,0 +1,45 @@
   + * Subscribers to this event should manipulate the cacheability object and the
   + * build array in this event.
   

   This seems copy/pasted

8. +++ b/core/modules/layout_builder/src/Event/PrepareLayoutEvent.php
   @@ -0,0 +1,45 @@
   +   * The Section Storage Plugin.
   

   The section storage plugin.

9. +++ b/core/modules/layout_builder/src/Event/PrepareLayoutEvent.php
   @@ -0,0 +1,45 @@
   +   * LayoutPrepareEvent constructor.
   
   +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
   @@ -0,0 +1,79 @@
   +   * PrepareLayout constructor.
   
   +++ b/core/modules/layout_builder/tests/modules/layout_builder_element_test/src/EventSubscriber/TestPrepareLayout.php
   @@ -0,0 +1,118 @@
   +   * PrepareLayout constructor.
   

   Constructs a new _____.

   And make sure it's the right class name

10. +++ b/core/modules/layout_builder/tests/modules/layout_builder_element_test/layout_builder_element_test.info.yml
    @@ -0,0 +1,8 @@
    +core: 8.x
    

    Not any more!

11. +++ b/core/modules/layout_builder/tests/modules/layout_builder_element_test/src/EventSubscriber/TestPrepareLayout.php
    @@ -0,0 +1,118 @@
    + * An event subscriber that prepares a LayoutBuilder element for preRender.
    

    More preRender

12. +++ b/core/modules/layout_builder/tests/modules/layout_builder_element_test/src/EventSubscriber/TestPrepareLayout.php
    @@ -0,0 +1,118 @@
    +    // Act before LB subscriber.
    ...
    +    // Act after LB subscriber.
    ...
    +   * Subscriber to test acting before the LB subscriber.
    

    spell out Layout Builder

Hi @tim.plunkett
updated patch please review.

With the repeat of work between #16 and #17, and #16 passing automated testing while #17 fails, marking Needs Review with work of #16.

One 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.

1. +++ b/core/modules/layout_builder/src/Element/LayoutBuilder.php
   @@ -53,15 +46,24 @@ class LayoutBuilder extends RenderElement implements ContainerFactoryPluginInter
   +      @trigger_error('The event_dispatcher service should be passed to LayoutBuilder::__construct() instead of the layout_builder.tempstore_repository service since 9.1.0. This will be required in Drupal 10.0.0. See https://www.drupal.org/node/3152690', E_USER_DEPRECATED);
   ...
   +      @trigger_error('Calling LayoutBuilder::__construct() with the $messenger argument is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0', E_USER_DEPRECATED);
   

   The CR link should be in both

2. +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayout.php
   @@ -0,0 +1,83 @@
   +   * PrepareLayout constructor.
   

   Missed one

3. +++ b/core/modules/layout_builder/src/LayoutBuilderEvents.php
   @@ -26,4 +26,19 @@ final class LayoutBuilderEvents {
   +   * Name of the event fired when preparing a LayoutBuilder Element.
   ...
   +   * This event allows modules to collabourate on creating the sections used in
   +   * a LayoutBuilder Element during PreRender().
   

   no U in collaborate, and missed a #pre_render. Also didn't make the change about not calling LayoutBuilder Element.

4. +++ b/core/modules/layout_builder/src/LayoutBuilderEvents.php
   @@ -26,4 +26,19 @@ final class LayoutBuilderEvents {
   +   * @EVENT
   

   Not sure what this is supposed to be :D

Very 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.

This 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!

Random fail in CKEditorIntegrationTest. Requeued/reRTBC'd

Broken 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

Thanks!

1. It nice seeing the event leading to less dependencies being injected in the LayoutBuilder element. This is looking good.

2. +++ b/core/modules/layout_builder/src/Event/PrepareLayoutEvent.php
   @@ -0,0 +1,45 @@
   +   * Gets the section storage.
   +   *
   +   * @return \Drupal\layout_builder\SectionStorageInterface
   +   *   The section storage.
   +   */
   +  public function getSectionStorage() {
   +    return $this->sectionStorage;
   +  }
   

   Can use a return typehint in new code.

3. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderPrepareLayoutTest.php
   @@ -0,0 +1,119 @@
   +    // Chack the block is on the node page.
   

   typo Chack

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderPrepareLayoutTest.php
   @@ -0,0 +1,119 @@
   +    // When we edit the layout, it get's the static blocks.
   

   typo: get's

It 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.

I'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.

per #40

Committed e77ca8b and pushed to 9.1.x. Thanks!

Patch from #26 works with Drupal 8.9.2.

Published change record