Allow the inline creation of non-reusable Custom Blocks in the layout builder

Plus one

Since it was unassigned, I just started hacking something together, here's a starter patch with the reusability. Will create a block plugin and more tests later in the week.

Neat. I could pretty much use the whole code provided from https://www.drupal.org/project/drupal/issues/2948064 and change the way its stored.

Heres an updated patch that allows you to add inline blocks. Still need to work with revisioning, update hook, etc.

Forgot the updated test.

New patch.

This is a possible solution for #2940755: block_content block derivatives do not scale to thousands of block_content entities.

I think we should just not have used derivates for this initially, same for menus and other things and just gone with an autocomplete widget + some kind of pre-fill feature through a place action on the block overview, but too late for that :)

In general I agree with the idea but I don't think we should make it layout builder specific but allow all content blocks to be placed like this instead. That means also thinking about the exact wording for this and how the UI would work for normal blocks. And maybe eventually even deprecate the old way?

I'm also not convinced to use the block revision ID for this. That would IMHO be extremely confusing for users as it would make updating very hard? But I also didn't look closely at the patch.

Good point. So we're basically building poormanscron paragraphs functionality here ;)

Adding related issue #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization, as Custom Block (Non-)Reusability should be considered a replacement for the "inline blocks" approach mentioned there.

Trying to understand what would happen with revisions in this scenario:

1. User creates a custom block places it in the main region on Page A and in the Sidebar on Page B.
2. User edits the layout on Page B.
3. User makes a change to the custom block content.
4. User reverts the layout on Page B to a previous version

Is the output of the custom block on Page A and Page B identical? Or because of where the revision happened, the pages are displaying two different versions of the block?

@kevincrafts

Here's what I think:

Page A and Page B would have separately blocks in the first place because inline blocks are not intended to be "reusable" but only where it was created.

Instead, I think it should be something like:

1. User create a Page A (revision 1), and adds a Inline custom block "Hello foo!"
2. User updates Page A (revision 2) and also change the inline custom block to "Hello bar!"
3. User reverts Page A to revision 1 and inline custom block should now be back at "Hello foo!"

There might actually be an argument for having _both_ approaches (this issue's "re-usable" checkbox vs. inline-blocks of #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization). Most notably, this approach supports translation of individual blocks. If a site needed to maintain parity of layout between translations, editors could place the content as a "standard" block instead of inline-block and then accept the inherent limitations (e.g., no content moderation support until ERR makes it into core).

@kevincrafts, I believe the behavior outlined by @johndevman is the ideal approach but falls outside the scope of this ticket. To support this kind of functionality, we would need ERR (or something akin to it) in core. Solving this very hard problem is one of the main motivators for going with a "serialized" approach (a la #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization) rather than saving blocks to the database as normal entities.

Currently, all we have is an ER field, which targets the entity ID and not the revision ID, so this content would not follow the expected revision pattern. This sub-optimal behavior is how things currently work when placing blocks via Layout Builder but is more acceptable / makes more sense when a block can be used in more than one place (its content doesn't "belong" to that particular node).

Adding related core issue that's needed to help make ERR a viable solution.

So how would one go about placing a reusable block content on multiple pages with Layout Builder that would not cause the block content to diverge on the editing of a page?

All blocks are currently reusable. So it would work the same as it does today.
I guess this issue is more about "non-re-usability"?

@kevincrafts, yes, it works like @tim.plunkett described. There would be two types of blocks:

1. "Re-usable" ones (what we already have in core) that behave like regular Entity Reference fields, targeting block entity ID instead of revision ID.
2. "Non-re-usable" ones that would use an ERR (Entity Reference Revisions) type of relationship and target the block entity revision, allowing it to follow a content moderation workflow, etc.

Make sense?

The thing with re-using (as in, the entity type, not a single entity) block_content with a new field however is that at least unless we adapt the view, /admin/structure/block/block-content will still list all those blocks and users will try to edit them through that, which will then not actually do anything as the embedded blocks reference a specific revision ID.

Also translations sounds like an interesting problem as each translation will have different revision id), I guess it would work with a translatable layout_selection field, but as config for an entity view display? And publishing draft translation revisions results in new revisions.

I might be biased as a paragraphs.module co-maintainer, but I would recommend to be very careful about trying to cover this with some kind of quick fix like this in core. As mentioned here, ERR/paragraphs is very complex.

As an different but possibly interesting idea, imagine what you could do if you couldn't just put fields into layouts but control each field item separately? E.g. you could have a multi-value body field and then split two deltas up into a two-column layout, or have a large image with smaller images below. Or have N paragraphs in a single paragraph field and place them on a page in whatever way you want ;)

That's sorta how https://www.drupal.org/project/paragraph_blocks works, and douggreen suggested a per-delta feature back in Baltimore. Not sure that it has a core issue yet.

@tim.plunkett/@Berdir, if the content can be edited elsewhere (e.g., the node edit form), then referencing field deltas with Layout Builder presents some challenges. Targeting the actual entity revision ID (as we do here) gets around those challenges, but then we get back into the realm of needing ERR-like functionality in core (#2812595: Support revision tracking of entity references in core), and I echo @Berdir's concerns about attempting to re-solve the issues ERR has already surfaced and tackled w.r.t. moderation, translation, etc.

@Berdir, I'd love to get your thoughts on #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization as an alternate approach. Will you chime in there? Many thanks!

I have been working on #2948064-49: Layout Builder should support inline creation of Custom Blocks using entity serialization but I ran into a bunch of problems that make me(and others) think that issue won't work. You can see the reasons in the comment I linked to here.

So now looking at this issue to solve the problem. It looks like this patch has made great progress so far!

1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,227 @@
   +    $block->save();
   +    $this->configuration['block_revision_id'] = $block->getRevisionId();
   

   If we save blocks in form submit in this way then any block that is added in the Layout Builder will exist whether the user actually saves the Layout or not.

   So if you add 5 new inline blocks but never actually save the layout then you have will have 5 new content blocks that aren't actually used anywhere.

   It would probably make sense to keep this in tempstore and only save the blocks when the layout is saved.

   This is also the case for editing existing blocks. If the blocks would be saved and new revision created, if applicable, and then if the user cancels the layout changes they likely do not want the changes to the blocks either.

2. re #26
   

   The thing with re-using (as in, the entity type, not a single entity) block_content with a new field however is that at least unless we adapt the view, /admin/structure/block/block-content will still list all those blocks and users will try to edit them through that, which will then not actually do anything as the embedded blocks reference a specific revision ID.

   Can we ship an updated View? Not sure how that works.

   If not we documented that View should updated or reverted back to new core View. We may want to disable access to the block edit form for non-reusable blocks outside of the layout builder or at least at the existing edit route. Because presumably editing of reusable blocks should only ever happen in block plugin form. So we could prevent that.

1. Changed \Drupal\layout_builder\Plugin\Derivative\InlineBlockContentDeriver::getDerivativeDefinitions() to \Drupal\Core\Entity\EntityInterface::getConfigDependencyKey() and \Drupal\Core\Entity\EntityInterface::getConfigDependencyName() instead of using hardcoded string
2. Renamed BlockContentDeriver to InlineBlockContentDeriver to match the plugin name
3. Override ::buildConfigurationForm() to take away the default title when adding a new block. The block type label doesn't make sense for the block title shown on entity view, i.e. "Basic Block" and the label will not be used anywhere else such as admin listing.
4. Updated InlineBlockContentBlockTest be most comprehensive based on changes from #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization. To make this test work I had to add $block->setNewRevision(TRUE); otherwise it wouldn't make a new revision. Without the new revision when you change the block in an override it actually changed the default also because it wasn't a new revision.
5. A bunch of nit from @phenaproxima's review of #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization because a lot of the code was the same.

Here is a failing test that proves #29.1

If you add an inline block to a layout but never end up saving the Layout because you either Cancel or Revert the content block still gets created and remains

@todo Create a similar test for editing existing inline blocks. When you edit a block but don't save the layout the content block changes remain.

Also in writing this test I found this #2970801: If you add block then try to Revert the layout it doesn't revert

First attempt on #29.1.

@johndevman thanks for the patch.
I think this method will work though I am not sure what would happen with field fields. Will the files be removed?

I think the test failure is because of #2970801: If you add block then try to Revert the layout it doesn't revert

So created a dataProvider function for \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockContentBlockTest::testNoLayoutSave to demonstrate that it works for "cancel" but not "revert"

So this will still fail for the "revert" case but should pass for the "cancel" case.

@tedbow Good question. What would the expected state of the block be if, it's deleted on an entity? It should not be deleted, because of revisioning. So I suppose the image will remain, right?

It should be deleted if the host entity is deleted (like Paragraphs) though(?) We should write tests for this.

Added a start on testing revisioning. Needs more scenarios covered... but this is a start :)

@johndevman thanks for the continued progress! Setting to need review to run tests.

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockContentBlockTest.php
   @@ -245,2 +245,78 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   

   Can't use @inheritdoc on test methods that don't override a method.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockContentBlockTest.php
   @@ -245,2 +245,78 @@
   +      'access contextual links',
   +      'configure any layout',
   +      'administer node display',
   +      'administer node fields',
   +      'administer nodes',
   +      'bypass node access',
   

   I don't think all of these permissions are needed. We should only have the permission needed to do the actions in the test.

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockContentBlockTest.php
   @@ -245,2 +245,78 @@
   +    $assert_session->elementExists('css', '.form-submit');
   +    $this->click('.form-submit');
   

   I think we should be able to just do $page->pressButton('Revert') like the other button presses in this method.

1. I added an in progress fix from #2970801: If you add block then try to Revert the layout it doesn't revert to \Drupal\layout_builder\Form\RevertOverridesForm::submitForm
2. Updated \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockContentBlockTest::testInlineBlocks() to cover canceling/reverting a layout with blocks that existing in the layout but with edits since last layout save. The block edits should not be saved. This passes.
3. But also if there is inline content block in override and the layout is reverted and the inline content block is used nowhere else it should be deleted. I added test coverage for this but it will not pass 😞.

   This case is much more complicated and must handle these cases:

   1. The inline content block was created in the default and still is referenced in the default. In this case the content block should not be deleted.
   2. The inline content block was created in the default but is no long referenced in the default.

   In this 2nd case the inline content block may still be referenced in a layout override for different entity for the same bundle if the override was created before the inline content block was removed from default. So in this case the content block should not deleted.
   But also in this 2nd case the inline content block may not be referenced if has removed from all overrides or if all the overrides were created after the inline block was removed from the default.

   There is actually no way to know this except by checking all entities of this bundle that have overrides.

   For this reason it may make more sense when first creating a layout override to duplicate any non-reusable blocks reference. Then when it is removed from an override it will then can safely be deleted from the content block storage.

This is a started patch on duplicating inline blocks when overriding.

Still needs to update test.

.. Meh.. whitespace.

#44 Regarding deletion, there many scenarios we need to take under consideration:

a) Override
1) When the entity is deleted
2) When an block is removed and the layout is saved
3) When the layout is reverted to default

