Bug in `generateComponentTreeKeys()` causes XB config entity containing a component tree to loses some component instances upon creation

This sounds a lot like #3534971: ApiLayoutController::getRegionForComponentInstance doesn't work well with nested components if there are multiple regions.. 🤔

Looking forward to the additional info to reproduce this! 😊🙏

I also saw this issue outside xb-demo, and I didn't have any region enabled in my theme to be managed by XB.

I just checked, the POST request xb/api/v0/config/pattern doesn't contain the full tree of the selected component.

I see, so then … it must be a client-side problem? But the video in #6 does not show the request body, so it could still be the same back-end bug that causes the information the client sent to get lost 😅

I'm going to bet that this is a back-end bug, and just another symptom of #3534971-6: ApiLayoutController::getRegionForComponentInstance doesn't work well with nested components if there are multiple regions.. Bumping the priority of that one. Odd that this was found so late!

right payload is ok, so backend issue.

Yeah then I'm 99% confident it's a duplicate of that other isssue. 😇

Let's first land that other issue, then test again here.

I added several nested code components inside two-column components and created a pattern with them. However, I noticed that some components were not included in the resulting pattern. @wim-leers this issue is not resolved with #3534971

Too bad #3534971: ApiLayoutController::getRegionForComponentInstance doesn't work well with nested components if there are multiple regions. didn't solve it. It's the same fundamental problem.

In this case though, \Drupal\experience_builder\Controller\ApiConfigControllers::post() is what creates Pattern (and other) XB config entities. That calls Pattern::createFromClientSide(), which ends up calling \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer().

Apparently ApiLayoutController contains an alternative implementation of fundamentally the same logic 😞 That's been a known issue for months. We've been prioritizing new features over refactoring/cleaning up, so I'm not surprised we never got to those.

My latest thinking and write-up about this was … on February 20 (almost 5 months ago!!!) at #3503239-4: [PP-1] Make use of the serializer for normalizing/denormalizing config entities in ApiConfigControllers.

I've started some investigation into this and it doesn't look to be the same issue as #3534971 and looks to be due to #3526127 and the way that the function generateComponentTreeKeys() builds the keys.

The array that is passed into the generateComponentTreeKeys() function contains all 10 items in the tree in my case, but when I go through the function I only end up with 8 items.

I did a test by commenting out the set() function in Pattern entity and everything saves as expected so that looks to prove where the root cause looks to be. Will continue to debug this further.

Will try to move this along a bit while @thoward216 sleeps 🧚‍♀️

Was able to write a test using @thoward216's excellent sleuth work at #16 - confirming this is not related to #3534971: ApiLayoutController::getRegionForComponentInstance doesn't work well with nested components if there are multiple regions. or #3526127: Ensure deterministic config export order of config-defined component trees

Pushed a failing test replicating #16

As the bug is in \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::generateComponentTreeKeys, this also impacts ContentTemplates - new title

Pushed a fix with some expanded docs of what is going on in that method.

Thanks @larowlan - I've manually tested this and everything is working as expected and the output of the component tree keys is what I was expecting to see also. I've just opened an MR to run the pipeline.

Whoops I forgot that, thanks

I've left some minor comments. Overall, It looks good to me though

Test-only CI job failed 👍

Requested a few clarifications, specifically around the continue — I'm not convinced yet it is necessary. It's better to fail explicitly than silently ignore problems — unless there's a good reason, and then that should be documented 🙏

AFAICT this could also affect PageRegions, because that's the third config entity type that relies on \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::generateComponentTreeKeys()

This boils down to data loss, so … tagging backport.

After writing #30, one thing didn't sit right: we're only adding test coverage for Pattern, but per @larowlan in #20, this affects ContentTemplates too, and per my #30, PageRegions too.

We should test all 3, generically. Made it so.

IOW this was a bug in #3526127: Ensure deterministic config export order of config-defined component trees — the test coverage that that added and this issue expanded has now been generalized using a new \Drupal\Tests\experience_builder\Kernel\Config\ConfigWithComponentTreeTestBase.

I expect linting errors but tests to pass — will fix after lunch 👍

@lauriii confirmed the backporting of this to 0.x in Slack :)