Skip config import if no instance entity

OK for me.

I see it was not merged yet. Maybe because of fails in phpunit. So back to "Needs work".

This fail was expected, and is related to changes in UI Patterns 2.0.16 released yesterday:

[
     0 => Array &1 [
+        'node_id' => '1',
         'source_id' => 'component',
         'source' => Array &2 [],
         'third_party_settings' => Array &3 [],
-        'node_id' => '1',
     ],
 ]

We just need to update the order of properties in the tests.

There are also 4 "Failed asserting that an array is empty." fails, because an empty array inside an empty array is not considered empty by PHP:

(
    [0] => Array (
            [node_id] => 
            [source_id] => 
            [source] => Array()
            [third_party_settings] => Array ()
        )
)

Investigating.

3 commits pushed on the MR, I guess this ticket is on your side.

Are you sure those changes in EntityViewDisplayTrait, API controllers, Instance entity and SourceTree are needed just because we have updated UI Patterns from 2.0.15 to 2.0.16?

But would be great to know if it's a problem from ui_patterns.

I was investigating that this afternoon. Argument #1 ($source_id) must be of type string, null given, called in SourceTree.php is caused by this (relevant) change in UI Patterns:

-class SourceValueItem extends MapItem {
+class SourceValueItem extends FieldItemBase {

Because we are losing the (maybe too) generous MapItem::isEmpty()

  public function isEmpty() {
    return empty($this->values);
  }

and relying on the more strict and convoluted Map::isEmpty().

So, with those 3 additional lines in UI Patterns + reordering the properties in InstancePublishingTest::testRestore() (2 lines) must be enough to fix, without the risk of changing our tests, the delicate SourceTree or the publishing logic.

I take the ticket back to finish the investigation and maybe to propose another MR (or two 😉).

I don't understand when I am supposed to work on this ticket or not 😕 it is the second time today commits are pushed while the ticket is assigned to me.

Here is my proposal: https://git.drupalcode.org/project/display_builder/-/merge_requests/296/...

UI Patterns 2.0.16 compatibility OK with only 2 lines changed in InstancePublishingTest::testRestore() & 3 lines added in #3604317: SourceValueItem missing isEmpty() causes phantom null items to survive filterEmptyItems() [AI assisted] (see my comment).

Without the risk of doing changes in the tests, the SourceTree, the controllers... parts of the codebase which are delicate and tricky.

What do you think?

Edit: I forgot to update composer.json

I will add the composer.json and also DisplayBuilderKernelTestBase::testGetParentId() from your MR because it add coverage without expecting a change of the existing logic 😉

Done.

Following this ui patterns bug, how could we improve the ui patterns release to avoid this kind of breaking changes?

I would not say those changes are made UI Patterns side (it has been a long time we are not changing anything in SourceValueItem) and are unfortunately breaking Display Builder.

Instead, those changes are made specially for Display Builder, to unlock features here, but in a confusing way because we didn't anticipate everything and it is taking us 3 tries to get things right:

• First, #3584856: Source field model and storage with most of the change for make SourceValueItem usable by Instance entity
• Then, #3601216: Add a main property to SourceValueItem because Instance entity is now able to do EntityQuery
• Finally #3604317: SourceValueItem missing isEmpty() causes phantom null items to survive filterEmptyItems() [AI assisted]

Once the fix in ui patterns is merged and released we'll be able to merge on Display Builder.

Great. Which MR?

Great. Which MR?

When this is merge #3604317: SourceValueItem missing isEmpty() causes phantom null items to survive filterEmptyItems() [AI assisted] and perhaps we have a release of ui_patterns.

I was asking about the MRs of the current ticket:

• the initial #294 MR
• the alternative, smaller, #296 MR

I am still afraid the first one is doing too much. For example, this change in InstancePublishingTest:

-    $instance->setNewPresent($testData, 'Modified state');
-    self::assertSame($testData, $instance->getCurrentState());
+    $instance->setNewPresent([$testData], 'Modified state');
+    self::assertSame($testData, $instance->getCurrentState()[0]);

And this change in InstanceTest:

-    self::assertEmpty($instance->getCurrentState());
+    $node = $instance->getNode($node_id);
+   self::assertEmpty($node);

Looks like leftovers of a previous stage of the investigations Potentially useless and harmful now we have moved the focus to #3604317: SourceValueItem missing isEmpty() causes phantom null items to survive filterEmptyItems() [AI assisted].

It would be safer to focus on the change of the 2 lines in InstancePublishingTest::testRestore(), without too much modifications unrelated to the ticket scope.

Maybe it is just me freaking out, and I would be happy to be contradicted and reassured 🤗