b) Default
1) When the content type is deleted
2) When the block is removed and the layout is saved

c) Layout Builder uninstall.

Initially, I planned to create a class, and do something like:

    $storage = \Drupal::service('entity_type.manager')->getStorage('block_content');

    foreach ($sections as $section) {
      $components = $section->getComponents();

      foreach ($components as $component) {
        $plugin = $component->getPlugin();
        if ($plugin->getBaseId() === 'inline_block_content') {
          $block_revision_id = $component->getConfiguration()['block_revision_id'];
          $block = $storage->loadRevision($block_revision_id);

          $block->delete();
        }
      }
    }

on each of these scenarios through out the code, but I think it would be cleaner if we could implement some event handling for Section storages for these events post save instead? What do you think?

Edit: Posted this idea in Slack:

There are various places where Inline blocks would need to “react” on somethings, based on
a) Something changed to the section list, and b) it is saved. What about adding various events for this? It would save so much hard-coding stuff in LB module to get it to work properly, and it would not need to rely on UIs like the Revert controller, and would work just fine if a contrib. implemented its own, etc.

re @Berdir comment on #14

In general I agree with the idea but I don't think we should make it layout builder specific but allow all content blocks to be placed like this instead.

The changes we having been making to the patch to support not saving blocks before that layouts are saved have actually been connecting to Layout Builder module.

