Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides

I've continually worked to cut down on all the magic layout_builder__layout spread throughout. I think this is one of the final offenders, so +1 to this.

OverridesSectionStorage will require a fieldable entity context that has the layout_builder__layout field

This would require a new kind of context right? Does that mean that every section storage plugin would need to create a new kind of context that would only be used for checking if they apply?

I think I would prefer doing something like an isApplicable method, which would encapsulate the one-time logic needed to check if a section storage applies.

Tracked down the failures and opened #2982626: ContextAwarePluginBase is incompatible with ContextAwarePluginDefinitionInterface
This issue can include that override within SectionStorageBase for now, with an @todo linking to the upstream.

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -239,11 +241,27 @@ public function buildMultiple(array $entities) {
   +      $definitions = $storage_manager->getDefinitionsForContexts($contexts);
   +      if ($definitions) {
   +        $storage_type = key($definitions);
   

   This "get all the matching ones, but then use key() to get the first one" part seems awkward to me.

   What if there were another place within LB that we need to delegate to all of them? Or run this as a chain-of-responsibility, where we run through each one until one returns something (like breadcrumb builders)?

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -249,6 +252,13 @@ public function getSectionListFromId($id) {
   +    return $this->getContextValue('entity_view_display');
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -134,6 +137,13 @@ public function getSectionListFromId($id) {
   +    return $this->getContextValue('entity')->get('layout_builder__layout');
   

   Are there other places this should be used?

3. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageDefinition.php
   @@ -28,6 +32,13 @@ class SectionStorageDefinition extends PluginDefinition {
   +    if (isset($definition['context'])) {
   +      foreach ($definition['context'] as $name => $context_definition) {
   +        $this->addContextDefinition($name, $context_definition);
   +      }
   +      unset($definition['context']);
   +    }
   

   This deserves a code comment. It reminds me of ContextAwarePluginBase but not quite?

4. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/layout_builder_test.module
   @@ -58,3 +59,11 @@ function layout_builder_test_node_view(array &$build, EntityInterface $entity, E
   +  $storages['overrides']->setClass(OverridesSectionStorage::class);
   
   +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -0,0 +1,17 @@
   +    \Drupal::state()->set('layout_builder_test_context_applied', TRUE);
   +    return parent::getSectionListFromContext();
   

   Nice approach for the test!

See also #2995071: Refactor LayoutBuilderLocalTaskDeriver to delegate local tasks to plugins as another part of LB which needs to delegate to different plugins in turn.
(Slightly different as that's during build-time not run-time)

I need to step through this more to see how we might do it as a CoR pattern

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -239,11 +241,27 @@ public function buildMultiple(array $entities) {
+    if (empty($section_list) || count($section_list) === 0) {
+      $section_list = $this;
+    }

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
@@ -25,6 +25,9 @@
+ *   context = {
+ *     "entity_view_display" = @ContextDefinition("entity:entity_view_display"),
+ *   }

Discussed this more with @phenaproxima.
The confusing part for me was that I couldn't see how DefaultsSectionStorage would make it through filtering. The answer is, it won't. This last hunk with $section_list = $this was a special case representing defaults.

If we switch to a foreach loop over each plugin, call the new method, and check for truthiness before moving onto the next one.

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -239,11 +241,35 @@ public function buildMultiple(array $entities) {
+        $storage = $storage_manager->loadEmpty($storage_type);
+        $storage->setContext('entity', $contexts['entity']);
+        $section_list = $storage->getSectionListFromContext();

How would this work if a contrib module wanted to say provide the ability to use the layout builder in other view modes by providing something like a OverridesSectionViewModeStorage?

Since the entity is context would be the same it seems like whichever storage type came first would win out. So with the entity context contrib could never provide the storage type unless some it made sure its storage types for evaluated first.

Looks 'weight' logic was removed somewhere along the line

1. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -239,11 +241,35 @@ public function buildMultiple(array $entities) {
   +      // Loop through every section storage plugin until we find one that
   +      // derives a valid section list from the context values.
   

   Can we remove "valid" here. We just loop till we fine 1. there is not validation here.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -239,11 +241,35 @@ public function buildMultiple(array $entities) {
   +        $storage = $storage_manager->loadEmpty($storage_type);
   +        $storage->setContext('entity', $contexts['entity']);
   +        $section_list = $storage->getSectionListFromContext();
   

   I still don't see how a contrib module could use this have there storage take over for only a particular view mode.

   I did some step debugging and I don't see anyway for the storage plugin to know what the current view mode is. Am I missing something?

   Could we also provide mode and even displayContext as context or otherwise let the storage know about these.

3. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -36,6 +40,24 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +    uasort($definitions, function (SectionStorageDefinition $a, SectionStorageDefinition $b) {
   +      $a = [
   +        'weight' => $a->get('weight'),
   +      ];
   +      $b = [
   +        'weight' => $b->get('weight'),
   +      ];
   +      return SortArray::sortByWeightElement($a, $b);
   

   It seems weird weight as a sortable thing in plugin manager in a one off situation.

   I think it could easily lead to a developer looking at \Drupal\layout_builder\Annotation\SectionStorage and assume weight is a general thing that plugins support.

   Because if you look at the file there are weight is specific to this plugin type but the others aren't. So how would a developer know?

   parent::findDefinitions also already provides a way to reorder plugins.
   $this->alterDefinitions($definitions);
   couldn't you use that to reorder plugins?

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -526,4 +527,35 @@ public function testBlockPlaceholder() {
   +    $state_key = 'layout_builder_test_context_applied';
   ...
   +    $this->assertTrue($state->get($state_key));
   

   There should be some kind comment and/or see to the test module to explain why checking this state value actually tests what we say it tests here.

5. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -526,4 +527,35 @@ public function testBlockPlaceholder() {
   +    // Test that plugin weights are respected.
   +    /** @var \Drupal\layout_builder\SectionStorage\SectionStorageDefinition[] $definitions */
   +    $definitions = $this->container
   +      ->get('plugin.manager.layout_builder.section_storage')
   +      ->getDefinitions();
   +
   +    $this->assertSame('overrides', key($definitions));
   +    $this->assertSame(-10, $definitions['overrides']->get('weight'));
   +    $this->assertSame(0, $definitions['defaults']->get('weight'));
   

   Does this actually test that weights are respected? It just seems to test that you can get the property.

   It doesn't actually test that they are in the correct order, meaning the weights are respected.

6. Is there issue to make \Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay::preSave() not assume layout_builder__layout Because that seems like same kind of problem

1. +++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
   @@ -22,6 +22,30 @@ class SectionStorage extends Plugin {
   +   * The plugin weight.
   +   *
   +   * When an entity with layout is rendered, section storage plugins are
   +   * checked, in order of their weight, to determine which one should be used
   +   * to render the layout.
   +   *
   

   👍

2. +++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
   @@ -22,6 +22,30 @@ class SectionStorage extends Plugin {
   +   * When an entity with layout is rendered, all section storage plugins which
   +   * match a particular set of contexts are checked, in order of their weight,
   

   Maybe this is just general comment on the context system or my lack of understanding of it but..

   "match a particular set of contexts are checked" sounds weird. Because DefaultsSectionStorage is checked and it does not "match" the contexts in a sense that it is not aware of any contexts and will always be checked regardless of the what the entity context is.

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -134,6 +137,16 @@ public function getSectionListFromId($id) {
   +    return $this->getContextValue('entity')->get('layout_builder__layout');
   

   Since the 'view_mode' context is now available why don't we check for it here.

   Isn't the intention of core's functionality to only take over for 'default'?

Working on this.

Some semi-related issues:
#2273381: Convert ContextAwarePluginBase to traits
#2982183: Conflict between TypedDataManager and TypedConfigManager in ConfigEntityAdapter
#3008431: A context object for a specific entity type will not match a generic requirement for any entity

Here's a big chunk of changes.

Has @todos pointing to:
#3008924: Callers of LayoutEntityHelperTrait::getEntitySections() do not account for the view mode
#3008943: Clean up todo in InlineBlockEntityOperations::handleEntityDelete() and use isLayoutCompatibleEntity()
#3008431: A context object for a specific entity type will not match a generic requirement for any entity

Obsoletes #2986403: Create an easy way to get layout sections for an entity.

Not 100% sold on this, but wanted to post some progress

Includes #2982183 and #3008431, we need both to fix this I believe. Let's see how many fails are left.

Great! That passes. Now to push on those two blockers.

Posting an update with some changes. Still rolled on top of the above two issues.

The above patch includes #2986735: Drupal\Core\Plugin\Context\Context needs DependencySerializationTrait as well.

Added a soft-blocker: #3008943: Clean up todo in InlineBlockEntityOperations::handleEntityDelete() and use isLayoutCompatibleEntity()

Rerolled.

Here's a slimmed down version moving some code to #2986403: Create an easy way to get layout sections for an entity..

Once the final blocker lands, I will reupload the patch from #66, unless there are substantive reviews of that code between now and then.

All blockers are in!
Updated the comment about the sort to be more clear.

Thanks for the CR, but as written it points out one thing that is bothering me.

TL;DR, I think we have a naming problem

Looking at SectionStorageManagerInterface we have 3 methods:

1. loadEmpty($type)
   This is the main API entry point, it gives you an instance of the section storage of a given type.
   No contexts are provided, so many portions of the storage API will throw exceptions.
   The things that actually work here might as well be static methods (i.e. buildRoutes() and buildLocalTasks()).

   This does not call to $plugin->access().

2. loadFromRoute($type, $value, $definition, $name, array $defaults)
   This is how all the routing code gets the section storage of a given type.
   Either $value has a magic string (for the Choose Section/Block pages), or it is provided within the keys of $defaults (for the main LB page).
   The instance returned here has been populated with context objects.

   This does not call to $plugin->access().

3. loadFromContext(array $contexts)
   This is the new part. The biggest difference is that you do NOT specify the type.
   It loops through all available types until it finds the first plugin type that:
   a) has all of its context requirements satisfied
   b) returns TRUE from $plugin->access()

   It is the only method here to check access.
   It is the only method here to not be passed the $type.

Borrowing from the terminology of the larger plugin system, the first two methods are Factory methods.
(loadEmpty() is 1:1 a wrapper of FactoryInterface::createInstance and nothing more.)

loadFromContext() is more of a Mapper method, like MapperInterface::getInstance.
From those docs:

An array of options that can be used to determine a suitable plugin to instantiate and how to configure it.

The part that became clear to me was that we are missing a Factory method for when you know the type AND how to configure it.
If we were not adding these three custom methods to the plugin manager, we would have to rewrite them as follows:

loadEmpty($type) would become createInstance($type)
loadFromRoute($type, $value, $definition, $name, $defaults) would become createInstance($type, ['value' => $value, 'definition' => $definition, 'name' => $name, 'defaults' => $defaults])
loadFromContext($contexts) would become getInstance(['contexts' => $contexts])

Once again thinking about the larger plugin system, to me create vs get is not implicitly clear as a distinction between factory and mapper.
But even worse is turning all of them into load methods.

Finally, it is clear from the CR that we need a method that can be passed a $type as well as an array of $contexts.
As it turns out, we have that method in the form of protected function loadWithContextsApplied($type, array $contexts).
But that name is far too similar to loadFromContext().

// Factory
  public function loadEmpty($type);
  public function loadFromRoute($type, $value, $definition, $name, array $defaults);
  // To be made public
  protected function loadWithContextsApplied($type, array $contexts);

// Mapper
  public function loadFromContext(array $contexts);

Oh btw I also wrote a test-only implementation of SectionStorageInterface, it felt bad to not have a third implementation, or not to have any solidly non-entity implementations.

Proposed:

// Factory
  public function loadEmpty($type);
  public function loadFromRoute($type, $value, $definition, $name, array $defaults);
  public function loadFromContext($type, array $contexts);

// Mapper
  public function findByContext(array $contexts);

After discussion with @EclipseGc, I settled on:

// Factory
  public function load($type, array $contexts);
  public function loadEmpty($type);

// Mapper
  public function findByContext($operation, array $contexts);

This moves the special loadFromRoute() which was only called once to be inline.

Okay I'm done refactoring, I promise.
Updating the CR

Updated IS to reflect last round of naming

Thanks!

#78

1) Fixed
2) Fixed (and the other one in OverridesSectionStorage)
3) Commented and also slightly refactored (guard conditions ftw)
4) I like it. One less magic string (->get('weight'))
5) Fixed, in my own way
6) Fixed both

#81.1 It is abstract on the trait, so all of the rest of the trait has a single means to set the sections.
#81.2 Okay :)

This looks great couple questions/points. I haven't reviewed the test coverage yet.

settings to needs work for "extractEntityFromRoute" point

1. +++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
   @@ -22,6 +22,30 @@ class SectionStorage extends Plugin {
   +   * Any required context definitions, optional.
   ...
   +  public $context = [];
   

   Should this plural because there could be more than one?

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -274,11 +276,21 @@ public function buildMultiple(array $entities) {
   +    $sections = NULL;
   ...
   +    // If we don't have a section list yet (i.e., no section storage plugin
   +    // was able to derive a section list from context, or this display is not
   +    // overridable), use this display as the section list.
   +    return $sections ?: $this->getSections();
   

   Should $sections here be renamed $overridden_sections it would make condition statement in the return make more sense. Because it seems the logic is "if no overridden sections available call getSections"

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -108,30 +106,42 @@ public function getStorageId() {
   +  public function getContextsFromRoute($value, $definition, $name, array $defaults) {
   

   I think extractEntityFromRoute() can now be moved into SectionStorageBase because the implementations in OverridesSectionStorage and DefaultsSectionStorage are identical.

4. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -79,27 +50,7 @@ public function getSectionListFromId($id);
   +   * Derives the required plugin contexts from route values.
   

   Would this always be *required* contexts? What if there was SectionStorage plugin that had option contexts that would inform which override would be in effect?

@tim.plunkett filed #3013773: Document LayoutBuilderEntityViewDisplay::getRuntimeSections() and consider renaming it for clarifying the purpose of the method that contains this bug.

1. +++ b/core/modules/layout_builder/src/Annotation/SectionStorage.php
   @@ -22,6 +22,30 @@ class SectionStorage extends Plugin {
   +  public $weight = 0;
   

   I get what you're doing here and generally approve. That being said, could we go with "priority" instead and flip the sorting logic? This would bring it in line with event dispatching and reduce the number of things we have that are different sorting mechanisms in core by 1. Not a cliff to die on, but I think it'd be nice to have.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -23,6 +21,10 @@
   + *     "view_mode" = @ContextDefinition("string", required = FALSE),
   

   I don't see this actually used in the plugin anywhere. Oversight of some sort? I only see "$this->getContextValue('view_mode')" in the test coverage. I also don't see it returned in the getContextsFromRoute() method. Maybe that's expected, but I was expecting we'd derive all of all of that.

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -108,30 +106,42 @@ public function getStorageId() {
   +    $entity = $this->entityTypeManager->getStorage($entity_type_id)->load($entity_id);
   

   Seems like we could figure out whether or not this entity type is fieldable much earlier. Maybe that's not necessary, but none of this effort needs to happen if it's not a content entity in the first place since the override plugin can't deal with non-content-entities.

4. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -39,33 +52,53 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   +  public function loadEmpty($type) {
   +    return $this->createInstance($type);
   

   This is interesting. I have no objections to what you're doing here, but per other conversations we've had I think we should revisit some of this in general as we begin to move toward D9.

All in all I like this patch. It opens the door to do cool new stuff (like layout_builder for views as an example). Overall the patch looks good too. Mostly I'm just nitpicking here.

Eclipse

re #87

1. I would second moving to priority unless we are using "weight" in relation to other plugins but I don't think we are. This is how normalizers work.
2. @see #43.2

#85
1) Let's go one step further, since this isn't context, they are context definitions

2) Okay

3) It could for these two cases, but is not used by the new third (test-only) implementation. I'd prefer to keep entity-specific code out of the base class.

4) s/required/available

#87

1) (and #88/89) I'd much rather stick with weight here. Priority to me denotes Symfony events, which these are not.

2) As 88 says, 43.2 explains the use case. And we have tests... It is similar to why we pass as much information to calls to getFilteredDefinitions()

3) We actually do check this much earlier, see OverridesSectionStorage::getEntityTypes() which checks $entity_type->entityClassImplements(FieldableEntityInterface::class)
This instanceof check is really just functioning as a fancy inline @var :)

4)
Earlier in the patch, there is

-  public function loadEmpty($id) {
-    return $this->createInstance($id);

We're just renaming $id to $type here for consistency with the new load($type, array $contexts = [])

I think the #85 and #87 has been addressed.

Great work!

This patch includes a hard break with the removal of four methods and will change how any storage plugins are implemented, so we probably should mention it in the release notes.

I asked whether it would make sense to use trigger_error()( (without the @, for broken/unintended behavior).

Per @tim.plunkett there's no real way around the break as the old code paths would be broken anyway by this change. Existing storage plugins will fatal anyway because they need to implement the new methods, and only an actual alternate layout building UI would be calling the methods externally, so coming up with a contorted way to raise deprecations before throwing an exception isn't really worthwhile either.

Just a few notes/nits so I don't lose them; still reading the patch.

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -202,3 +202,15 @@ function layout_builder_block_content_access(EntityInterface $entity, $operation
   +  // The \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage
   +  // plugin defines the entity context via its annotation but cannot specify the
   +  // constraint inline.
   

   Both @phenaproxima and @tim.plunkett explained this to me at some point but I still don't understand it from the comment.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -237,34 +234,49 @@ protected function getEntityTypes() {
   +   * @param mixed $value
   +   *   The raw value from the route.
   

   Can we really not be any more specific than mixed here? From the method body it looks like it's a string|null?

   Edit: Sorry, should have highlighted that this is for:

   +  protected function extractEntityFromRoute($value, array $defaults) {
   

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -16,40 +15,17 @@
   -  protected function getSectionList() {
   -    if (!$this->sectionList) {
   -      throw new \RuntimeException(sprintf('%s::setSectionList() must be called first', static::class));
   -    }
   -    return $this->sectionList;
   -  }
   +  abstract protected function getSectionList();
   

   This change isn't in the CR (that SectionStorageBase no longer provides a default implementation of this method).

4. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -103,4 +79,22 @@ public function removeSection($delta) {
   +   * @todo Remove after https://www.drupal.org/project/drupal/issues/2982626.
   

   This issue is RTBC so we can probably reroll to remove these and make the patch smaller, yay!

FWIW, all my issues were addressed to my satisfaction.

Eclipse

Working on this

One other thing (point 2 below):

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -23,6 +21,10 @@
   + *     "view_mode" = @ContextDefinition("string", required = FALSE),
   

   Huh. I have never seen this before (the required property for an annotation attribute. This is from Doctrine and used once or twice already for context stuff. Just not anywhere else.) :)

2. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManager.php
   @@ -39,33 +52,53 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
      /**
       * {@inheritdoc}
       */
   -  public function loadEmpty($id) {
   -    return $this->createInstance($id);
   ...
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function loadEmpty($type) {
   +    return $this->createInstance($type);
      }
   

   Seems like the only change here is internally changing the parameter name from $id to $type; is this in scope other than consistency with the naming for load() (which is new in this patch anyway)?

+++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManagerInterface.php
@@ -15,51 +20,46 @@
-  public function loadFromRoute($type, $value, $definition, $name, array $defaults);

This method removal is also missing from the CR. (At first I thought this might be an artifact of the goofy diff but it doesn't exist anymore with the patch applied AFAICS.)

1. +++ b/core/modules/layout_builder/src/SectionStorage/SectionStorageManagerInterface.php
   @@ -7,6 +7,11 @@
   + * Note that this interface purposefully does not implement
   + * \Drupal\Component\Plugin\PluginManagerInterface, as the below methods exist
   + * to serve the use case of \Drupal\Component\Plugin\Factory\FactoryInterface
   + * and \Drupal\Component\Plugin\Mapper\MapperInterface.
   

   This is also unclear from context (pun!).

2. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -79,27 +50,7 @@ public function getSectionListFromId($id);
   -  public function getRedirectUrl();
   ...
   -  public function getLayoutBuilderUrl($rel = 'view');
   
   @@ -110,21 +61,30 @@ public function getLayoutBuilderUrl($rel = 'view');
   +  public function getRedirectUrl();
   ...
   +  public function getLayoutBuilderUrl($rel = 'view');
   

   These aren't actually being removed; just a diff artifact

3. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -110,21 +61,30 @@ public function getLayoutBuilderUrl($rel = 'view');
   -  public function getContexts();
   

   But this one appears to be real and is missing.

4. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -557,4 +558,58 @@ public function testBlockPlaceholder() {
   +  /**
   +   * Tests that the test implementation of Layout Builder works as expected.
   +   */
   +  public function testSimpleConfigBasedLayout() {
   

   This is not a great docblock. :P

5. +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -557,4 +558,58 @@ public function testBlockPlaceholder() {
   +    // The pre-existing section is found.
   +    $this->drupalGet('layout-builder-test-simple-config/existing');
   +    $assert_session->elementsCount('css', '.layout', 1);
   +    $assert_session->elementsCount('css', '.layout--twocol', 1);
   +
   +    // The default layout is selected for a new object.
   +    $this->drupalGet('layout-builder-test-simple-config/new');
   +    $assert_session->elementsCount('css', '.layout', 1);
   +    $assert_session->elementsCount('css', '.layout--onecol', 1);
   

   I didn't see this in the fixtures; is it being inherited? If so should it be documented?

Good test coverage.

#101
2) The name of the parameter doesn't affect anything other than readability/consistency, and yes it was changed to match the new load().

#102
Fixed

#103
1) This was added in response to #78.5. I'm usually against adding comments explaining why we didn't do something or why someone shouldn't refactor it in the future, but I did so anyway. If you explain which part is unclear, maybe we can improve it?

3) As #104 says, it's now on a parent interface

4) Fixed

5) What do you mean?
The existing one is set up 2 lines above the code you highlighted, and the new one does not exist yet, hence the name.
The route is defined by \Drupal\layout_builder_test\Plugin\SectionStorage\SimpleConfigSectionStorage::buildRoutes().

Additionally, @phenaproxima and I realized that the access check for overrides was incomplete, it should check for emptiness when being used to view the layout, but can continue to return when used by the update UI.
Also, the context declaration on DefaultsSectionStorage clashed with the one on OverridesSectionStorage, but was being masked by the extra checks in getRuntimeSections. Fixed that up.

And finally, I had to move testRenderByContextAwarePluginDelegate() out to a new test class as the fixtures it needed were interfering with other test methods.

#3014871: Adapt to new changes in Layout Builder via the delegation issue is the Layout Library issue to implement these changes. The good news is that it works great, and Layout Library can finally function!

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -16,40 +15,17 @@
   +  abstract protected function getSectionList();
   

   Can you elaborate on why this is now abstract, this is an API change which would mean we would be wary of doing this in a minor. Any reason we can't do a fallback of sorts and trigger a deprecation error or similar?

2. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -110,21 +61,30 @@ public function getLayoutBuilderUrl($rel = 'view');
   +  public function getContextsFromRoute($value, $definition, $name, array $defaults);
   

   Can we provide a base implementation in SectionStorageBase for this? Would minimize API changes in minors, would mean we could consider backporting.

3. +++ b/core/modules/layout_builder/tests/modules/layout_builder_test/src/Plugin/SectionStorage/SimpleConfigSectionStorage.php
   @@ -0,0 +1,211 @@
   +class SimpleConfigSectionStorage extends ContextAwarePluginBase implements SectionStorageInterface, SectionStorageLocalTaskProviderInterface, ContainerFactoryPluginInterface {
   

   aka layout_library_lite in core ;), kind of like contact_storage

#108

1) Before we relied on the section list being injected into the plugin, and this was a simple getter.
Now this method must rely only on the context.

Defaults has a context named display with the type entity:entity_view_display

  protected function getSectionList() {
    return $this->getContextValue('display');
  }

Overrides has a context named entity with the generic type entity but an alter does ->addConstraint('EntityHasField', 'layout_builder__layout')

  protected function getSectionList() {
    return $this->getContextValue('entity')->get('layout_builder__layout');
  }

Between this required change and the necessary changes to SectionStorageManager, a BC layer is essentially impossible.

2) Continuing from above, I don't think that really helps much. If anything, not having it masked by an empty implementation will help clarify what is needed when fixing code.

3) In a sense, yes. I was really uncomfortable making more changes without a 3rd implementation to go against, and specifically wanted one that wasn't 100% tied to entities.

OK - the need to implement protected function getSectionList() isn't listed in the change record.

I will defer to a release manager on the changes

Updated the IS a bit to explain why we cannot provide more of a BC layer here. Updating the CR now to address #110

1. This comment was also confusing to me and chatted with @tim.plunkett about this. I think making this comment clear would be too complicated and explains the details how this patch changed which is unnecessary.
   I think it would be clearer to just delete this comment and add a comment to SectionStorageManager that explains that plugins should loaded via the methods on SectionStorageManagerInterface instead of via the methods on PluginManagerInterface or related.
2. re #103.5 @xjm's question

   I didn't see this in the fixtures; is it being inherited? If so should it be documented?

   This test using the SimpleConfigSectionStorage storage manager which makes these routes. Since it just relies on simple config
   This code

   // Prepare an object with a pre-existing section.
       $this->container->get('config.factory')->getEditable('layout_builder_test.test_simple_config.existing')
         ->set('sections', [(new Section('layout_twocol'))->toArray()])
         ->save();
   

   Is all that is needed.

   I think this test should at least but a @see to SimpleConfigSectionStorage since this confusing where the route is coming from and that we don't need anything else setup.

Great, thanks!

Reroll for #2936360: Remove duplicate references to the "layout_builder__layout" field from Layout Builder, no other changes

If we didn't have the clash between the old \Drupal\layout_builder\SectionStorageInterface::getContexts() and the newly implemented (but way pre-existing) \Drupal\Component\Plugin\ContextAwarePluginInterface::getContexts(), it might have been possible to attempt a more graceful API break for existing plugins. (The full break for SectionStorageManagerInterface is not avoidable.)

But since the entire point of the issue is to make these plugins context aware, even if we were able to maintain BC for existing plugins, NONE of them would be loaded by the new SectionStorageManagerInterface::findByContext(), thereby breaking their use cases anyway.

reread all comments and patches since #102 when it was set to Needs work from RTBC. I think everything has been addressed.

Also reread the CR. Made 2 small changes.

Tagging per #110. This change is a hard break so we need to make sure we're confident about the BC break.

Can we add a parargaph summarizing the change and impacts for the release notes? See the "Important Update Information" in release notes of recent minors for examples of what to write, e.g.:
https://www.drupal.org/project/drupal/releases/8.5.0

Would it be reasonable to split the CR into 2? One for those providing implementations of SectionStorage, one for those interacting with the API externally?

I think two CRs is a great idea and would help disentangle the info for developers.

Potential CR intro and first part of the release note:

The Layout Builder module is designed to support extensible storage for layouts, so that layouts can be created and saved for various usecases. (For example, the core module has two storages, one for default content layouts and another for individual entity layouts.) However, a serious bug with the the beta API prevents other storages from [XYZ].

And then for the release notes we could say:

If your module extends the layout builder with a custom SectionStorage implementation, blah with a link to that CR. If your module does blah blah blah with the API, blah blah other CR.

NW for the work on the CRs and the release notes; I think we're on a good track for that now.

Repurposed https://www.drupal.org/node/3012353 to be about the section storage manager changes, and wrote https://www.drupal.org/node/3016262 specifically for those providing existing plugins.

Added a release notes section to the IS

While writing the new CR, I encountered an oversight from before: while getContexts() should have been dropped from the plugins, it was not and was only not broken because nothing currently calls the new getContexts() method from the parent ContextAwarePluginBase class. Fixed that by introducing a new name for the old method, getContextsDuringPreview().
Added tests for this change.

Finally, removed workarounds for #2982626: ContextAwarePluginBase is incompatible with ContextAwarePluginDefinitionInterface now that it landed.

Good catch on #121. Just a few notes before I go to sleep:

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -203,3 +203,15 @@ function layout_builder_block_content_access(EntityInterface $entity, $operation
   +  // The \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage
   +  // plugin defines the entity context via its annotation but cannot specify the
   +  // constraint inline.
   

   It's still unclear to me what "but cannot specify the constraint inline" means or why that means we need this alter.

2. Was my review from #98 addressed? (The above was raised back there.)
3. In https://www.drupal.org/node/3016262:
   

   Corresponding to this change is the new protected method getSectionList(). This method is responsible for returning a section list, and should derive it from the context values available.

   It's not new; it's just a new requirement for each storage plugin to implement it individually. Previously there was a different implementation, but as described in #109 we can no longer rely on that default implementation because the section list is no longer injected. So this is one of the other changes an affected module must make.

I missed #95, will address that and the rest of #123 tomorrow

In #121 I added an optional param to getAvailableContexts(), and changed one call to pass TRUE. But in reality, it doesn't need the param, all of the calls need the new code path.

The fail patch in #121 is correct though.

#95.1
I opened #3016420: Allow context definition annotations to specify constraints and adjusted this comment.

#95.2
This matches the docblock of \Drupal\Core\ParamConverter\ParamConverterInterface::convert(), and while it is always string|null in these implementations, it could also be int... Or anything, really.
I added an @see to convert() on deriveContextsFromRoute()

#95.3
That was added to the CR

#95.4
Just done recently

#123.1/2 point to #95

#123.3
Fair point https://www.drupal.org/node/3016262/revisions/view/11206150/11207096

Also fixed the spare `use` statement.

Split out the refactor from here, which will leave this issue to actually use the new API and fix the bug.

Here's a patch for after #3016473: Allow a single SectionStorage plugin to be returned for a set of available contexts goes in.

Moved relevant tags to #3016473: Allow a single SectionStorage plugin to be returned for a set of available contexts

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -266,20 +266,13 @@ public function buildMultiple(array $entities) {
-  protected function getRuntimeSections(FieldableEntityInterface $entity) {
-    if ($this->isOverridable() && !$entity->get(OverridesSectionStorage::FIELD_NAME)->isEmpty()) {
-      return $entity->get(OverridesSectionStorage::FIELD_NAME)->getSections();
-    }
-
-    return $this->getSections();
+  private function sectionStorageManager() {

Rather than removing this method, we should remove the usage but change it to wrap:

@@ -240,8 +241,14 @@ public function buildMultiple(array $entities) {
+      $contexts = [
+        'view_mode' => new Context(ContextDefinition::create('string'), $this->getMode()),
+        'entity' => EntityContext::fromEntity($entity),
+        'display' => EntityContext::fromEntity($this),
+      ];
+      $contexts += $this->contextRepository()->getAvailableContexts();
+      $storage = $this->sectionStorageManager()->findByContext('view', $contexts);
+      if ($storage && $sections = $storage->getSections()) {

And then deprecate it and add a CR.

It's internal as a protected method, but:
https://www.drupal.org/core/deprecation#internal

While calling the method would now be expensive, expensive is better than broken for the 0-1 users out there who might be doing this. We shouldn't pick and choose where to apply the deprecation policy and we should only have breaks where it's a hard requirement to do so.

I discussed this with @tim.plunkett and he disagreed, but I feel pretty strongly it's important to follow the deprecation process consistently wherever and whenever possible.

I keep meaning to set this to be a bug. The API was designed to support multiple, extensible storages, but because of this bug it's impossible to implement one effectively.

#3016473: Allow a single SectionStorage plugin to be returned for a set of available contexts is in now, yay! So setting this to NW to be rerolled on top of that & with all the misc. things we scoped to this issue.

Re-re-re-reroll!
Also expanded the actual delegation test because the existing version was inconclusive IMO.

The FAIL patch has the weights removed from the Overrides and Defaults annotations, to prove that it actually matters now.
Drupal\Tests\layout_builder\Functional\LayoutBuilderTest::testOverrides() should fail as that explicitly tests that functionality. Many more tests will also likely fail due to implicitly relying on the behavior.

+++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
@@ -275,11 +291,9 @@ public function buildMultiple(array $entities) {
   protected function getRuntimeSections(FieldableEntityInterface $entity) {
-    if ($this->isOverridable() && !$entity->get(OverridesSectionStorage::FIELD_NAME)->isEmpty()) {
-      return $entity->get(OverridesSectionStorage::FIELD_NAME)->getSections();
-    }
-
-    return $this->getSections();
+    $contexts = $this->getContextsForEntity($entity);
+    $storage = $this->sectionStorageManager()->findByContext('view', $contexts);
+    return $storage ? $storage->getSections() : [];
   }

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -332,6 +333,12 @@ public function save() {
   public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
     $default_section_storage = $this->getDefaultSectionStorage();
     $result = AccessResult::allowedIf($default_section_storage->isLayoutBuilderEnabled())->addCacheableDependency($default_section_storage);
+
+    // Ensure there are sections available during view.
+    if ($operation === 'view') {
+      $overrides_available = ($default_section_storage->isOverridable() && count($this));
+      $result = $result->andIf(AccessResult::allowedIf($overrides_available))->addCacheableDependency($this->getEntity());
+    }
     return $return_as_object ? $result : $result->isAllowed();
   }

+++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
+++ b/core/modules/layout_builder/tests/src/Kernel/OverridesSectionStorageTest.php
@@ -102,9 +102,13 @@ public function providerTestAccess() {

@@ -102,9 +102,13 @@ public function providerTestAccess() {
-    $data['view, enabled, no data'] = [TRUE, 'view', TRUE, []];
+    $data['view, enabled, no data'] = [FALSE, 'view', TRUE, []];

This is the key change of this issue. Before, OverridesSectionStorage::access() would return TRUE even if it was empty, and it relied on the hardcoded logic in getRuntimeSections to handle this check. Now that logic is correctly made completely internal and it will properly check its own access.

This necessitates the split into view and update operations, because overrides must be able to provide a UI even when there is no data (i.e. when trying to *add* data!)

1. It's great to see the fail-coverage in #137, this (and the kernel I eventually found at the end of the patch from #3016473: Allow a single SectionStorage plugin to be returned for a set of available contexts) address my questions from #3016473-63: Allow a single SectionStorage plugin to be returned for a set of available contexts.2 and .5.

2. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -249,13 +253,6 @@ public function buildMultiple(array $entities) {
   -        $contexts['layout_builder.entity'] = EntityContext::fromEntity($entity, $label);
   

   Ahh, there's the line that went into our workaround for getContextsDuringPreview().

3. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -265,6 +262,25 @@ public function buildMultiple(array $entities) {
   +  /**
   +   * Gets the available contexts for a given entity.
   +   *
   +   * @param \Drupal\Core\Entity\FieldableEntityInterface $entity
   +   *   The entity.
   +   *
   +   * @return \Drupal\Core\Plugin\Context\ContextInterface[]
   +   *   An array of context objects for a given entity.
   +   */
   +  private function getContextsForEntity(FieldableEntityInterface $entity) {
   +    return [
   +      'view_mode' => new Context(ContextDefinition::create('string'), $this->getMode()),
   +      'entity' => EntityContext::fromEntity($entity),
   +      'display' => EntityContext::fromEntity($this),
   +      // @todo Remove in https://www.drupal.org/project/drupal/issues/3018782.
   +      'layout_builder.entity' => EntityContext::fromEntity($entity),
   +    ] + $this->contextRepository()->getAvailableContexts();
   +  }
   

   Private again. Are we really sure we don't want to allow a child class to use this helper in its own context/section handling?

4. +++ b/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php
   @@ -399,4 +413,14 @@ protected function getDefaultSection() {
   +  private function sectionStorageManager() {
   

   This as private totally makes sense.

5. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -23,6 +23,7 @@
     * @SectionStorage(
     *   id = "defaults",
   + *   weight = 20,
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -21,6 +21,7 @@
     * @SectionStorage(
     *   id = "overrides",
   + *   weight = -20,
   

   Can we add a line to the docblock above these why we picked each weight? (Because it needs to override/be overridden).

6. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -332,6 +333,12 @@ public function save() {
      public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
        $default_section_storage = $this->getDefaultSectionStorage();
        $result = AccessResult::allowedIf($default_section_storage->isLayoutBuilderEnabled())->addCacheableDependency($default_section_storage);
   +
   +    // Ensure there are sections available during view.
   +    if ($operation === 'view') {
   +      $overrides_available = ($default_section_storage->isOverridable() && count($this));
   +      $result = $result->andIf(AccessResult::allowedIf($overrides_available))->addCacheableDependency($this->getEntity());
   +    }
        return $return_as_object ? $result : $result->isAllowed();
      }
   

   I'm still a little confused by this. So by default, access is granted if Layout Builder is enabled for the entity type. And then if the operation is view, then it's allowed if the entity is overridable. I guess this is special because layout building happens on the frontend when you're literally viewing the parent entity? I think the comment should say that. And it still sounds like "access is granted to everyone if the module is enabled!!" so we might want to document where else access control happens.
    

7. I was expecting to see a change to the problematic code hardcoding the two storages in the IS in this issue. It doesn't seem to be touching that class anymore. This also wasn't one of the things we deprecated AFAIK. What happened to that part?

3) This is an entity class that we're _already_ swapping out. If someone needs to in turn swap out all of LB, they can. We can always make something protected later, but we can't go the other direction.

5) Writing the docs for Overrides makes sense. But when I went to write it for defaults, I realized it doesn't make sense for it to override the default value at all. After all, it is the default.
Though on rereading the patch, it feels _really silly_ to include that comment, as it is describing the entire purpose of the property which is documented on that property in the annotation...

6) Expanded comments and linked to #2914486: Add granular permissions to the Layout Builder as well.

7) That's in there, 5th hunk in LayoutBuilderEntityViewDisplay. I highlighted it in #139.

Though on rereading the patch, it feels _really silly_ to include that comment, as it is describing the entire purpose of the property which is documented on that property in the annotation...

Sorry, I meant why the specific integers were chosen. Weights are magic numbers; this is a problem basically as old as Drupal. So explaining why we specific magic number is chose and how it relates to the other magic numbers is important, even though the general purpose of the weight's existence is already documented elsewhere (when we've done our jobs).

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -19,6 +19,8 @@
+ * The weight is set to ensure it runs earlier than other implementations.

"Runs earlier" isn't really accurate. Only one storage "runs" (is picked off the top of the list).

So for overrides I was thinking something like:

The OverridesSectionStorage uses a negative weight to ensure it is picked over other implementations, since customizing an individual entity takes precedence. It must have a lighter weight than the DefaultSectionStorage.

@see \Drupal\blah\DefaultSectionStorage

For the default storage's weight, maybe it's useful to think about other hypothetical implementations. Zero should be whatever we want the default behavior to be for a custom implementation. If someone creates their own storage, what are the chances that they need their storage to override the defaults? What are the chances that they want defaults to override their storage? What are the chances that they don't care and we want it to just be whatever's first in the list according to that internal logic about when things are the same weight?

To me it seems like a positive integer for the default makes sense, because most custom implementations' default behavior should usually be to override the defaults when they're relevant for the context. If they want to also trump individual entity overrides, then they can go ahead and set a more negative weight, but that seems less common than the "almost always" that their implementation might want to override the bundle defaults.

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -441,7 +440,13 @@ public function getThirdPartyProviders() {
   +    // For all operations ensure that the Layout Builder is enabled for this
   +    // display.
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -331,14 +333,21 @@ public function save() {
   +    // For all operations ensure that the Layout Builder is enabled for the
   +    // corresponding default to this override.
   

   I'd reword this as:

   For all operations, access should only be granted if the Layout Builder is enabled for this display.

   And similarly:

   For all operations, access should only be granted if the Layout Builder is enabled for this display that this override corresponds to.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -441,7 +440,13 @@ public function getThirdPartyProviders() {
   +    //   https://www.drupal.org/node/2914486. For now all access is controlled
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -331,14 +333,21 @@ public function save() {
   +    //   https://www.drupal.org/node/2914486. For now all access is controlled
   

   Nit: "For now, all access..."

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -441,7 +440,13 @@ public function getThirdPartyProviders() {
   +    //   at the routing level by the 'configure any layout' permission.
   
   +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -331,14 +333,21 @@ public function save() {
   +    //   at the routing level by the 'configure any layout' permission.
   

   Should we @see the relevant code?

4. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -331,14 +333,21 @@ public function save() {
   -    // Ensure there are sections available during view.
   +    // When viewing ensure that overrides are enabled and that there are
   +    // sections available.
   

   And maybe:

   Individual entity overrides are edited in the user interface when viewing an entity. For this reason, we grant access during the entity "view" operation, if overrides are enabled for the corresponding entity display and there are sections available.