Error: [attachToSlot] moveToSlot failed with invalid data

Hi, thanks for your ticket.

Can you provide a way to reproduce the issue? You said it happens often to you, maybe there is a pattern to identify here.

I can reproduce, seems there is a problem with our changes on moveToSlot.

The message is wrong because of a wrong copy, will fix on this MR to have proper message.

From new instance

• add a component with one slot (ex: bootstrap button)
• add the component 2 times inside the previous slot
• add textfield in each slots
• refresh page
• move around textfield and components in the slots, before, after, inside, component...
• the error will occur

Sometimes it can occur without textfield, just moving around same 2 components in a slot, move order or in and out.

Removed librairies data from issue error message, it's not relevant.

I have the error from $data = NestedArray::getValue($root, $path);, when the error occur, $data is null.

At some point when moving around from one slot to another, $root start on index 1 instead of 0, the 0 index is strangely removed.

Looks good, removed some InstanceHistoryTest for now as they were not relevant.
I still ave a problem with testMoveFromComponentToNestedLayoutEmptyRegion(), @pdureau could you check?

I see you are unifying the move to same slot and different slot by removing ::isNodeAlreadyInSlot(), ::changeSourcePositionInSlot() and ::doMoveToSameSlot(). that's very cool.

I still ave a problem with testMoveFromComponentToNestedLayoutEmptyRegion(), @pdureau could you check?

Indeed, the test is skipped and fails if executed:

1) Drupal\Tests\display_builder\Kernel\InstanceMoveToSlotTest::testMoveFromComponentToNestedLayoutEmptyRegion
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+    0 => Array &1 [
+        'node_id' => 'source_1',
+        'source_id' => 'textfield',
+    ],
+]

Step 1

This phpunit fails is not the real issue. First, we need to adapt the test to the new logic which is rebuilding the index during the moveToSlot() (maybe not at the right moment, but we will see later).

I have pushed a commit in a side branch: https://git.drupalcode.org/issue/display_builder-3576984/-/commits/35769...

Step 2

Now we have the real issue:

1) Drupal\Tests\display_builder\Kernel\InstanceMoveToSlotTest::testMoveFromComponentToNestedLayoutEmptyRegion
Failed asserting that a NULL is not empty.

/var/www/html/web/modules/custom/display_builder/tests/src/Kernel/InstanceMoveToSlotTest.php:281

And we see the source we are moving around has disappeared from the source tree!

The first moveToSlot is chaining:

1. doRemove
2. doAttachToSlot
3. doUpdateSlotValue

The second moveToSlotis chaining:

1. doRemove
2. doAttachToSlot

doUpdateSlotValue() is not executed because the parent data is not found. That's weird because when i check, i found the expected data.

Step 3 ?

I guess we may have an other issue here because we are rebuilding the index in a "Internal atomic change of the root state." method.: at the last line of ::doUpdateSlotValue()

In my opinion, this must not happen. It is better to chain the "internal atomic changes of the root state" consistent indexes with and altered but not rebuilt index. Before and/or after those steps, it is OK to rebuild the index.

We may be safe here because ::doUpdateSlotValue() is the last step of the chain, so rebuilding the index is not messing with next steps, but I am afraid it is not future proof.

What can we do to be 100% safe and make this clear for next updates? What do you think about moving the atomic methods to a tiny utility class:

• called by Instance entity, and only Instance entity, and calling nothing else
• fully stateless, with only static methods
• no index manipulation
• no history awareness

Does it makes sense?

I am full for moving this data tree manipulation to it's own class, I am starting that on a new branch.

Added a class, we store a 'simple' tree now with only ids to have something lighter and a flat list of source values by ids.

The sources 'layout' and 'page_layout' with their 'regions' brings some complexity for nothing, would be great if they match the 'component' slots.

For now I probably removed the case of move to the same place, but it's more important to resolve the current bug.

The sources 'layout' and 'page_layout' with their 'regions' brings some complexity for nothing, would be great if they match the 'component' slots.

They will not, unfortunately. Component slots were badly modeled (unexpected "component" and "sources" levels) when we built UI Patterns 2 in 2024 and we must deal with legacy until at least UI Patterns 3.

For now I probably removed the case of move to the same place, but it's more important to resolve the current bug.

Which case exactly? Moving a source to the same slot but at a different position ? This is a very common action.

The actual SourceTree in the MR doesn't meet the target in my opinion:

