Support context aware layout plugins

Ouch, I didn't realize we left this out so completely.
Moving to 9.0 to be able to run tests

Desperately needs #2273381: Convert ContextAwarePluginBase to traits, but embedded the methods directly for now.

Postponing on the plugin system issue.

Rebasing the patch from #2 to assume the patch in the linked issue will be merged.

Backporting patch from #7 to 8.9.x branch.

The patch has been updated to seed Section::getLayout() with contexts from LayoutBuilderContextTrait::getAvailableContexts($section_storage) to avoid the following exception when attempting to use the layout_builder.entity context for the default layout:

You must provide the context IDs in the @{service_id}:{unqualified_context_id} format.

This exception was being thrown because the custom contexts provided by Layout Builder via LayoutBuilderContextTrait::getAvailableContexts() weren't being included in the layout plugin since Section::getLayout() was using context.repository to get available contexts.

To fix this, an additional parameter array $contexts = [] was added to Section::getLayout() to allow contexts to be injected by the caller since this method can't access $section_storage to retrieve contexts from Layout Builder automatically. Each call to Section::getLayout() was then updated to include the additional parameter where possible.

While it's not ideal to add this parameter to Section::getLayout(), I'm not sure that there's a better way to handle this. I'm open to suggestions for how this could be handled better.

This brings up a potential pain point for context definition plugins: should they have a canonical way to provide sample values? If so, we could eliminate the need for SectionStorageInterface::getContextsDuringPreview() and instead rely on plugin-provided sample values. This would enable us to use context.repository with no repercussions (as far as I can tell) and avoid having to pass contexts to Section::getLayout().

Despite this being blocked on the other issue, if you're actively working on the patch it's better to keep the tests running so we can be sure each change isn't breaking something. For now that means posting "combined" patches with this and the other blocking issue.

No changes have been made since #10; just reorganizing to facilitate testing.

Changed ConfigureSectionForm to not pass any contexts into Section::getLayout() similar to how ConfigureBlockFormBase doesn't pass any contexts into SectionComponent::getPlugin().

This should resolve the test failure from #12.