1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -189,9 +190,7 @@
   -    $block->setNewRevision(TRUE);
   -    $block->save();
   -    $this->configuration['block_revision_id'] = $block->getRevisionId();
   +    $this->configuration['block_serialized'] = serialize($block);
   

   So with these changes the content block won't actually be saved/created when the block plugin configuration form is save.

   This means if Block UI module is used to place the block then the content block won't actually be saved.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -230,6 +230,28 @@ public function label() {
      public function save() {
   +
   +    $sections = $this->getSections();
   +
   +    foreach ($sections as $section) {
   +      $components = $section->getComponents();
   +
   +      foreach ($components as $component) {
   +        $plugin = $component->getPlugin();
   +        if ($plugin->getBaseId() === 'inline_block_content') {
   +          $configuration = $component->getConfiguration();
   +          if (!empty($configuration['block_serialized'])) {
   +            $block = unserialize($configuration['block_serialized']);
   +            $block->setNewRevision(TRUE);
   +            $block->save();
   +            $configuration['block_serialized'] = NULL;
   +            $configuration['block_revision_id'] = $block->getRevisionId();
   +            $component->setConfiguration($configuration);
   +          }
   +        }
   +      }
   +    }
   

   Here we actually do the saving in \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::save(). This connects the functionality to the Layout Builder module.

   I don't see how we get around this.

   One idea would be to save the content blocks in \Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockSubmit so that it would work for Block UI. Then \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::save if the block is no longer referenced, in the case it was removed, we delete the content block.

   But then what if user never actually saves the layout but just closes the tab. Then a content would have saved but it is marked as not reusable but it is not used anywhere, orphaned.

We basically need the block form to act 2 different ways. With the Layout Builder don't save Content Block then with other uses save the Content Block in \Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockSubmit.

Could use a different plugin form for layout builder using the functionality we created in #2763157: Allow plugins to provide multiple forms?
This is what we used in Settings Tray to provide a different block plugin form when used with the settings tray.

So basically in \Drupal\layout_builder\Form\ConfigureBlockFormBase::doBuildForm()

We could do something like

if ($block instanceof PluginWithFormsInterface) {
      return $this->pluginFormFactory->createInstance($block, 'layout_builder', 'configure');
    }

Copied from \Drupal\settings_tray\Block\BlockEntitySettingTrayForm::getPluginForm()

This gives a try at

We basically need the block form to act 2 different ways. With the Layout Builder don't save Content Block then with other uses save the Content Block in \Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockSubmit.

Could use a different plugin form for layout builder using the functionality we created in #2763157: Allow plugins to provide multiple forms?
This is what we used in Settings Tray to provide a different block plugin form when used with the settings tray.

It allows the configuration form to not save the content block when used with layout builder but to save the content block when used with all other uses, i.e. block ui.

1. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php
   @@ -103,4 +103,30 @@ public function removeSection($delta) {
   +      foreach ($components as $component) {
   +        $plugin = $component->getPlugin();
   +        if ($plugin->getBaseId() === 'inline_block_content') {
   

   This requires the Layout Builder \Drupal\layout_builder\Plugin\SectionStorage\SectionStorageBase to know about 'inline_block_content' plugins.

   Really what need here is concept that plugin form could be submitted but we don't yet want to make the changes permanent. In this case it because don't want to create Content Block entity when the plugin configuration form is submitted. For all we know the user may never say the layout and the content block entity would be orphaned.

   So in this patch I am created a PermanentSavePluginInterface to denote a plugin that has extra save step that makes permanent changes.

   So also renaming saveInlineBlocks() to permanentlySaveComponents().

2. +++ b/core/modules/layout_builder/src/SectionStorageInterface.php
   @@ -138,4 +138,11 @@ public function label();
   +  public function saveInlineBlocks();
   

   I renamed this in the patch but also does actually need to be on the interface? Couldn't this be a protect method on SectionStorageBase

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -230,7 +231,34 @@ public function label() {
+  public function duplicateDefaultsInlineCustomBlocks($sections) {
+    /** @var Section $section */
+    foreach ($sections as $section) {
+      $components = $section->getComponents();
+
+      foreach ($components as $component) {
+        $plugin = $component->getPlugin();
+        if ($plugin->getBaseId() === 'inline_block_content') {
+          $configuration = $component->getConfiguration();
+          if (!empty($configuration['block_revision_id'])) {
+            $entity = $this->entityTypeManager->getStorage('block_content')->loadRevision($configuration['block_revision_id']);
+            $duplicated_entity = $entity->createDuplicate();
+            $configuration['block_revision_id'] = NULL;
+            $configuration['block_serialized'] = serialize($duplicated_entity);
+            $component->setConfiguration($configuration);
+          }
+        }
+      }
+    }
+
+    return $sections;
+  }

I feel like we should also abstract this away from inline_block_content plugins.

Should we have some concepts of plugins that are not safe to just create duplicate configuration of. Meaning we should tell the plugin that we want to create duplicate and would do any necessary operations if any and update configuration if needed.

Mostly nits.

1. +++ b/core/modules/block_content/src/BlockContentInterface.php
   @@ -48,6 +48,28 @@ public function setInfo($info);
   +  public function setReusable();
   +
   +  /**
   +   * Sets the block not to be re-usable.
   +   *
   +   * @return $this
   +   */
   +  public function setNotReusable();
   

   Don't think we need both here we can just a have a $reusable parameter.

2. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -200,6 +200,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setLabel(t('Re-usable'))
   

   "Reusable" is one word. Changing all.

3. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
   @@ -0,0 +1,64 @@
   +      'description' => "Provides a block type that increases your site's spiffiness by upto 11%",
   

   "upto" is 2 words. Won't comment on whether "spiffiness" is a word 😉

4. +++ b/core/modules/layout_builder/src/Form/ConfigureBlockFormBase.php
   @@ -237,7 +237,8 @@ protected function successfulAjaxSubmit(array $form, FormStateInterface $form_st
   diff --git a/core/modules/layout_builder/src/Form/InlineBlockDefaultForm.php b/core/modules/layout_builder/src/Form/InlineBlockDefaultForm.php
   
   +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,275 @@
   +   * @var \Drupal\Core\Session\AccountInterface.
   

   Should not have period.

5. +++ b/core/modules/layout_builder/src/Plugin/Block/PermanentSavePluginInterface.php
   @@ -0,0 +1,18 @@
   +}
   \ No newline at end of file
   diff --git a/core/modules/layout_builder/src/Plugin/Derivative/InlineBlockContentDeriver.php b/core/modules/layout_builder/src/Plugin/Derivative/InlineBlockContentDeriver.php
   

   Whoops I have fixed my editor now.

6. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -230,7 +231,34 @@ public function label() {
   +    $this->permanentlySaveComponents();
   

   This can be move up to the parent class.

7. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -197,7 +197,7 @@ public function setWeight($weight) {
   -  protected function getConfiguration() {
   +  public function getConfiguration() {
   

   Can't change this back to protected but if we change \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::duplicateDefaultsInlineCustomBlocks to something more generic we can.

1. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
   @@ -0,0 +1,64 @@
   +      'description' => "Provides a block type that increases your site's spiffiness by up to 11%",
   

   It's taken from a similar test in core!

2. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,274 @@
   +      $this->setConfigurationValue('block_serialized', NULL);
   +      $this->configuration['block_serialized'] = NULL;
   

   Nit: We can remove one of these.

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -230,7 +231,36 @@ public function label() {
   +        if ($plugin instanceof DerivativeInspectionInterface) {
   +          if ($plugin->getBaseId() === 'inline_block_content') {
   

   Could be one-liner?

+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
@@ -121,6 +122,11 @@ protected function prepareLayout(SectionStorageInterface $section_storage, $is_r
+        // Create a duplicate of each default inline custom block.
+        if ($sections) {
+          $sections = $section_storage->duplicateDefaultsInlineCustomBlocks($sections);
+        }

Now I am thinking what we actually need here is an event for preparing the layout for the UI.

Since Inline Content Blocks ultimately probably should live in the Layout Builder module then we don't want specific logic for them here.

I will give this a try.

Here is rough start for #61

The event doesn't work how I would like yet though I think it works for what inline blocks needs. But it would be nice if it fired whenever a layout was prepared for the UI not just creating an override.

I have to switch to something else now so just wanted to get this start out there but should able to get back this in a few hours.

So more thoughts

1. #51

   #44 Regarding deletion, there many scenarios we need to take under consideration:

   a) Override
   1) When the entity is deleted
   2) When an block is removed and the layout is saved
   3) When the layout is reverted to default

   b) Default
   1) When the content type is deleted
   2) When the block is removed and the layout is saved

   For override we couldn't delete a content block when it was removed from the layout unless the parent entity is not using revisions
   We could only delete when delete content blocks when the parent entity is deleted. and then we would have to make sure every content block referenced in ever revision is delete.

   Since we could have 100's or 1000's of revisions this might be difficult to do on saving the parent entity.

2. We would probably want to avoid non-reusable blocks from having their own layout overrides. or when delete parent entity that references content blocks we would also to check for every revision of the content block for inline non-usable content blocks... and so on
3. Been chatting with @johndevman about how we could track all referenced non-usable content blocks. To avoid tracking table we could keep append usage for every revision to the override layout information of the latest revision.

   We would only have to store the ids. This way we wouldn't have deserialize every layout revision.

   We could add `OverridesSectionStorage::addNonResubleEntityUsage()` and `::getAllNonReusbleEnityUses()` then on parent entity delete would could all `::getAllNonReusbleEnityUses()` to get all entities to delete

   The only problem I can think of right now would be what if `::getAllNonReusbleEnityUses()` returned 100's of entities. could you delete them all when deleting the parent entity?

4. We could just add an EntityUsage service like have for FileUsage service but I think should explore other options first like OverridesSectionStorage::addNonResubleEntityUsage()

   if we had a `EntityUsageService` then in `hook_entity_delete` we could call `EntityUsage::removeUsagesWhereEntityIsUser($entity_deleted)`

   Assuming that you only use this service for entities that should be deleted when not used elsewhere.

1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,274 @@
   +    $form['title']['#description'] = $this->t('The title of the block as shown to the user.');
   

   This doesn't do anything right now.

2. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,274 @@
   +      unset($form['label']['#default_value']);
   

   Can we make the Block description optional? It is not really required for many blocks when used inline. (it is required for the block UI, and if to be changed through Block UI, since required) but not in LB context, Perhaps we could use some sensible fallback if used in LB and you don't want the field shown.

This patch begins to explore the idea of tracking dependent entities on the storage. It implements a new interface PluginWithDependentEntitiesInterface, an idea that tedbow had.

Some hacky code from my experiments remains, but it works.

1. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -273,2 +273,20 @@
   +      $entity = $this->entityTypeManager->getStorage('block_content')->loadRevision($this->configuration['block_revision_id']);
   ...
   +    // @TODO Store dependent entity on configuration instead?
   

   I think we can store the ID here instead on the configuration?

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +82,22 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +    $dependent_entities = unserialize($entity->get('layout_builder__layout')->dependent_entities);
   

   Can the unserialize be done on the field item instead?

3. +++ b/core/modules/layout_builder/src/Plugin/Block/PluginWithDependentEntitiesInterface.php
   @@ -0,0 +1,25 @@
   +   * Add dependent entities.
   

   Needs docblock updated with return type.

4. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
   @@ -35,6 +35,10 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +    $properties['dependent_entities'] = DataDefinition::create('any')
   

   Not sure which data type to use here?

5. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
   @@ -46,7 +50,9 @@ public function __get($name) {
   +    if (method_exists($this, 'getProperties')) {
   +      $this->getProperties();
   +    }
   

   This is not needed. It failed when I used the wrong data type.

6. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +82,22 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +    $storage = \Drupal::service('entity_type.manager')->getStorage('block_content');
   ...
   +      $entity = $storage->load($dependent_entity_id);
   

   Should we support any entity type rather than just block content?

1. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +82,22 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +  if ($entity->hasField('layout_builder__layout')) {
   

   Need to check if $entity instance of FieldableEntityInterface

2. +++ b/core/modules/layout_builder/src/Plugin/Block/PluginWithDependentEntitiesInterface.php
   @@ -0,0 +1,25 @@
   +   * Add dependent entities.
   +   */
   +  public function addDependentEntities();
   

   Not sure this method is needed on the interface. InlineBlockContentBlock doesn't need it and really neither does OverridesSectionStorage because it is only called internally

   If each plugin keeps track of it's own dependent entities this doesn't need to be public.

3. +++ b/core/modules/layout_builder/src/Plugin/Block/PluginWithDependentEntitiesInterface.php
   @@ -0,0 +1,25 @@
   +  /**
   +   * Get dependent entities.
   +   *
   +   * @return array
   +   *   An array of entity IDs.
   +   */
   +  public function getDependentEntities();
   

   For this to work outside of layout builder would need to return an array where the keys are entity type ids and values are array of entity ids.

   layout_builder_entity_predelete currently is hardcode for block_content entity type.

4. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
   @@ -35,6 +35,10 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +    $properties['dependent_entities'] = DataDefinition::create('any')
   +      ->setLabel(new TranslatableMarkup('Dependent Entities'))
   +      ->setRequired(FALSE);
   

   This maybe the part I like best about the change in this interdiff, keeping the dependent_entities at a field property.

   But it does kind of field weird to keep information about pass revisions in the current revision field value.

5. +++ b/core/modules/layout_builder/src/Plugin/Field/FieldType/LayoutSectionItem.php
   @@ -46,7 +50,9 @@ public function __get($name) {
   +    if (method_exists($this, 'getProperties')) {
   +      $this->getProperties();
   +    }
   

   Not sure how this is related.

6. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -31,7 +32,7 @@
   -class OverridesSectionStorage extends SectionStorageBase implements ContainerFactoryPluginInterface, OverridesSectionStorageInterface {
   +class OverridesSectionStorage extends SectionStorageBase implements ContainerFactoryPluginInterface, OverridesSectionStorageInterface, PluginWithDependentEntitiesInterface {
   

   DefaultsSectionStorage storage would also need to implement PluginWithDependentEntitiesInterface because default also will have InlineContentBlock plugins. So maybe SectionStorageBase would implement it instead.

Now I wondering if PluginWithDependentEntitiesInterface was a good idea 😜

The proposed solution for dependent entities in #66 won't work because it assumes that the latest/current revision keeps track of ALL dependent entities. If the entity is reverted to an older revision that could mean that some dependent entities are NOT in the new revision and latest revision.

So we'll continue from patch #62 instead, moving forward with perhaps the entity usage table.

This adds tests for making sure content blocks are deleted when they should be. Currently it will fail.

Agree with #71, @johndevman thanks for exploring the PluginWithDependentEntitiesInterface idea.

Thoughts

• This patch add schema for Inline Blocks plugin which was missing. It was causing undetected errors because some places we were saving the layout but not confirming the save message.

• Interested EntityUsage service/table idea which would be similar to file usage. 1 big difference to file usage is that the way it works currently is that a inline block content would only be used by 1 entity or default.

  If we didn't duplicate blocks then that won't be true. I suggested duplicating blocks in override to make the usage tracking easier. So maybe if we had better tracking duplicating would not be better.

  One strange situation if we didn't duplicate blocks in overrides would be you could get revision histories that would be:

  • default create block
  • edit from default
  • edit from node/1
  • edit from default
  • edit from node/2
  • edit from default

  Of course maybe this doesn't matter if you don't look at revisions for an individual inline block but if someone wanted to build a view to look at them there won't be a way to separate out what the revisions are connect to.

• One reason to keep duplicating blocks in overrides is that it might make tracking unnecessary. Looking at how \Drupal\file\FileUsage\FileUsageBase::delete actually
  works deleting a file usage just marks the file has temporary and then file_cron actually deletes the files. In our case we would need to delete blocks on cron because there could be hundreds in the revisions for a single node. But if we are duplicating the content blocks from the default to the override we maybe able to skip the tracking.

  Here is an idea:

  1. Change the new reusable field from a boolean to reusable_state an integer. Have 3 constants for the states REUSABLE, NON_REUSABLE_ACTIVE, and NON_REUSABLE_INACTIVE,
  2. When an entity that has an override is delete. got through all the revisions and set the reusable_state to NON_REUSABLE_INACTIVE.
  3. on block_content_cron delete NON_REUSABLE_INACTIVE content blocks.

  Now that I write it out maybe that doesn't help 😟. Doing hundreds of ::save() vs hundreds of ::delete() calls is probably not that much better.

• since defaults don't have revisions we should be safe to delete any inline block content entities as they are removed the layout and the layout is saved.
  For that reason when deleting a bundle we would only have to delete the inline content blocks that were currently on the layout

Talked with @johndevman. Going to give EntityUsage Service a try.

1. Looking back I think was wrong in #55 when I introduced the PermanentSavePluginInterface.

   The problem I was trying to solve was that I thought there wasn't a point where we could save the block after the configuration form was saved and we didn't want to save it when the configuration form was save because we would end up with orphaned content blocks.

   Now I realize that what I added in \Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::savePermanently() can actually be done in hook_presave if it is fieldable entity then just need to check for layout_builder__layout and if the entity type is entity_view_display it will be in the third party settings.

2. +++ b/core/modules/layout_builder/src/EventSubscriber/PrepareLayoutForUiSubscriber.php
   @@ -0,0 +1,75 @@
   +  public function onPrepareLayout(PrepareLayoutForUiEvent $event) {
   +    if (!$event->isRebuilding()) {
   +      foreach ($event->getSections() as $section) {
   +        $components = $section->getComponents();
   +        foreach ($components as $component) {
   +          $plugin = $component->getPlugin();
   +          if ($plugin instanceof DerivativeInspectionInterface) {
   +            if ($plugin->getBaseId() === 'inline_block_content') {
   +              $configuration = $component->getConfiguration();
   +              if (!empty($configuration['block_revision_id'])) {
   +                $entity = $this->entityTypeManager->getStorage('block_content')->loadRevision($configuration['block_revision_id']);
   +                $duplicated_entity = $entity->createDuplicate();
   +                $configuration['block_revision_id'] = NULL;
   +                $configuration['block_serialized'] = serialize($duplicated_entity);
   +                $component->setConfiguration($configuration);
   +              }
   +            }
   +          }
   +        }
   +
   +      }
   +    }
   

   I am also wondering if we could get rid of this event altogether. Do we need to duplicate the blocks here or would be able to do this also in hook_entity_presave or hook_entiy_save.

   When creating a new override we could check $entity->original to see if it is new or is there another way to know?

   Either when creating a new override we could duplicate the blocks we are actually saving the entity.

   One drawback would this fire on every entity save vs this custom event.

3. I have started work on the EntityUsage service but probably won't get back to till Monday. But my current work is here https://github.com/tedbow/drupal/tree/2957425-48_entity_usage/core/modul...

Unassigning myself since not working on it currently

This does #76.1 gets rid of the need for PermanentSavePluginInterface.

I think it makes sense not to introduce this if we don't need it.

1. +++ b/core/modules/block_content/block_content.install
   @@ -138,3 +138,18 @@ function block_content_update_8400() {
   +    ->setDefaultValue(TRUE);
   +
   +  $reusable->setInitialValue(TRUE);
   

   There's no need to call setInitialValue()on a separate line :)

2. +++ b/core/modules/block_content/src/Plugin/Derivative/BlockContent.php
   @@ -43,7 +43,7 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   -    $block_contents = $this->blockContentStorage->loadMultiple();
   +    $block_contents = $this->blockContentStorage->loadByProperties(['reusable' => TRUE]);
   

   I remember that we had to do some kind of optimization for loading config entities only by their ID, because it was quite slow otherwise. So we need to ensure that we're not losing it with this change..

3. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -128,11 +146,16 @@ protected function prepareLayout(SectionStorageInterface $section_storage, $is_r
            $sections[] = new Section('layout_onecol');
          }
    
   +
   +
          foreach ($sections as $section) {
            $section_storage->appendSection($section);
          }
   +
   +
          $this->layoutTempstoreRepository->set($section_storage);
        }
   +
   

   There are some extra empty lines here that can be removed.

4. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,274 @@
   + * Defines an inline custom block type.
   

   I think this means that the patch is doing two things:

   1) allows a custom block to be re-usable or not
   2) adds a new concept of "inline block content"

   If that's the case, why aren't we doing it in two separate issues?

@amateescu thanks for the review!

I think this means that the patch is doing two things:

1) allows a custom block to be re-usable or not
2) adds a new concept of "inline block content"

If that's the case, why aren't we doing it in two separate issues?

I updated the summary to say why I think we should keep this as 1 big patch for now

This issue actually include several concepts that should probably be split out into separate issues.
To determine if end goal is achievable and advisable with this approach lets keep this as a big patch containing the sub tasks until it looks like it is good idea. We won't want to commit small supporting parts if we are going abandon this approach for something like #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization(currently that issue seems not doable)

+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
@@ -122,11 +137,8 @@ protected function prepareLayout(SectionStorageInterface $section_storage, $is_r
+        $event = new PrepareLayoutForUiEvent($sections, $is_rebuilding);
+        $this->eventDispatcher->dispatch(LayoutBuilderEvents::PREPARE_SECTIONS_FOR_UI, $event);

This patch removes the need for this event that was added in #62. The event was used to duplicate the content blocks from the default when creating an override layout.

This can actually be done in layout_builder_entity_presave without the need for a new event.

The only problem is that to determine if the layout is new override I am using $original_sections_field->isEmpty().
This seems to work now because if you remove all the sections from an override and save the layout you actually revert to default. So if that is intentional then this method will work.

Would we ever want to allow the ability to save an override with no sections?

So that reduced the size of this patch a good bit 🎉

Review #79
1. fixed
2. BlockContent entities are not "config entities" does you comment still apply.
3. Fixed this in #81

Other changes.

1. +++ b/core/modules/block_content/src/Entity/BlockContent.php
   @@ -200,6 +200,11 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['reusable'] = BaseFieldDefinition::create('boolean')
   +      ->setLabel(t('Reusable'))
   +      ->setDescription(t('Determine if the block should be reusable or not.'))
   +      ->setDefaultValue(TRUE);
   +
        return $fields;
      }
    
   @@ -282,6 +287,21 @@ public function setRevisionLogMessage($revision_log_message) {
   
   @@ -282,6 +287,21 @@ public function setRevisionLogMessage($revision_log_message) {
        return $this;
      }
    
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isReusable() {
   +    return (bool) $this->get('reusable')->value;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setReusable($reusable = TRUE) {
   +    $this->set('reusable', $reusable);
   +    return $this;
   +  }
   +
   

   Just note here these are outside layout builder. After we have a patch validates this whole reuse concept we will probably want split change to block_content module to it's own issue.

   Or if we really wanted to be experimental in this module we could actually extend \Drupal\block_content\Entity\BlockContent

   Use our own class for this entity. We could also swap out the deriver for our own to handle reuse.

   Probably want a separate issue but just saying it could be done.

2. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
   @@ -2,6 +2,7 @@
   +use Drupal\Component\Plugin\DerivativeInspectionInterface;
   

   Unused "use"

3. +++ b/core/modules/layout_builder/src/SectionComponent.php
   @@ -197,7 +197,7 @@ public function setWeight($weight) {
   -  protected function getConfiguration() {
   +  public function getConfiguration() {
   

   Re #57.7 we don't need to be public anymore 👍

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -44,3 +44,20 @@ layout_builder.component:
   +      label: 'Block revisions'
   

   Block revision ID?

2. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +85,51 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +      $new_revision = $entity->isNewRevision();
   +      $new_revision = TRUE;
   

   This will always be TRUE.

3. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -0,0 +1,295 @@
   +  public function saveBlockContent($new_revision = FALSE, $duplicate_block = FALSE) {
   

   Nice rename. I am unsure about doing duplicate logic within the save method though. This behaviour is kinda coupled to LB only as well. Right?

I did some manual testing:

General (things adressed in current patch)

1. The duplicated block from default layout changed property time is the same, do we want to make sure this is actually the timestamp from when it was duplicated instead?
2. Do we need to distinguish somehow that the block has been duplicated, or is that implicitly expected? Like adding a placeholder (copy) perhaps?
3. I feel a bit uncomfortable when working with inline blocks, when there currently are no new parent revision when I save layout. Since, I can't assure old work will remain, because I'm overriding it unless, I tactically create a new revision on the parent beforehand, which is not very usable. This issue would solve that: #2937199: Track Layout override revisions on entities which support revisioning

I started playing some with Content Moderation, and I noticed this issue as well:

Also consider to #2942675: Layout builder should use the active variant of an entity to avoid orphaned revisions add as a blocker? We cannot properly Content Moderation integration without that issue solved.
I am mainly referring to the fact that blocks can be moderated as well.

Ok this patch does a few things(also sorry have not read #85 & #86 yet, thanks @johndevman for looking into these issues)

1. Removes changes in #53 where I added the 2 forms for the plugin. I realized that since when used with the Block UI placing the block will always involved saving a Block config entity we can use this as a trigger to save the Inline Block. This also means we can use the Block config entity delete hook to delete the Inline Block.
2. Adds an generic entity usage service. This service is different than file usage service because it doesn't require any changes to the entities being tracked like setting the file to temporary. It keeps records where $count == 0 to denote entities that were used but have had all there usages removed.
3. Adds an InlineBlockContentUsage service to keep track of Inline Block entities and make sure they are deleted when necessary. This logic is bit complicated I was doing this all in entity_crude hooks so I changed these hooks to just call the service.

   #2937199: Track Layout override revisions on entities which support revisioning keeps this from working correctly with revisions.

   \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockContentBlockTest::testDeletion is still not passing. My manual testing seems to indicate that it working so I have figure if the test is wrong or if there is some other bug.

   Except for Inline Blocks that are placed via the Block UI the Inline Blocks are actually deleted on cron not entity deleted. It is possible that for LayoutBuilderEntityViewDisplay we should just delete the Inline Blocks when the block is deleted.

   It also attempts to delete Inline Blocks if the entity is not a new revision on update.

Great work @tedbow, here's just a quick review when looking at first part of the patch (it's quite big). Will look in to it more in depth later.

1. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -38,3 +38,64 @@ function layout_builder_install() {
   +      'entity_id' => [
   

   An entity ID can also be a string, so I don't think we can store it as int due to generic usage.

2. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -38,3 +38,64 @@ function layout_builder_install() {
   +        'length' => 64,
   

   We can use EntityTypeInterface::ID_MAX_LENGTH instead.

3. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -38,3 +38,64 @@ function layout_builder_install() {
   +      // @todo will the user always be an entity. File usage uses object
   

   I think we should keep it open, if possible.

4. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -0,0 +1,184 @@
   +   * The name of the SQL table used to store file usage information.
   

   file usage => entity usage

5. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -0,0 +1,184 @@
   +   *   (optional) The table to store entitys usage info. Defaults to 'v'.
   

   Defaults to 'v' => Defaults to 'entity_usage'

6. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -0,0 +1,184 @@
   +   * Adds usage by ids.
   

   Nit: id => IDs?

7. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -0,0 +1,184 @@
   +  public function removeByEntities(EntityInterface $used_entity, EntityInterface $user_entity, $count = 1) {
   

   $count is not used here.

8. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -0,0 +1,184 @@
   +  public function getEntitiesWithNoUses($entity_type_id, $limit = 100) {
   

   Nice with $limit, was just thinking about it, when looking earlier in code.

9. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -0,0 +1,184 @@
   +  public function deleteByEntity(EntityInterface $entity) {
   

   Can we make the method name more explicit, is this used or user?

Ok, let's see what test bot says.

So I updated patch to delete node through UI, and it works. I also tested this manually. And I also tried deleting through Node::load(1)->delete() via CLI, which also worked. So the failing tests seems to be only happening in the test case...

I also updated testNoLayoutSave::revert to not assert empty blocks because with current logic in patch this is not the intended behaviour anymore.

This is just a rough start on working on test coverage for Entity Usage service. I think @tedbow planned doing some changes, so I just upload this now, that I can continue later when the API becomes more in its finished state.

Edit: Looks like random fails, since the diffs in this patch should not affect these tests.

re #95 thanks for starting \Drupal\Tests\layout_builder\Kernel\EntityUsageTest this patch expands on it which uncovered some bugs.

In this patch

1. Changed terminology from used/user to parent/child - much more readable
2. Added \Drupal\layout_builder\EntityUsageBase because some the methods didn't actually do any database logic. These were entity related helper methods just to improve DX.
3. Changed the logic in ::remove() to return the total remaining uses of the child entity not just by parent being removed.
4. Expanded test coverage

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockContentBlockTest.php
@@ -0,0 +1,539 @@
+    // Remove block from override.
+    // Currently revisions are not actually created so this check will not pass.
+    // @see https://www.drupal.org/node/2937199
+    /*$this->removeInlineBlockFromLayout();
+    $this->assertSaveLayout();
+    $cron->run();
+    // Ensure content block is not deleted because it is needed in revision.
+    $this->assertNotEmpty(BlockContent::load($node_1_block_id));
+    $this->assertCount(2, BlockContent::loadMultiple());*/

I have confirmed the patch here #2937199-13: Track Layout override revisions on entities which support revisioning allows these test lines to be uncommented.

So that will need to be blocker for this issue to work correctly

This does a few things

1. Prevent access to block_content entity update and delete routes if the block is not reusable. These blocks should never been edited or deleted outside of their parent entities.
2. Creates the entity_usage table in an update hook.
3. Updates the configuration for the "Custom Block Library" view to filter on reusable blocks. Non-usable blocks should not be shown here.
4. In an update hook adds a is reusable filter to this view. The config changes will only take effect on module install. It specifically only change the "Custom Block Library" view not other views against this entity type. Should we attempt to add the filter to all views against this entity type? I would think not be because then we would have to deal with relationships also where block_content entity type is brought in as a relationship.

#101

+++ b/core/modules/block_content/block_content.install
@@ -152,3 +152,34 @@ function block_content_update_8600() {
+ * Update 'block_content' view to filter on new 'reusable' field.
...
+function block_content_update_8601() {

Can we do this? Given the user might have changed that view?

Nvm: Looks like this is being done in #2909435: Update block library page to adapt publishable block content implementation, so I suppose it's not an issue.

Am I the only one who thinks this issue is getting way too big ? :) Adding poormans-paragraphs *and* entity_usage in a single issue?

Did not actually review the patch yet, but it's 80+ kb changes, a lot of it on an existing stable module/entity type and I guess there is still a lot to do? If we do really want to go down that path, would it maybe make more sense to do this with a completely new, experimental module/entity type? I'd be much less worried about introducing such a huge change then and we would have a clean separation and not have to worry about existing views and all that.

#104

Am I the only one who thinks this issue is getting way too big ? :) Adding poormans-paragraphs *and* entity_usage in a single issue?

I think this part in the IS summarises it well:

This issue actually include several concepts that should probably be split out into separate issues.
To determine if end goal is achievable and advisable with this approach lets keep this as a big patch containing the sub tasks until it looks like it is good idea. We won't want to commit small supporting parts if we are going abandon this approach for something like #2948064: Layout Builder should support inline creation of Custom Blocks using entity serialization(currently that issue seems not doable)

...

Did not actually review the patch yet, but it's 80+ kb changes, a lot of it on an existing stable module/entity type and I guess there is still a lot to do? If we do really want to go down that path, would it maybe make more sense to do this with a completely new, experimental module/entity type? I'd be much less worried about introducing such a huge change then and we would have a clean separation and not have to worry about existing views and all that.

I think most of the required changes are present in this patch now.

Yeah, it's not a bad idea. Introducing a new entity type would most likely make some issues easier to solve. For instance, we could have a parent id stored on that entity type and no longer need the entity_usage?
Though, if we want that new entity type to be re-usable as well, we are just implementing Block content again, but with the use case flipped. If not, a User might need to maintain both a Inline block and a Content block entity type doing the same thing, would that be reasonable?

Fine if this is just a place to experiment, did not read the updated issue summary, sorry :)

Well, if they want to use a reusable block they can still use a standard block_content entity, because with reusable, the expectations on revisions are very different and while the UX might not be great when editing things inline (same with media and other similar use cases), it can't be by-reference-id because editing the block over the overview must work. That's what workspaces are for then.

@Berdir thanks for taking look again. Yes it is huge issue but I think it is still an experiment. Using a different entity might eliminate the need for entity tracking for instance.

For using a new entity type instead of block content:

Pros:

1. Don't have add property to existing entity type
2. Don't have to alter existing views
3. Might not have to add entity usage tracking service(if we can add parent id to new entity type)
4. Don't have to add access check on existing for block content edit and delete forms(b/c they shouldn't be update outside of parent)
5. Having the parent_id on the new inline_content entity type as base entity reference field would add possibilities for making relation ships via Views This would only work if we had ERR
6. All the changes could happen inside Layout Builder which is still an experimental module(though beta so we still have BC concerns)

Cons:

1. Existing sites would have to configure bundles instead of using existing ones
2. Blocks that were inline vs existing will have different bundles so they will have different field configuration. If would want a "Video Block" or "Gallery" block to be usable with block UI and layout builder inline block you would have to keep the the fields in sync.
3. Theme inline and reusable blocks would be different.
4. New permissions to manage(this could be Pro for more control)

For the biggest win from a site builder perspective for using the existing "Block Content" entity type would be the same block types could be used in both situations. I assume this would be much easier for theming but that is out of experience area.

Maybe though if this is really a "poor man's paragraph" then having different block types will not be a drawback as this what people are use to with paragraphs.

re:

Blocks that were inline vs existing will have different bundles so they will have different field configuration. If would want a "Video Block" or "Gallery" block to be usable with block UI and layout builder inline block you would have to keep the the fields in sync.

and:

Theme inline and reusable blocks would be different.

Is there some clever way of making a new entity type that actually just inherits the bundle configuration and theme hooks of another entity type? (and disable Field UI for the new entity type). That all might be confusing as hell for developers and a bastardization that we don't want in core, but the idea just occurred to me.

I'd vote for allowing the same block entity bundles to be useable here, it feels like an arbitrary separation otherwise.

Updating the title for now. Need to update the summary soon.

Created a parent issue #2974864: [Meta] Determine best method of allowing inline content creation Layout Builder to discuss the differences between using Custom blocks, i.e, this issue and using a new entity type, a new issue #2974860: Allow inline content creation in layout builder through a single purpose entity type

This patch creates \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestBase which will be shared with #2974860: Allow inline content creation in layout builder through a single purpose entity type.

This ensure we are testing the same functionality where appropriate. @see #2974864-7: [Meta] Determine best method of allowing inline content creation Layout Builder for reasoning

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php
@@ -533,7 +541,28 @@ protected function addInlineBlockToLayout($title, $body) {
+  protected function loadBlock($id) {
+    return $this->loadBlock($id);

Copy & paste recursion error 🙀

in the comment 5 on #2974864-5: [Meta] Determine best method of allowing inline content creation Layout Builder I was worried about

I thought of another problem with [THIS_ISSUE] block access.
We have to worry about the case where the site builder either creates view that exposes list of the inline blocks or sets up a entity reference to point to inline blocks.

Because determining view access will be tricky, based on view access the revision that inline block placed on.
Right now \Drupal\block_content\BlockContentAccessControlHandler::checkAccess just gives view access if the block is published.

I go on for another 5 paragraphs after... 😜

I am hoping we can actually handle this access problem.

This patch does

1. Adds AccessDependentInterface(and AccessDependentTrait to implement it). This simply has getter and setter for an access dependee.
2. InlineBlockContentBlock and BlockContent both implement AccessDependentInterface
3. In \Drupal\layout_builder\EventSubscriber\BlockComponentRenderArray::onBuildRender set call $block->setAccessDependee($entity) passing on layout entity as a dependency.
4. In InlineBlockContentBlock::getEntity() pass on dependency to BlockContent
5. Now when ::\Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::blockAccess get called the block entity will have the dependency set
6. Also the dependency will set in \Drupal\layout_builder\Plugin\Block\InlineBlockContentBlock::build this allows field blocks on Block entities one level down to still be rendered in the layout builder default. (i.e. Node entity -> Content Block -> field block body(checks entity access) 💥🧠)
7. Updates BlockContentAccessControlHandler to check non-reusable block and checks the access on the dependee.
8. All existing blocks will be reusable so this logic should not effect them.

So #117 stop you from being able to view non-reusable blocks but they will still should up as referenceable entities in entity reference field.

This is because:

1. The block_content module does not provide a EntityReferenceSelection plugin
2. The \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildEntityQuery does add the tag 'block_content_access' but the block_content module does not do anything with this because up to tell now this was not a problem. Blocks really didn't have access control

We could add a EntityReferenceSelection plugin to the block_content module to override buildEntityQuery but this would not effect any custom EntityReferenceSelection plugins that currently exist and are used with entity reference fields targeting block_content entities.

Instead I am adding block_content_query_block_content_access_alter which:
Checks for tags 'entity_query_block_content', 'entity_reference', 'block_content_access'(because of function name)
checks for table block_content_field_data
checks for any conditions against block_content_field_data.reusable(nested also)
If there are no conditions set against this field and all other conditions are true the it $query->condition('block_content_field_data.reusable', TRUE);

This should stop the DefaultSelection plugin from returning any non-reusable block entities. It should also work against other EntityReferenceSelection plugins unless they are adding the proper tags in which we should not alter them.

Unable to generate test groups

🙀
Fixed.

Doing interdiff against #117

Working on big self-review patch

1. +++ b/core/modules/block_content/block_content.services.yml
   @@ -4,7 +4,3 @@ services:
   -  access_check.entity.block_content:
   -    class: Drupal\block_content\Access\ReusableAccessCheck
   -    tags:
   -      - { name: access_check, applies_to: _is_reusable_block_content }
   

   This service to check route access is not no longer needed because the logic has been moved into the access controller.

2. +++ b/core/modules/block_content/src/BlockContentAccessControlHandler.php
   @@ -19,26 +19,23 @@ class BlockContentAccessControlHandler extends EntityAccessControlHandler {
   -    if ($operation === 'view') {
   +    /** @var \Drupal\block_content\BlockContentInterface $entity */
   +    if (!$entity->isReusable()) {
   

   checking all operations. This means you can't update non-reusable custom online blocks outside of layout builder.

3. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentEntityReferenceSelectionTest.php
   @@ -33,6 +33,8 @@ class BlockContentEntityReferenceSelectionTest extends KernelTestBase {
   +    $this->installSchema('system', ['sequence']);
   +    $this->installEntitySchema('user');
   

   This test was passing locally for me but not on DrupalCI. Seeing if this fixes it.

4. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -130,35 +130,31 @@ public function listUsage(EntityInterface $child_entity) {
   +    $sub_query = $this->connection->select($this->tableName, 't2');
   +    $sub_query->where('t1.entity_id = t2.entity_id');
   +    $sub_query->condition('t2.entity_type', $child_entity_type_id)
   +      ->condition('count', 0, '>');
   +    $sub_query->fields('t2', ['entity_id']);
   +    $query->notExists($sub_query);
   

   Implement the $limit logic which needed to actually be converted to use ::notExists

5. +++ b/core/modules/layout_builder/src/DatabaseBackendEntityUsage.php
   @@ -130,35 +130,31 @@ public function listUsage(EntityInterface $child_entity) {
   -  public function delete($child_entity_type_id, $child_entity_id) {
   +  public function deleteMultiple($child_entity_type_id, array $child_entity_ids) {
   

   This is more efficient than having many separate update calls.

6. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -39,13 +39,6 @@ class InlineBlockContentBlock extends BlockBase implements ContainerFactoryPlugi
   -  /**
   -   * The Drupal account to use for checking for access to block.
   -   *
   -   * @var \Drupal\Core\Session\AccountInterface
   -   */
   -  protected $account;
   

   $account was never used after being set.

7. +++ b/core/modules/layout_builder/src/Plugin/Block/InlineBlockContentBlock.php
   @@ -154,7 +143,6 @@ public function blockForm($form, FormStateInterface $form_state) {
   -    $form['title']['#description'] = $this->t('The title of the block as shown to the user.');
        return $form;
      }
    
   @@ -270,6 +258,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   
   @@ -270,6 +258,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
          // can't check $this->getEntity()->isNew().
          unset($form['label']['#default_value']);
        }
   +    $form['label']['#description'] = $this->t('The title of the block as shown to the user.');
   

   This wasn't actually showing. Fixed

8. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php
   @@ -310,6 +310,7 @@ protected function assertSaveLayout() {
   +    $this->assertNotEmpty($assert_session->waitForElement('css', '.messages--status'));
   

   I was getting random failures that I think was because there was no wait for the message.

Whoops logic error in \Drupal\block_content\BlockContentAccessControlHandler::checkAccess

Fixed

This patch does

1. Refactor InlineBlockTestBase to use existing method addInlineBlockToLayout() for all block placements.
2. Refactor InlineBlockTestBase to sue new method configureInlineBlock() for configuring existing blocks
3. Remove unneeded test module dependencies in InlineBlockTestBase
4. Change EntityUsageTest to not create actual entities since most method accept just IDs. Mock an entity when needed.
5. Simplify the logic in \Drupal\block_content\BlockContentAccessControlHandler::checkAccess(), no change in functionality.
6. Update BlockContentEntityReferenceSelectionTest to make sure the block is referenceable when change to reusable.

Updated Issue summary.

I created 2 child issues
#2976334: Allow Custom blocks to be set as non-reusable adding access restriction based on where it was used.
#2976366: Create generic entity usage tracking service

So here is a do-not-test patch with the other code stripped out.

In [#2976366-] @phenaproxima rightly stressed how problematic entity usage tracking could be based on our experience with File usage.

See #2821423: Dealing with unexpected file deletion due to incorrect file usage.
but we may have need for Media usage tracking #2835840: Track media usage and present it to the site builder (in the media library, media view, on media deletion confirmation, etc.)

But that got me thinking:

Just seems weird to make the generic entity tracking when our first use case is expressly for for entity that should not be reused.

These inline *non-reusable* blocks are *owned* in a real sense by the layout for the entity they are placed in. We only need to track them because there could hundreds referenced in a single entity because we could have hundreds of revision each with their own *non-reusable* blocks. So I think we could get a timeout if we tried to delete them in hook_entity_delete.

So basically are use of tracking is not to determine if the entity is use still anywhere in the system. Each non-reusable block as used by the Layout Builder will only have 1 entity that is using it.

So this patch adds non-generic usage tracking only for this use case to the Layout Module itself.

Whoops not all the files got in the combined patch. the interdiff and do-not-test patches should be the same.

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php
@@ -0,0 +1,549 @@
+    $assert_session->linkExists('Save Layout');
+    $this->clickLink('Save Layout');
+    $this->assertNotEmpty($assert_session->waitForElement('css', '.messages--status'));

This seems to fail on DrupalCI and randomly fail on my local.

I am not sure why. When I output the page when the message is not there after the wait then it is still on the layout builder page. But usually it passes.

I am changing this for now to manually drupalGet() the href of the link. This seems to fix the random failure for me.

It will at least let us see if the rest of the test passes. Anyone else has any ideas why this might happening?

This patch removes all logic from \Drupal\layout_builder\InlineBlockContentUsage that does deal directly with the usage tracking.

I added \Drupal\layout_builder\EntityOperations that is called from the entity crud hooks and cron. It calls the usage service.

This seems cleaner.

1. Until #2937199: Track Layout override revisions on entities which support revisioning is fixed we can't know when to make revision of a inline block. So that would mean we would never make revisions for inline blocks and looking back at previous revisions would always give you same revision and we would lose all changes.

   To get around this for now I am making a new revision of the inline block whenever block is edit in a layout and the parent entity supports revisions. If the layout is updated but a particular existing inline block is not edit then this will not cause a new revision. This makes \Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTestBase::testInlineBlocksRevisioning() pass for me locally.

2. For the other test failure I can't reproduce them locally so I am going to add small wait and see if it is a timing issue on DrupalCI

• Ok the problem with the test failure was actually be
  

  +++ b/core/modules/layout_builder/layout_builder.install
  @@ -38,3 +40,75 @@ function layout_builder_install() {
  +      'parent_entity_id' => [
  +        'description' => 'The ID of the object using the entity.',
  +        'type' => 'varchar_ascii',
  +        'length' => EntityTypeInterface::ID_MAX_LENGTH,
  

  This was actually causing the last test failure.

  The id length was too short and getting a truncate error.

Self review, big interdiff but hopefully b/c simplifying.

1. Currently the patch allows adding inline blocks in the Block UI. Since this an experimental module so we should only offer this in layout.

   re @Berdir in #14

   In general I agree with the idea but I don't think we should make it layout builder specific but allow all content blocks to be placed like this instead. That means also thinking about the exact wording for this and how the UI would work for normal blocks. And maybe eventually even deprecate the old way?

   If we want to add this functionality in the Block UI then we should create a follow up. The wording will be very important to not confuse users.

   For now I have implemented hook_plugin_filter_TYPE_alter to disallow this new block plugin except when the $consumer is layout_builder.

   I have also removed the logic from \Drupal\layout_builder\EntityOperations that handle block config entities that used this plugin.

2. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -38,3 +40,75 @@ function layout_builder_install() {
   +        'length' => 64,
   ...
   +        'length' => 64,
   

   Replace with EntityTypeInterface::ID_MAX_LENGTH,

3. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -38,3 +40,75 @@ function layout_builder_install() {
   + * Create the 'entity_usage' table.
   

   Table name wrong

4. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -56,6 +57,18 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +    // Set block access dependee even if we are not checking access on
   

   s/dependee/dependency

5. +++ b/core/modules/layout_builder/src/EventSubscriber/BlockComponentRenderArray.php
   @@ -56,6 +57,18 @@ public function onBuildRender(SectionComponentBuildRenderArrayEvent $event) {
   +    // this level. The lock itself may render another AccessDependentInterface
   

   s/The lock/The block

6. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTestBase.php
   @@ -0,0 +1,565 @@
   +/**
   + * Temporary inline-block base test.
   + *
   + * 2 issues are currently trying to solve the same problem of inline block
   + * creation. The 2 different methods should function the same way so the
   + * functional test should be nearly identical to make sure they do.
   + *
   + * @see https://www.drupal.org/node/2957425
   + * @see https://www.drupal.org/node/2974860
   + */
   +abstract class InlineBlockTestBase extends JavascriptTestBase {
   

   I think it is safe to remove this base class because this issue seems more promising than #2974860: Allow inline content creation in layout builder through a single purpose entity type

   This patch remove the need for the base class and simplifies InlineBlockContentBlockTest.

Review:

1. +++ b/core/modules/layout_builder/config/schema/layout_builder.schema.yml
   @@ -44,3 +44,20 @@ layout_builder.component:
   +      label: 'Block revisions'
   

   Block revisions => Block revision ID

2. +++ b/core/modules/layout_builder/layout_builder.install
   @@ -38,3 +40,75 @@ function layout_builder_install() {
   +        'description' => 'The name of the object type in which the entity is used.',
   ...
   +        'description' => 'The ID of the object using the entity.',
   

   Why do we use the phrasing object?

3. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +83,43 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +  $entity_operations->handlePreSave($entity);
   ...
   +  $entity_operations = \Drupal::service('class_resolver')->getInstanceFromDefinition(EntityOperations::class);
   +  $entity_operations->handleEntityDelete($entity);
   ...
   +  $entity_operations = \Drupal::service('class_resolver')->getInstanceFromDefinition(EntityOperations::class);
   +  $entity_operations->removeUnused();
   

   These can be chained in to a one line.

   i.e.

   \Drupal::service('class_resolver')->getInstanceFromDefinition(EntityOperations::class)
   ->handlePreSave($entity);
   

4. +++ b/core/modules/layout_builder/layout_builder.module
   @@ -81,3 +83,43 @@ function layout_builder_field_config_delete(FieldConfigInterface $field_config)
   +  if ($consumer !== 'layout_builder') {
   

   Could we just add a simple comment to explain why we do this?

5. +++ b/core/modules/layout_builder/src/EntityOperations.php
   @@ -0,0 +1,294 @@
   + * @internal
   ...
   +class EntityOperations implements ContainerInjectionInterface {
   

   Is this class even overridable?

6. +++ b/core/modules/layout_builder/src/EntityOperations.php
   @@ -0,0 +1,294 @@
   +    if ($entity->isNew() || !isset($entity->original) || $sections === NULL || ($entity->getEntityTypeId() !== 'entity_view_display' && empty($sections))) {
   

   Can we make a comment to explain better what we're not targeting here, or break them up to more return conditionals to make it more clear?

7. +++ b/core/modules/layout_builder/src/EntityOperations.php
   @@ -0,0 +1,294 @@
   +    if ($this->storage && $this->isLayoutCompatibleEntity($entity)) {
   

   Why do we need to check $this->storage?

8. +++ b/core/modules/layout_builder/src/EntityOperations.php
   @@ -0,0 +1,294 @@
   +   * @internal
   

   I don't think we need @internal here?

9. +++ b/core/modules/layout_builder/src/EntityOperations.php
   @@ -0,0 +1,294 @@
   +      $sections_field = $entity->get('layout_builder__layout');
   +      return $sections_field->getSections();
   

   Can this be a one-line?

10. +++ b/core/modules/layout_builder/src/EntityOperations.php
    @@ -0,0 +1,294 @@
    +    if ($sections = $this->getEntitySections($entity)) {
    

    I think we can opt-out early if there are no sections?

11. +++ b/core/modules/layout_builder/src/EntityOperations.php
    @@ -0,0 +1,294 @@
    +   * @param array $sections
    ...
    +   * @param \Drupal\layout_builder\Section[] $sections
    

    We should probably use same param type hint here.

12. +++ b/core/modules/layout_builder/src/EntityOperations.php
    @@ -0,0 +1,294 @@
    +      if ($block = $this->getPluginBlockRevision($component->getPlugin())) {
    ...
    +  protected function getPluginBlockRevision(PluginInspectionInterface $plugin) {
    

    Instead of loading all blocks, we could make a entity query instead