• fully stateless, with only static methods : ❌
• no index manipulation: ❌
• no history awareness: ✅

SDC slot manipulation is hardcoded in the logic, back to the situation before beta2 and SourcewithSlotInterface:

    if ($source_id === self::SOURCE_TYPE_COMPONENT && isset($item['source']['component']['slots'])) {
      foreach ($item['source']['component']['slots'] as $slot_id => $slot) {
        unset($item['source']['component']['slots'][$slot_id]['sources']);
      }
    }
    elseif (\in_array($source_id, [self::SOURCE_TYPE_LAYOUT, self::SOURCE_TYPE_PAGE_LAYOUT], TRUE) && isset($item['source']['regions'])) {
      foreach ($item['source']['regions'] as $slot_id => $region) {
        $item['source']['regions'][$slot_id] = [];
      }
    }

  /**
   * Source type: component.
   */
  private const SOURCE_TYPE_COMPONENT = 'component';

  /**
   * Source type: layout.
   */
  private const SOURCE_TYPE_LAYOUT = 'layout';

  /**
   * Source type: page_layout.
   */
  private const SOURCE_TYPE_PAGE_LAYOUT = 'page_layout';

So we are increasing coupling here where the opposite was expected.

I see the commit has been merged while being assigned to me for review. It is not common for me to ask such a thing but, considering the situation, I think we must revert it and start again from 3576984-attachtoslot-failed or 3576984-attachtoslot-failed-pierre

(misclick)

I have pushed an additional commit to the previous branch 3576984-attachtoslot-failed_pierre with a proposal of what could be such a utility class:

abstract class SourceTreeAtomicOperations {

  public static function attachToRoot(array $root, int $position, array $data): array;
 
  public static function attachToSlot(array $root, array $parent_path, string $slot_id, int $position, array $data): array;

  public static function remove(array $root, array $path): array;
   
  public static function getSlotSourcePlugin(array $data): ?SourceInterface {
   
  private static function getNode(array $root, $path): array;

  private static function updateSlotValue(array $root, array $path, SourceWithSlotsInterface $source, string $slot_id, array $slot_data): array;

}

The idea is to make every manipulation of the source tree as safe as possible, so each operations are moved there, in a totally stateless logic, with a total data consistency from the first step to the last, and easily testable with Unit Tests.

So, no index manipulation, no history awareness. Every method get a source tree as $root and returns the altered source tree and nothing else.

We can now mess around in Instance entity, with the business logic, the entity & CMS logic, the history logic, and the stateful index, without never ever breaking the source tree.

By the way, the remaining issue ("Failed asserting that a NULL is not empty" in InstanceMoveToSlotTest::testMoveFromComponentToNestedLayoutEmptyRegion) seems to be fixed now 😉The pipeline is green ✅

If you like this, we can move on together:

• adding the unit tests for this class
• what about the naming?
• was it a good idea to add \assert() in the beginning of each methods?
• was it a good idea to make ::getSlotSourcePlugin() public in order to avoid duplication in Instance ?
• ...

Also, not related to SourceTreeAtomicOperations but to previous works. Many tests have been removed from InstanceHistoryTest : ::testClear(), ::testRedo(), ::testHashDuplicateDetection(), ::testSetNewPresent(), ::testUndo(). Is it normal?

Seems there is other problems introduced...
I am going back to my branch SourceTree that resolve the issue, was fully covered by Unit tests and was totally isolated without any service injection.

Hi Jean, do you want me to review?

-      $is_move ? DisplayBuilderEvents::ON_MOVE : DisplayBuilderEvents::ON_ATTACH_TO_ROOT,
+      $is_move ? DisplayBuilderEvents::ON_MOVE : DisplayBuilderEvents::ON_ATTACH_TO_SLOT,

Does that means the root cause was #3540701: Wrong event triggered by ApiController::attachToSlot() since the beginning and there was nothing wrong with the current logic in Instance? 🙃 If yes, let's close the other issue as "duplicate" or "outdated".

That doesn't mean the current MR doesn't worth the hard work because:

• it is a good feeling to have the most important logic of the module moved out Instance entity class, so we can mess with the Entity API without breaking anything
• on the 1139 lines added, 736 (65%) were in tests

OK for me because:

• i didn't find any bug in my tests
• I see the changes are minimal from an API point of view: Interfaces stayed the same, current tests have been left intact