Duplicated and new entities with inline blocks do not duplicate their inner blocks and/or track their usage

1. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -583,4 +583,77 @@ public function testEditInlineBlocksPermission() {
   +   * Tests that Inline Blocks are added to cloned entities.
   

   I think we should use the term "duplicate" rather than "clone", given that's the method we're actually testing on the entity. Also maybe lower case on inline blocks instead.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -583,4 +583,77 @@ public function testEditInlineBlocksPermission() {
   +  public function testClonedEnitityInlineBlockUsage() {
   

   Enitity => Entity

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -583,4 +583,77 @@ public function testEditInlineBlocksPermission() {
   +    $this->drupalLogin($this->drupalCreateUser([
   

   Do we really need all these permissions?

The entity id in "block_usage" is empty with the last patch. This happens because the entity is empty in presave. I exclude the entity with empty ids onPresave and add them in hook_entity_insert.

1. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -196,6 +196,29 @@ public function handlePreSave(EntityInterface $entity) {
   +  public function handleInsert(EntityInterface $entity) {
   

   Shame that one of the existing methods is handleEntityDelete and the other is handlePreSave. Oh well, this one is probably better.

2. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -196,6 +196,29 @@ public function handlePreSave(EntityInterface $entity) {
   +    // If the parent entity is created with already associated blocks
   +    // the blocks must added after the entity is saved.
   +    // This happens e.g. the entity is duplicated.
   

   This comment should be rewrapped, and perhaps rewritten to make that e.g. less awkward

3. +++ b/core/modules/layout_builder/src/InlineBlockEntityOperations.php
   @@ -276,7 +299,7 @@ protected function saveInlineBlockComponent(EntityInterface $entity, SectionComp
   +    if ($entity->id() !== NULL && ($duplicate_blocks || (empty($pre_save_configuration['block_revision_id']) && !empty($post_save_configuration['block_revision_id'])))) {
   

   Why not isNew() here?
   If there's a very good reason, please add a line explaining why.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/InlineBlockTest.php
   @@ -587,4 +587,88 @@ public function testEditInlineBlocksPermission() {
   +    /** @var \Drupal\Core\Cron $cron */
   +    $cron = \Drupal::service('cron');
   ...
   +    $cron->run();
   

   IMO this can be inlined

Maybe I've overlooked it and this patch does already duplicate inline blocks, but inline blocks are supposed to be single-use blocks afaik. Meaning they should only be tied to a single parent. Updating block usage sounds like the incorrect approach. The inline blocks should simply also be duplicated if their parent is duplicated.

Per the feedback in #3062819-13: Duplicated and new entities with inline blocks do not duplicate their inner blocks and/or track their usage:

• I updated the comment in 3062819-13#2 to be less awkward.
• Since it's been a year on 3062819-13#3, should we just replace with isNew()?
• I checked 3062819-13#4, and I suppose it kept the separation on $cron to keep in-line between testDuplicateEntityInlineBlockUsage (uses cron once) and testDeletion (uses cron a few times). Is keeping that consistency in test design helpful, or nah?

Leaving this Needs Work and sans tests for the remaining two bullet points that I don't have an answer for.

Maybe I've overlooked it and this patch does already duplicate inline blocks, but inline blocks are supposed to be single-use blocks afaik. Meaning they should only be tied to a single parent. Updating block usage sounds like the incorrect approach. The inline blocks should simply also be duplicated if their parent is duplicated.

Yeah that's what I'd expect too

Tested #17 and it worked perfectly with https://www.drupal.org/project/quick_node_clone

The issue we are running into is cloning a page with inline blocks via layout builder appeared to clone but it wasn't duplicating the blocks. So we believe they were still tied today. Causing users to conflict with each other because if a block was edited in the source the cloned page would get the error the content had been edited by another user.

#17 has continued to work for us. Wondering if this can be marked as Reviewed by community?

I have a section storage for a new entity, (that hasn't been saved previously), so this patch actually fixes the problem that inline blocks gets saved with a NULL entity_id (which my code later needs to remove, and re-add usages).

+1 for getting something like this in.

I suppose I need to update the issue summary on this to mark where we are, I can take a first pass at that Monday to make our future direction succinct (from feedback in #20 and #21).

Let's circle back to feedback in #3062819-13: Duplicated and new entities with inline blocks do not duplicate their inner blocks and/or track their usage and apply it into the patch in #3062819-17: Duplicated and new entities with inline blocks do not duplicate their inner blocks and/or track their usage.

Specifically:

• #3062819-13: Duplicated and new entities with inline blocks do not duplicate their inner blocks and/or track their usage comment #3 suggested that if we don't use isNew(), we provide a comment justifying the rationale ("If there's a very good reason, please add a line explaining why."

#3062819-13: Duplicated and new entities with inline blocks do not duplicate their inner blocks and/or track their usage comment #4 feels to be a personal preference/code clean-up, which I suppose can be added into it as well.

Can someone implement the bulleted to-do above, so we can mark this for review by @tim.plunkett and company?

Confirming #17 fixes the issue. Should this be marked as Reviewd?

Similar to the comment in #21... #17 fixed my issue with a migration of nodes with Fieldable Panel Panes that were being converted to inline blocks within Layout Builder. Before the patch the inline_block_usage table was getting layout_entity_type of "node" but the layout_entity_id column was NULL due to the entity being new. Trying to view the node/12345/layout tab was broken. After the patch the node ids are being populated correctly and tab loads fine.

This is not correct. You can not have more than one usage for an inline block. If another module clones the node, it's their responsibility to also clone the inline blocks and add the block usage for those new blocks. Adding block usage for whatever blocks are linked to a node in a hook_entity_insert, sure, it fixes those modules but it also hides it when someone does it incorrectly and only clones the node and not its layout and the inline blocks there. This will cause a whole host of difficult to trace issues down the line, I can almost guarantee it.

The test that's been written even demonstrates the issue. A single inline block, linked to two nodes' layouts. This is *not* supported. It is in fact partly why inline blocks were created; single use. I believe there is an issue somewhere to allow users to select whether or not the block they are creating can be reusable, but until that makes it in they must remain single use.

If you're cloning an entity in custom code, it's up to you to clone all relevant data (including layouts and inline blocks) and do whatever else is needed to make the clone succeed (in this case add block usage for the newly cloned inline blocks). If a contrib does it, it is equally up to that module to take all the needed steps. It is not up to core to slap a bandaid on it that will mask more issues than it solves.

Now, if contrib says "we can't do it cleanly, we need an event to listen to", that is something core could add.

Might be related to #3195047: getBlockIdsForRevisionIds should query all revisions.

I'm testing this patch, coming from quick_node_clone module: https://www.drupal.org/project/quick_node_clone/issues/3100117
Patch from #17 fixes the issue for me.

@Grayle: I don't think there is a risk af adding duplicate usages for 1 inline block since this patch specifically deals with adding usages for cloned inline blocks (cloning takes places before the parent entity has an id). Unless this patch could cause duplicates in other circumstances?

Honestly, I feel that the whole approach of the inline_block_usage table is problematic in a number of ways. I think that a much more bullet-proof approach would be to implement a stateless strategy to managing inline block usage. The approach taken by Entity Usage seems to be a lot less problematic. If when things get out of sync, just revalidate state.

Of course, this could come with scalability problems as installations grow to hundreds of thousands of content items, but at least it'd be a lot safer than assuming that contributed / custom code didn't poke the database table in a strange way and cause unpredictable data loss that might only manifest hours / days / weeks later on cron run.

Both entity_clone and replicate modules are currently bugged when cloning Layout builder nodes with inline blocks. As previously stated in this issue, the blocks need to be cloned as well. That in turn should add usage

#3050027: Inline Blocks on Layout Builder Fail to Clone Correctly
#3100763: Replicating node with non-reusable block added via layout builder does not replicate the block

This is a clean re-roll of #17 for 9.5.5 in prep for next patch.

This builds on #35 and includes a fix that addresses the issue noted in #32 and earlier that comes up when using entity_clone, mentioned in #48 on this entity_clone issue: https://www.drupal.org/project/entity_clone/issues/3050027#comment-14915222

When cloning, getPluginBlockId($plugin) uses the revision ID of the new revision on the new block to retrieve the block id from a db query. Due to the order of operations here, that id may not be returned from the db query. This causes addUsages() to fail because it is passed a null value for the id.

In this case, the new block is referenced by the plugin. The plugin's getEntity() is a protected method, so we use reflection to call it and get the entity id from there. Then we pass the id to the addUsages() function like would have happened otherwise:

      $id = $this->getPluginBlockId($plugin);
      if(!$id){
        // The standard getPluginBlockId() didn't return an id, let's use
        // reflection to get it off of the $plugin.
        // We have to use reflection for this as it's a protected method.
        $reflectionMethod = new \ReflectionMethod($plugin, 'getEntity');
        $reflectionMethod->setAccessible(TRUE);
        $new_entity = $reflectionMethod->invoke($plugin);
        $id = $new_entity->id();
      }
      $this->usage->addUsage($id, $entity);

Confirming that patch in #36 works with 10.2.6 core.
Errors we found before the patch with all clone modules like quick node clone:
- Deleting a block from the original entity throws a missing block error on the cloned page.
- Adding new media to cloned pages throws a access dependency error and therefore when you clock add media nothing happens.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

I'm still facing the issue in Drupal 10.2.6 after applying the patch #36

inline block content is showing as the last updated/translated block content.I'm using layout builder st. I faced the same issue in Drupal 9. After applying the patch #16 it got resolved. Recently I upgraded to drupal 10. Now again I'm getting the issue. I've already tried #36 patch.

Thanks in advance.

We get the error in 10.3.0 as well.
Layout builder st is also used.

I updated patch #36 for Drupal 10.3.0, correcting only the positioning of changes within InlineBlockEntityOperations.php, which previously caused the patch application to fail.

Hi @remoschneider,

Please, refrain from posting a patch that isn't functionally brought up to date.
The patch you posted applies, but is buggy due to the lack of the 'getPluginBlockId'-method in the InlineBlockEntityOperations class.

I'll post a patch that is functionally in line with the old one, if someone wants to turn it into an MR.

Was getting this error upon editing and saving a LB page on 10.3.1:

Error: Call to undefined method Drupal\layout_builder_st\InlineBlockEntityOperations::getPluginBlockId() in Drupal\layout_builder\InlineBlockEntityOperations->saveInlineBlockComponent() (line 259 of /var/www/html/docroot/core/modules/layout_builder/src/InlineBlockEntityOperations.php)

I uninstalled layout_builder_st, but was still getting the error, although this time pointing to `layout_builder`:

Error: Call to undefined method Drupal\layout_builder\InlineBlockEntityOperations::getPluginBlockId() in Drupal\layout_builder\InlineBlockEntityOperations->saveInlineBlockComponent() (line 259 of /var/www/html/docroot/core/modules/layout_builder/src/InlineBlockEntityOperations.php).

I then updated from #43 version of this patch to the version from #44 and issue was resolved.

I've had some strange behaviours on a site when submitting webforms with the entity_insert hook where we also have the hook_event_dispatcher module enabled:

TypeError: layout_builder_entity_insert(): Argument #1 ($entity) must be of type EntityInterface, Drupal\webform\Entity\WebformSubmission given, called in /app/web/modules/contrib/hook_event_dispatcher/src/HookEventDispatcherModuleHandler.php on line 53

I've added a patch to not strictly define the EntityInterface in the parameter but instead check if the $entity variable is an instance of EntityInterface and if not then get out of the hook.

Heres an updated version

The rerolled patch, which can be applied on 11.2.x, because #47 is not applicable.
Corrected only changes for `layout_builder.module` file.

Created a MR for D11 from the latest patch ... using OOP hooks.
Needs Review .. for (re)start.

Haven't review but can IS be updated to use the issue template please

Thanks.

As part of the IS update, can we get clarity on whether this issue is meant to address duplicating (via EntityInterface::createDuplicate()) or cloning (via clone) or both? Looking at #5 and the test in the MR, it seems like only createDuplicate() is being handled, and if so, the title should probably only use "duplicate" terminology.

Also, the entity_duplicate hook introduced in #3040556: It is not possible to react to an entity being duplicated and documented in CR https://www.drupal.org/node/3268812 seems to be a good fit for this use case?

With this change, there appears to be an error when trying to add a new user:

Steps to reproduce

• Running a fresh Drupal install from this issue fork
• Set the display for the account to use Layout Builder (/admin/config/people/accounts/display)
• Manage the layout for the display and add an inline content block (/admin/config/people/accounts/display/default/layout)
• Save the display layout
• Attempt to create a new user

This results in an error preventing the creation of the new user.
Error output:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO "inline_block_usage" ("block_content_id", "layout_entity_id", "layout_entity_type") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => 3 [:db_insert_placeholder_2] => user ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 815 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php). 

Echoing #53, `hook_entity_duplicate` should be used here (or at least explored) and hopefully should reduce the complexity.

> As part of the IS update, can we get clarity on whether this issue is meant to address duplicating (via EntityInterface::createDuplicate()) or cloning (via clone) or both?

We have at least three different words for the same thing: duplicate, clone and replicate in this context all mean the some thing. We have the node/entity_clone modules, we have the replicate modules and the core API which calls it duplicate. Not great and we're discussing in #3554633: Should Replicate UI become THE Drupal entity cloning module (and deprecate all others)? what to do about the naming situation in context of the replicate ui module that I maintain. The php clone keyword is different and not relevant here because that's on it's own (duplicate internally does a clone, but so do other cases like getting a translation) still the same entity with the same ID. That said, I'm +1 on on renaming this to duplicate as that is the core terminology, made a suggestion but it's a bit long.

Duplicate is not the only scenario in which the duplicate action is necessary, as this is already an existing case when the layout is customized. Additionally, this I believe is also the correct fix for #3359137: Update inline block usage on import, as any new entity with existing/inlined block entities needs *some* of this treatment. Which actually shows that the isNew() part on its own might not be enough just yet. Because a completely new entity doesn't need to duplicate if the blocks are also new and not yet used by something.

What I'd consider doing to avoid duplication is split the presave into the part that needs to happen *before* save (duplicate inline blocks) and then run the update-usage and cleanup part on both insert and update, we can now run the same method on both hooks with two attributes.

so to summarize:

* Do usage tracking and clean for both insert and update.
* Either on preSave detect new-entity-with-used-blocks or keep this and instead explicitly implement hook_entity_duplicate() additionally and not change the existing logic for that.

I did not review the stuff around getPluginBlockId(), I don't think using or not using hook_entity_duplicate() is going to simplify that, but not certain.

Coming here after creating an issue for the Replicate module that's related to this: #3583082: Inline blocks in layout builder pages are saved incorrectly. That issue is to fix an problem the Replicate module causes when forced revisions are enabled for an entity type being cloned (like those using Content Moderation). But the proper fix for it in Replicate would then expose the problem that I think this issue is covering, which is that inline block usage data for *new* entities is not saved correctly. Layout Builder assumes that the entity already has an ID when it saves usage data.

It seems like the MR here is on the right track by only saving the usage data in the insert hook after the ID has become available. But I'm not sure how complete this MR is.

Just a quick note that my plan is to deprecate the replicate module as a while as core now has the duplicate hooks. replicate_ui will receive a 2.x branch that will no longer depend on replicate and only use $entity->createDuplicate() directly. See #3554633: Should Replicate UI become THE Drupal entity cloning module (and deprecate all others)?,