Ensure that Layout Builder Inline Blocks doesn't assume section storage internals

This blocks a major so let's make it major as well :)

1. +++ b/core/modules/layout_builder/src/EventSubscriber/SetInlineBlockDependency.php
   @@ -127,7 +131,7 @@ protected function getInlineBlockDependency(BlockContentInterface $block_content
   -    if ($this->isLayoutCompatibleEntity($layout_entity)) {
   +    if ($this->getSectionStorageForEntity($layout_entity) !== NULL) {
   
   +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -144,20 +150,16 @@ public function handleEntityDelete(EntityInterface $entity) {
   -    if (!$this->isLayoutCompatibleEntity($entity)) {
   +    if ($this->getSectionStorageForEntity($entity) === NULL) {
   
   +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -16,17 +20,11 @@
   -  protected function isLayoutCompatibleEntity(EntityInterface $entity) {
   

   Can we keep this helper method around and change it to make this exact comparison?

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -128,13 +128,19 @@ protected function getRemovedBlockIds(EntityInterface $entity) {
   +   * We cannot call ::getSectionStorageForEntity() for the deleted entity
   +   * because validating the contexts would throw an exception trying to load the
   +   * deleted entity. Therefore we most attempt to remove the layout usage
   +   * tracking for all deleted entities.
   ...
   +   * @see \Drupal\Core\Entity\Plugin\Validation\Constraint\ValidReferenceConstraintValidator::validate
   

   Instead of this @see, can this be an @todo linking to #3008943: Clean up todo in InlineBlockEntityOperations::handleEntityDelete()?

3. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -128,13 +128,19 @@ protected function getRemovedBlockIds(EntityInterface $entity) {
   +   * @see \Drupal\layout_builder\LayoutEntityHelperTrait::getSectionStorageForEntity
   

   Not sure about this @see, but if you keep it, add the () to the end please

4. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -264,4 +266,27 @@ protected function saveInlineBlockComponent(EntityInterface $entity, SectionComp
   +    if ($entity instanceof FieldableEntityInterface) {
   

   In theory I understand this check, but none of the code after it really cares if it is fieldable or not...

5. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -264,4 +266,27 @@ protected function saveInlineBlockComponent(EntityInterface $entity, SectionComp
   +        if ($original_section_storage instanceof DefaultsSectionStorage) {
   

   Let's use the interface here, not the class

6. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -94,16 +87,33 @@ protected function getInlineBlockComponents(array $sections) {
   +    $contexts = [
   +      'entity' => EntityContext::fromEntity($entity),
   +    ];
   ...
   +      $contexts['entity'] = EntityContext::fromEntity($entity);
   

   Added twice

7. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -94,16 +87,33 @@ protected function getInlineBlockComponents(array $sections) {
   +    $view_mode = 'full';
   

   This should have an @todo to #3008924: Callers of LayoutEntityHelperTrait::getEntitySections() do not account for the view mode

Thanks for the changes!

A couple nits and one question

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -132,9 +130,9 @@ protected function getRemovedBlockIds(EntityInterface $entity) {
   +    // @todo In https://www.drupal.org/node/3008943 call
   +    // \Drupal\layout_builder\LayoutEntityHelperTrait::isLayoutCompatibleEntity()
   
   +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -94,16 +100,32 @@ protected function getInlineBlockComponents(array $sections) {
   +    // @todo Take into account other view modes in
   +    // https://www.drupal.org/node/3008924
   

   Nit: second line should be indented two spaces and end with a trailing space.

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -264,4 +258,28 @@ protected function saveInlineBlockComponent(EntityInterface $entity, SectionComp
   +    if ($section_storage instanceof OverridesSectionStorageInterface) {
   +      if (!$entity->isNew() && isset($entity->original)) {
   

   Nit: could be combined into one to cut down on nesting

3. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -94,16 +100,32 @@ protected function getInlineBlockComponents(array $sections) {
   -  protected function isEntityUsingFieldOverride(EntityInterface $entity) {
   -    return $entity instanceof FieldableEntityInterface && $entity->hasField(OverridesSectionStorage::FIELD_NAME);
   

   Do we need to provide this method still? If so, could be an instanceof check like the one in entityHasNewOverride()

+++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
@@ -132,9 +130,9 @@ protected function getRemovedBlockIds(EntityInterface $entity) {
+    // @todo In https://www.drupal.org/node/3008943 call
+    // \Drupal\layout_builder\LayoutEntityHelperTrait::isLayoutCompatibleEntity()

Missed one of the @todos from #17.1 to fix

#18.3, good point.

Does this mean #2986403: Create an easy way to get layout sections for an entity. should be closed as a duplicate?

Thanks for the update. Confirmed that isEntityUsingFieldOverride was added back in #22 which matches the interdiff. Is there any way to manually test this?

Great! Thanks for adding the deprecation and test.
One additional thing: deprecations are supposed to have CRs associated with them in the trigger_error message. I know this was only recently added but it was in 8.6.0 and it's on a trait.

Thanks!

Patch looks good to me, just a few questions:

1. +++ b/core/modules/layout_builder/src/EventSubscriber/SetInlineBlockDependency.php
   @@ -64,11 +65,14 @@ class SetInlineBlockDependency implements EventSubscriberInterface {
   -  public function __construct(EntityTypeManagerInterface $entity_type_manager, Connection $database, InlineBlockUsage $usage) {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, Connection $database, InlineBlockUsage $usage, SectionStorageManagerInterface $section_storage_manager) {
        $this->entityTypeManager = $entity_type_manager;
        $this->database = $database;
   

   Shouldn't we remove the $database connection property and the related constructor param, while we are at this?

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -47,13 +47,14 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
   +  public function __construct(EntityTypeManagerInterface $entityTypeManager, InlineBlockUsage $usage, SectionStorageManagerInterface $section_storage_manager) {
   

   Was the $database param unused?

3. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -132,9 +130,9 @@ protected function getRemovedBlockIds(EntityInterface $entity) {
   +    // @todo In https://www.drupal.org/node/3008943 call
   +    //   \Drupal\layout_builder\LayoutEntityHelperTrait::isLayoutCompatibleEntity().
   +    $this->usage->removeByLayoutEntity($entity);
   

   Why are we doing this here?

4. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -264,4 +258,26 @@ protected function saveInlineBlockComponent(EntityInterface $entity, SectionComp
   +  protected function entityHasNewOverride(EntityInterface $entity)
   

   Would it make sense to move this into LayoutEntityHelperTrait?

Ok, thanks

Oh good point, let's not burn ourselves again

So:

We should preferably use private for things that, architecturally, should be private, not to avoid providing BC. Increasing the visibility of something later is a disruptive change because it can cause method name collisions. So it's not a total freebie in terms of BC/disruption. (I discussed this with @plach and he was hesitant for policy reasons as well, i.e. our official core policy is still "we never use private, only protected" and we definitely don't have any sort of policy for using private as a substitution for BC or deprecation.)

Furthermore, regardless of that, we should have an @todo linking an issue for what needs to be changed later if we think it needs to be changed later.

Oh great, that makes much more sense, and no need for a follow-up now. +1 on the new name too!

This issue has two CRs that look to be about the same thing; which is preferred?

https://www.drupal.org/node/3030609
https://www.drupal.org/node/3028211

+++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
@@ -47,13 +47,14 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
-   * @param \Drupal\Core\Database\Connection $database
-   *   The database connection.
+   * @param \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface $section_storage_manager
+   *   The section storage manager.
    */
-  public function __construct(EntityTypeManagerInterface $entityTypeManager, InlineBlockUsage $usage, Connection $database) {
+  public function __construct(EntityTypeManagerInterface $entityTypeManager, InlineBlockUsage $usage, SectionStorageManagerInterface $section_storage_manager) {

@@ -63,7 +64,7 @@ public static function create(ContainerInterface $container) {
-      $container->get('database')
+      $container->get('plugin.manager.layout_builder.section_storage')

I don't think removing the DB parameter is in scope, is it?

At least the typehints are different so it will cleanly fatal vs. having weird bugs, but we are moving away from constructor breaks in general even for "internalish" code. See #3030640: [policy, documentation] Clarify Constructor BC policy and document best practices for subclassing other classes.

A related followup woulod be to explain the "why" in the @internal. Is it because LB is experimental? Because subclassing this is a totally bad idea? Etc.

...Clarifying #38, the "more BC" way to change the constructor is to keep the DB service param, and make the new section storage param optional with a fallback to \Drupal::service() that @trigger_error()s.

+++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
@@ -43,18 +44,31 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
+   * @todo This constructor has 1 optoinal parameter, $section_storage_manager,
+   *    and 1 totally unused $database parameter. Deprecate the current

"Optoinal" sounds like a non-Western musical scale. ;)

Also, nit: we should spell out the word "one" in a sentence. (I think the rule they taught me in grade school was to spell out numbers less than or equal to ten.)

Interdiff looks good, all of the feedback since the last RTBC seems to have been addressed.

Drupal\Tests\media\FunctionalJavascript\MediaStandardProfileTest random fail

+++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
@@ -43,17 +44,31 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
+    if ($section_storage_manager === NULL) {
+      $this->sectionStorageManager = \Drupal::service('plugin.manager.layout_builder.section_storage');
+    }
+    else {
+      $this->sectionStorageManager = $section_storage_manager;
+    }

The @trigger_error() is missing here in the if.

Also, we could get rid of the else for easier future deprecation cleanup, by setting $section_storage_manager instead in the if, and then just setting $this->sectionStorageManager thereafter outside of any conditional.

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -43,17 +44,30 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
   +   * @param \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface $section_storage_manager
   +   *   The section storage manager.
   

   Nit: I think this parameter description needs to be prefixed with (optional).

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -85,10 +100,7 @@ protected function removeUnusedForEntityOnSave(EntityInterface $entity) {
        if ($entity->isNew() || !isset($entity->original) || $entity instanceof RevisionableInterface) {
          return;
        }
   -    $sections = $this->getEntitySections($entity);
   -    // If this is a layout override and there are no sections then it is a new
   -    // override.
   -    if ($this->isEntityUsingFieldOverride($entity) && empty($sections)) {
   +    if ($this->originalEntityUsesDefaultStorage($entity)) {
   

   It looks like the call to $this->originalEntityUsesDefaultStorage() could be merged into the preceding if statement.

3. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -93,6 +100,35 @@ protected function getInlineBlockComponents(array $sections) {
   +    if ($entity instanceof LayoutBuilderEntityViewDisplay) {
   

   Should this type hint to LayoutEntityDisplayInterface instead?

4. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -93,6 +100,35 @@ protected function getInlineBlockComponents(array $sections) {
   +        if ($display instanceof LayoutBuilderEntityViewDisplay) {
   +          $contexts['display'] = EntityContext::fromEntity($display);
   +        }
   

   Discussed in Slack with @tedbow and @xjm; Given the way Layout Builder works (it takes over the entity_view_display entity type) I'm not sure this instanceof check is actually needed. Although this also isn't really hurting anything, I don't see why we bother to check.

5. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -101,9 +137,39 @@ protected function getInlineBlockComponents(array $sections) {
   +      if ($original_section_storage instanceof DefaultsSectionStorageInterface) {
   +        // If there were no sections in the original entity the storage will
   +        // revert to the default storage and this is a new override.
   +        return TRUE;
   +      }
   

   To remove yet another level of indentation, this can just be return $original_section_storage instanceof DefaultsSectionStorageInterface.

6. +++ b/core/modules/layout_builder/tests/src/Unit/LayoutEntityHelperTraitTest.php
   @@ -0,0 +1,66 @@
   +    $this->assertEquals($expected, $method->invoke($test_class, $entity));
   

   Any reason not to use assertSame() here?

Briefly discussed with @tedbow and @xjm and I think we are going to need explicit test coverage of the new trait methods too.

Since this needs work anyway, I noticed a couple tiny nitpicks.

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -43,17 +44,30 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
   +   * @todo This constructor has one optional parameter, $section_storage_manager
   +   *    and one totally unused $database parameter. Deprecate the current
   +   *    constructor signature in https://www.drupal.org/node/3031492 after the
   +   *    general policy for constructor backwards compatibility is determined in
   +   *    https://www.drupal.org/node/3030640.
   

   Nitpick: One extra space before and, constructor, general, https. (3 spaces instead of 2)

2. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -93,6 +100,35 @@ protected function getInlineBlockComponents(array $sections) {
   +   *   The section storage if found otherwise NULL.
   

   Nitpick: Add comma before otherwise.

Here's a WIP test

#3035825: Make it easier to set up the current_user service in kernel tests will be installing the user entity schema in many many more tests, so I think that change is more than reasonable here.

Tests look good!

OK, I didn't get through the whole patch (this is hard to grok!) but here are questions and feedback I had while reviewing it down to the tests:

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -43,17 +44,30 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
   +   * @todo This constructor has one optional parameter, $section_storage_manager
   +   *    and one totally unused $database parameter. Deprecate the current
   +   *    constructor signature in https://www.drupal.org/node/3031492 after the
   +   *    general policy for constructor backwards compatibility is determined in
   +   *    https://www.drupal.org/node/3030640.
   

   @todos go at the bottom of the dockblock. Also, it's a little hard to read. How about:

   @todo The current constructor signature is deprecated:
     - The $section_storage_manager paeameter is optional, but should become required.
     - The $database parameter is unused and should be  removed.
     Deprecate in https://www.drupal.org/node/3031492.
   

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -43,17 +44,30 @@ class InlineBlockEntityOperations implements ContainerInjectionInterface {
   -  public function __construct(EntityTypeManagerInterface $entityTypeManager, InlineBlockUsage $usage, Connection $database) {
   +  public function __construct(EntityTypeManagerInterface $entityTypeManager, InlineBlockUsage $usage, Connection $database, SectionStorageManagerInterface $section_storage_manager = NULL) {
   

   I added a line at the end of the CR for this.

   "Additionally InlineBlockEntityOperations has been updated to use the section storage manager, but this is unlikely to affect any extending code."

3. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -85,10 +100,10 @@ protected function removeUnusedForEntityOnSave(EntityInterface $entity) {
   +    // If the original entity used the default storage then we cannot remove
   +    // unused inline blocks because they will still be referenced in the
   +    // defaults.
   

   What if the default storage only uses some of the inline blocks, or totally different ones? How do these get cleaned up? Is there a mechanism to clean them up manually?

4. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -132,9 +147,9 @@ protected function getRemovedBlockIds(EntityInterface $entity) {
   -    if ($this->isLayoutCompatibleEntity($entity)) {
   -      $this->usage->removeByLayoutEntity($entity);
   -    }
   +    // @todo In https://www.drupal.org/node/3008943 call
   +    //   \Drupal\layout_builder\LayoutEntityHelperTrait::isLayoutCompatibleEntity().
   +    $this->usage->removeByLayoutEntity($entity);
   

   How is this change a part of the patch? Why do we now need #3008943: Clean up todo in InlineBlockEntityOperations::handleEntityDelete() when we didn't before?

5. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -93,6 +100,35 @@ protected function getInlineBlockComponents(array $sections) {
   +      $contexts['display'] = EntityContext::fromEntity($entity);
   ...
   +      $contexts['entity'] = EntityContext::fromEntity($entity);
   

   At first it looked like this line was exactly the same in both codepaths, but one is setting $contexts['display] and the other is setting $contexts['entity'] (and then $contexts['display'] gets set later to something else). So this is correct, but an inline comment in each of the if and the else might be helpful to describe what they're doing.

6. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -101,9 +137,47 @@ protected function getInlineBlockComponents(array $sections) {
   +   * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0.
   +   *   To determine if an entity has an layout override use
   +   *   \Drupal\layout_builder\LayoutEntityHelperTrait::getSectionStorageForEntity()
   +   *   and check if the result is an instance of
   +   *   \Drupal\layout_builder\DefaultsSectionStorageInterface
   +   *   See https://www.drupal.org/node/3030609.
   

   Typos here. It should say:

   To determine if an entity has a layout override, use \Drupal\layout_builder\LayoutEntityHelperTrait::getSectionStorageForEntity() and check whether the result is an instance of \Drupal\layout_builder\DefaultsSectionStorageInterface.

   And then the "See" should be moved into an @see.

7. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -101,9 +137,47 @@ protected function getInlineBlockComponents(array $sections) {
   +   * Determines if the original entity used the default storage.
   

   This could give a little more explanation.

8. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -101,9 +137,47 @@ protected function getInlineBlockComponents(array $sections) {
   +  protected function originalEntityUsesDefaultStorage(EntityInterface $entity) {
   +    $section_storage = $this->getSectionStorageForEntity($entity);
   +    if ($section_storage instanceof OverridesSectionStorageInterface && !$entity->isNew() && isset($entity->original)) {
   +      $original_section_storage = $this->getSectionStorageForEntity($entity->original);
   +      // If there were no sections in the original entity the storage will
   +      // revert to the default storage and this is a new override.
   +      return $original_section_storage instanceof DefaultsSectionStorageInterface;
   +    }
   +    return FALSE;
   +  }
   

   So this method is essentially checking whether the updated entity:

   1. has a layout override,
   2. is not a brand-new entity,
   3. is being updated and differs from the previous entity, and
   4. previously had no override and just used the defaults.

   In a nutshell, whether a new override layout is being created for the entity in this update operation.

   The method name kind of confused me when I read it in calling code. I think there's already been discussion about what this could be called and I don't want to put too much Naming Things in here, but just flagging for now that it tripped me up reading the code.

9. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -101,9 +137,47 @@ protected function getInlineBlockComponents(array $sections) {
   +  /**
   +   * Gets the section storage manager.
   +   *
   +   * @return \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface
   +   *   The section storage manager.
   +   */
   +  private function sectionStorageManager() {
   +    return $this->sectionStorageManager ?: \Drupal::service('plugin.manager.layout_builder.section_storage');
   +  }
   

   So I would have expected this fallback in the constructor itself for BC, but it's here in the trait. Seems odd/wrong for a trait to have dependencies on services or be falling back to \Drupal. Why is it on the trait?

Apparently this hard-blocks a stable blocker, so tagging accordingly.

Thanks!

I believe #70 addresses all of the concerns from #69, and the changes look good. Setting back to RTBC.

This looks really close - only a few minor nits.

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -150,14 +171,10 @@ public function handlePreSave(EntityInterface $entity) {
   -      if ($this->isEntityUsingFieldOverride($entity)) {
   -        if (!$entity->isNew() && isset($entity->original)) {
   -          if (empty($this->getEntitySections($entity->original))) {
   -            // If there were no sections in the original entity then this is a
   -            // new override from a default and the blocks need to be duplicated.
   -            $duplicate_blocks = TRUE;
   -          }
   -        }
   +      if ($this->originalEntityUsesDefaultStorage($entity)) {
   +        // This is a new override from a default and the blocks need to be
   +        // duplicated.
   +        $duplicate_blocks = TRUE;
          }
   

   My head hurts less. Nice.

2. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -62,13 +74,8 @@ protected function getInlineBlockRevisionIdsInSections(array $sections) {
   -    return NULL;
   +    $section_storage = $this->getSectionStorageForEntity($entity);
   +    return $section_storage ? $section_storage->getSections() : [];
   

   We're slightly changing the return type here. I think an empty array makes more sense but then we need to update the docs to remove |null. Also the docs have an @todo - is that done now?

3. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -101,9 +137,50 @@ protected function getInlineBlockComponents(array $sections) {
   +    return $this->sectionStorageManager ?: \Drupal::service('plugin.manager.layout_builder.section_storage');
   

   Should the NULL case be deprecated? I always find traits and services tricky.

4. +++ b/core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
   @@ -108,6 +108,12 @@ protected function setUp() {
   +    // Install the 'user' entity schema because the workspaces module's install
   +    // hook creates a workspace with default uid of 1. Then the layout_builder
   +    // module's implementation of hook_entity_presave will cause
   +    // \Drupal\Core\TypedData\Validation\RecursiveValidator::validate() to run
   +    // on the workspace which will fail because the user table is not present.
   

   That's funky. That's probably worth a follow to fix workspaces to not create a workspace when user does not exist.

5. +++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
   @@ -15,6 +20,13 @@
   +  /**
   +   * The section storage manager.
   +   *
   +   * @var \Drupal\layout_builder\SectionStorage\SectionStorageManagerInterface
   +   */
   +  protected $sectionStorageManager;
   

   I think we should document that we should document that classes using this trait are expected to set this.

#79

2)
Fixed. Also, that todo points to an issue that this one duplicates. I'll close it.

3)
I have no problem leaving this as it is. I think deprecating is too prescriptive here, there's nothing actually wrong with the service call IMO.

4)
Opened #3039217: ResolvedLibraryDefinitionsFilesMatchTest needs to install the user entity schema

5)
Given my take on 3, I don't think this is needed. The only time this would matter is during unit testing.

+++ b/core/modules/layout_builder/src/LayoutEntityHelperTrait.php
@@ -101,9 +133,50 @@ protected function getInlineBlockComponents(array $sections) {
+      $original_section_storage = $this->getSectionStorageForEntity($entity->original);
+      return $original_section_storage instanceof DefaultsSectionStorageInterface;

Nit: This could be a single (albeit very long) line.

Feedback looks pretty solidly addressed to me, and I agree that the logic surrounding originalEntityUsesDefaultStorage() is a lot cleaner. I only skimmed the tests, but the coverage looks (at a glance) comprehensive. Seeing as how this already seems to have @alexpott's blessing, I'm not sure there's much more for me to do here but RTBC this.

Committed and pushed 09a6d81ec9 to 8.8.x and 70b46d5554 to 8.7.x. Thanks!

I'm not 100% convinced on the usage of \Drupal::service() in traits but this trait is far from alone and service location is a massive issue and fixing the way we do it far beyond the scope here so /shrug.

This was not successfully pushed

hint: Updates were rejected because the tip of your current branch is behind sorry :(

Pushing to 8.8.x now and will backport to 8.7.x once we're out of the freeze. Committed 30654bd41d and pushed to 8.8.x. Thanks!

Setting the correct status.

We're still in commit freeze, so reverted from 8.7.x for now.

publish change record