+++ b/core/modules/layout_builder/src/Form/MoveBlockForm.php
@@ -119,9 +121,10 @@ public function buildForm(array $form, FormStateInterface $form_state, SectionSt
+    $contexts = $this->getAvailableContexts($section_storage);
...
-      $layout = $section->getLayout();
+      $layout = $section->getLayout($this->getAvailableContexts($section_storage));

Should use the $contexts variable inside the loop

Thanks for pointing that out! It looks like I created the variable and everything... just forgot to actually use it.

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -129,6 +132,16 @@ class LayoutDefinition extends PluginDefinition implements PluginDefinitionInter
+    // If there are context definitions in the plugin definition, they should
+    // be added to this object using ::addContextDefinition() so that they can
+    // be manipulated using other ContextAwarePluginDefinitionInterface methods.
+    if (isset($definition['context_definitions'])) {
+      foreach ($definition['context_definitions'] as $name => $context_definition) {
+        $this->addContextDefinition($name, $context_definition);
+      }
+      unset($definition['context_definitions']);
+    }

Idk if it should be done here or in a follow-up, but this is very important code that is 1:1 lifted from \Drupal\layout_builder\SectionStorage\SectionStorageDefinition::__construct() and should probably be moved to a trait.

The patch looks good to me, and iteration has largely stopped here, so I'm putting this back to postponed for now.
Thanks @clayfreeman!

Sounds good.

I agree that a pause is in order on this for now until the blocking issue is merged. Once that's been accepted, I'll pick this back up.

Thanks @tim.plunkett for the work you've done on this issue!

Didn't realize this hadn't already been done, but adding the aforementioned issue.

Blocker is in. This is just the standalone patch from #16 minus one debug line

Sorry for the delayed response.

I've tested this patch against 9.1.0-beta1 and I was able to get my context-dependent layout working.

The patch also seems relatively straightforward without too much room for error from what I can tell.

+++ b/core/lib/Drupal/Core/Layout/LayoutDefinition.php
@@ -11,8 +13,9 @@
-class LayoutDefinition extends PluginDefinition implements PluginDefinitionInterface, DerivablePluginDefinitionInterface, DependentPluginDefinitionInterface {
+class LayoutDefinition extends PluginDefinition implements PluginDefinitionInterface, DerivablePluginDefinitionInterface, DependentPluginDefinitionInterface, ContextAwarePluginDefinitionInterface {

+++ b/core/lib/Drupal/Core/Layout/LayoutInterface.php
@@ -6,11 +6,12 @@
-interface LayoutInterface extends PluginInspectionInterface, DerivativeInspectionInterface, ConfigurableInterface, DependentPluginInterface {
+interface LayoutInterface extends PluginInspectionInterface, DerivativeInspectionInterface, ConfigurableInterface, DependentPluginInterface, ContextAwarePluginInterface {

would be great to document API changes, probably change record required

CR at https://www.drupal.org/node/3183338, setting back to RTBC. Thanks for pointing that out @andypost!

@tim.plunkett Thank you! CR looks great

RTBC++

Kinda surprised there's no unit test changes here. But looking at everything I think we're good. It's always hard to tell if everything is covered by tests.

+++ b/core/modules/layout_builder/src/Section.php
@@ -422,4 +430,24 @@ public function getThirdPartyProviders() {
+  /**
+   * Wraps the context repository service.
+   *
+   * @return \Drupal\Core\Plugin\Context\ContextRepositoryInterface
+   *   The context repository service.
+   */
+  protected function contextRepository() {
+    return \Drupal::service('context.repository');
+  }

This is not used anywhere.

Section::getLayout was never tested before because it was a one-line wrapper. When I went to add some, I kept hitting this bug where I couldn't get the test to switch on the instanceof check.

Which led me to realize that the instanceof check was added in a version of the patch before we extended the interface.
Yay for writing tests!

1. +++ b/core/modules/layout_builder/src/Section.php
   @@ -88,17 +88,24 @@ public function toRenderArray(array $contexts = [], $in_preview = FALSE) {
   -  public function getLayout() {
   ...
   +  public function getLayout(array $contexts = []) {
   

   primary change has BC

2. +++ b/core/modules/layout_builder/src/Section.php
   @@ -422,4 +429,14 @@ public function getThirdPartyProviders() {
   +  protected function contextHandler() {
   +    return \Drupal::service('context.handler');
   
   +++ b/core/modules/layout_builder/tests/src/Unit/SectionTest.php
   @@ -362,4 +366,40 @@ public function testGetThirdPartyProviders() {
   +    $container = new ContainerBuilder();
   +    $container->set('plugin.manager.core.layout', $layout_plugin_manager->reveal());
   +    $container->set('context.handler', $context_handler->reveal());
   +    \Drupal::setContainer($container);
   ...
   +    $output = $this->section->getLayout($contexts);
   

   somehow there's no way to inject it...

3. +++ b/core/modules/layout_builder/tests/src/Unit/SectionTest.php
   @@ -362,4 +366,40 @@ public function testGetThirdPartyProviders() {
   +  public function testGetLayout(array $contexts, $should_context_apply) {
   ...
   +    if ($should_context_apply) {
   

   $should_context_apply needs boolean type-hint

1+2)
Section is a domain object, and is not subclassed. It's either created by new Section() or Section::fromArray().

Adding an *optional* parameter to a method that is not backed by any interface is not a BC break.

Because it's a domain object, it has no mechanism for dependency injection.

As you can see, this is not the first method in the class to use Drupal::service()

3)
"needs" is a little strong :)
Also Drupal's coding standards only mention objects and arrays. No guidance/requirement of scalars to be found.
This will be only the 7th bool parameter type in core...

This looks good to me.

This looks good - I've got one more question...

+++ b/core/config/schema/core.data_types.schema.yml
@@ -362,6 +362,11 @@ layout_plugin.settings:
+    context_mapping:
+      type: sequence
+      label: 'Context assignments'
+      sequence:
+        type: string

Do we have to worry about an update path here? I.e. if some one is using layouts and the go and press save (without making any changes) on a layout is there any change that their configuration will change? I tried to work this out for myself but I'm not sure either way.

@alexpott It seems that an empty context mapping is added to layouts once this patch is applied, but only after a layout is updated:

         layout_id: layout_onecol
         layout_settings:
           label: ''
+          context_mapping: {  }
         components:
           5aac1b30-b0d3-4504-992e-e8ab6888c305:
             uuid: 5aac1b30-b0d3-4504-992e-e8ab6888c305

I wouldn't anticipate this being an issue, however. (I didn't encounter any errors or hiccoughs while checking for config changes.)

@clayfreeman #32 means that we need to provide an update function because configuration can change merely by resaving it. We need an update function so, for example, unexpected configuration does not occur on production systems making it harder to merge development changes in. Configuration should always be up-to-date with what would happen if you do:

\Drupal\Core\Entity\Entity\EntityViewDisplay::load('some_id')->save();

So I think we need a post update function to use the \Drupal\Core\Config\Entity\ConfigEntityUpdater to re-save all config that could be affected. I think that that is entity view display which are using layout.

Added an update hook to Layout Builder to add empty context mappings to entity view displays.

1. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -34,3 +40,31 @@ function layout_builder_removed_post_updates() {
   +    if ($display instanceof LayoutBuilderEnabledInterface && $display instanceof SectionListInterface && $display->isLayoutBuilderEnabled()) {
   

   LayoutEntityDisplayInterface extends SectionListInterface, so no need to check both

2. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -34,3 +40,31 @@ function layout_builder_removed_post_updates() {
   +        $section->setLayoutSettings($section->getLayoutSettings() + [
   +          'context_mapping' => [],
   +        ]);
   

   This shouldn't be explicitly needed, just the fact that it is saved will do whatever we need.

3. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -34,3 +40,31 @@ function layout_builder_removed_post_updates() {
   +        $needs_update = TRUE;
   

   Which means we can return TRUE here, no need to keep looping

4. +++ b/core/modules/layout_builder/layout_builder.post_update.php
   @@ -34,3 +40,31 @@ function layout_builder_removed_post_updates() {
   +    return $needs_update;
   

   Can return FALSE here with the other change.

Honestly, I think the whole callback could be a one-liner:

    return $display instanceof LayoutBuilderEnabledInterface && $display->isLayoutBuilderEnabled();

1. Ah, this is much simpler; thanks!
2. This is required; I tested it without (assuming the save operation would be enough) and it didn't add the empty context mapping. As a result, we must apply this update to each section manually.

Attached a patch with the updated interface & conditional.

Fair enough. Thanks!

@clayfreeman update paths need a test. What's interesting though is this...

If I do this with and without the patch applied I get the same configuration for core.entity_view_display.node.article.default

1. Install standard
2. Install layout_builder
3. Log in and visit admin/structure/types/manage/article/display
4. Enable layout builder

If I then apply the patch to site that was created without the patch and run the new update method the config gets a context_mapping and a new dependency on layout_discovery! Not sure what is going on here. I think that there is another bug here. If I do the same steps as above on an unpatched site and then do \Drupal\Core\Entity\Entity\EntityViewDisplay::load('node.article.default')->save(); via drush php this will result in the layout_discovery dependency being added.

Also given that we have to call

+        // Add an empty context mapping to each section where one doesn't exist.
+        $section->setLayoutSettings($section->getLayoutSettings() + [
+          'context_mapping' => [],
+        ]);

in order to set it I'm confused about how to get an empty context_mapping via the UI. @clayfreeman can you detail the steps you took in #32?

Replication steps on a clean 9.2.x testing environment:

1. Install the Layout Builder module.
2. Enable Layout Builder for one of the default content type's default view display.
3. Next, we want to shake out any configuration discrepancies that may or may not exist, but are likely outside of the scope of this issue. Begin by clicking "Manage layout" on the view display configuration form for which you enabled Layout Builder.
4. Click "Configure Section 1," then click "Update."
5. Click "Save layout."
6. Finally, click "Save" on the entity view display configuration.
7. Now we want to export the site's configuration as a baseline for comparison. Visit /admin/config/development/configuration/full/export and click "Export."
8. Extract this archive to the source tree, then commit. We'll repeat this step later.

Now that you have a baseline in place, you can either choose to apply the patch then repeat steps 3 - 8, or run & compare the update result and repeat steps 7 - 8. In either case, you should see the difference in configuration is identical (i.e., a single line difference where the empty context_mapping value is added).

I'm not sure why you're seeing anything different; possibly because your replication criteria didn't include steps 3 - 6.

With regard to a test case for the update hook, I'll be researching precedence for how those should be added and attach an updated patch later today.

@alexpott

In trying to write a test for the update path, I discovered the following:

1. There is no Drupal 9 dump from which to develop an update path test, and I don't know how to make one of these.
2. Using the Drupal 8.8 dump, there is an error when attempting to invoke only ::runUpdates(). It appears that an update for system is being ran to uninstall Classy. This causes Layout Builder to attempt to access a nonexistent table because the hack-y install method that these update tests have to use doesn't give Drupal a chance to initialize the module's schema.
3. Attempting to install this schema seems nigh impossible without a bunch of extra work.

Without being able to write a test for this update path, my next step was to see if there may be an alternative that doesn't require an update hook; I discovered that if we make the following change to the previous patch, no difference exists between an unpatched & patched site's configuration for layouts without context definitions:

--- a/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
+++ b/core/modules/layout_builder/src/Form/ConfigureSectionForm.php
@@ -176,7 +176,9 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
     // If this layout is context-aware, set the context mapping.
     if ($this->layout instanceof ContextAwarePluginInterface) {
-      $this->layout->setContextMapping($subform_state->getValue('context_mapping', []));
+      if ($context_mapping = $subform_state->getValue('context_mapping', [])) {
+        $this->layout->setContextMapping($context_mapping);
+      }
     }
     $plugin_id = $this->layout->getPluginId();

This doesn't really solve the issue per se; it simply avoids setting an empty sequence if there is no context mapping data.

I think @tim.plunkett and I both agree that an update path would be cleaner instead of relying on a NULL default for the context mapping value, but I think it's a decent compromise given the situation.

Configuration differences could still occur with this patch if a layout plugin defines at least one optional context definition, maps a value for it, and subsequently removes the mapping. (The context mapping value under this circumstance would result in an empty array.)

For all existing layouts without context definitions, there is no chance for discrepancy and the update hook can be postponed and addressed in a follow-up issue.

I'm uploading this alternate patch for your consideration, as I'm on holiday starting tomorrow & through next week and will be unavailable to contribute any additional patches during that time.

This is now blocked on landing #3194562: Add database dumps for 9.0.0

Updating patch with test using 9.0.0 dump.

Since #3194562: Add database dumps for 9.0.0 is now merged, and tests are again passing, this should be ready for additional review.

#43 is working for me.

Committed 5e7fb73 and pushed to 9.2.x. Thanks!

Removing needs subsystem maintainer review because @tim.plunkett is a subsystem maintainer and okayed the subsystem changes in #24 - the main blocker since then was update path.