Integrate saving sections with the backend: (de)normalize Pattern config entities using the client-side data model

👍 Thanks!

This issue's scope will be actually integrating the frontend pieces with the new backend endpoint(s)? for saving a section.

Yep, but the endpoints actually already exist, i.e. the following routes:

• experience_builder.api.config.list
• experience_builder.api.config.get
• experience_builder.api.config.patch
• experience_builder.api.config.post
• experience_builder.api.config.delete

… but they currently only support the PageTemplate config entity, because that's the only XB-owned config entity type that implements \Drupal\experience_builder\Entity\XbHttpApiEligibleConfigEntityInterface — #3479643: Introduce a `Pattern` config entity will add the Pattern config entity type that also implements that interface 👍

#3479643: Introduce a `Pattern` config entity is in!

It seems that the FE is already connected but I'm getting an error when trying to use this. This probably needs someone on the backend to debug this.

The error with a backtrace:

Error: Call to undefined method Drupal\Core\Config\Schema\Undefined::getIterator() in Drupal\Core\Entity\Plugin\DataType\ConfigEntityAdapter->getIterator() (line 74 of /var/www/html/drupal/core/lib/Drupal/Core/Entity/Plugin/DataType/ConfigEntityAdapter.php).
Backtrace	
#0 /var/www/html/drupal/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(163): Drupal\Core\Entity\Plugin\DataType\ConfigEntityAdapter->getIterator()
#1 /var/www/html/drupal/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php(106): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode()
#2 /var/www/html/drupal/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php(93): Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate()
#3 /var/www/html/drupal/core/lib/Drupal/Core/TypedData/TypedData.php(132): Drupal\Core\TypedData\Validation\RecursiveValidator->validate()
#4 /var/www/html/drupal/modules/custom/experience_builder/src/Controller/ApiConfigControllers.php(106): Drupal\Core\TypedData\TypedData->validate()
#5 [internal function]: Drupal\experience_builder\Controller\ApiConfigControllers->post()
#6 /var/www/html/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array()
#7 /var/www/html/drupal/core/lib/Drupal/Core/Render/Renderer.php(593): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#8 /var/www/html/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(121): Drupal\Core\Render\Renderer->executeInRenderContext()
#9 /var/www/html/drupal/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()
#10 /var/www/html/drupal/vendor/symfony/http-kernel/HttpKernel.php(183): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#11 /var/www/html/drupal/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#12 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/Session.php(53): Symfony\Component\HttpKernel\HttpKernel->handle()
#13 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#14 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#15 /var/www/html/drupal/core/modules/big_pipe/src/StackMiddleware/ContentLength.php(32): Drupal\Core\StackMiddleware\ContentLength->handle()
#16 /var/www/html/drupal/core/modules/page_cache/src/StackMiddleware/PageCache.php(116): Drupal\big_pipe\StackMiddleware\ContentLength->handle()
#17 /var/www/html/drupal/core/modules/page_cache/src/StackMiddleware/PageCache.php(90): Drupal\page_cache\StackMiddleware\PageCache->pass()
#18 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\page_cache\StackMiddleware\PageCache->handle()
#19 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#20 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#21 /var/www/html/drupal/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle()
#22 /var/www/html/drupal/core/lib/Drupal/Core/DrupalKernel.php(709): Drupal\Core\StackMiddleware\StackedHttpKernel->handle()
#23 /var/www/html/drupal/index.php(19): Drupal\Core\DrupalKernel->handle()
#24 {main}

Here's the request:

fetch("https://drupal-dev.ddev.site/xb/api/config/pattern", {
  "headers": {
    "accept": "*/*",
    "accept-language": "en-US,en;q=0.9",
    "content-type": "application/json",
    "priority": "u=1, i",
    "sec-ch-ua": "\"Google Chrome\";v=\"131\", \"Chromium\";v=\"131\", \"Not_A Brand\";v=\"24\"",
    "sec-ch-ua-mobile": "?0",
    "sec-ch-ua-platform": "\"macOS\"",
    "sec-fetch-dest": "empty",
    "sec-fetch-mode": "cors",
    "sec-fetch-site": "same-origin"
  },
  "referrer": "https://drupal-dev.ddev.site/xb/node/1/editor/component/6e4aac04-cb0d-4431-87df-afaec573375a",
  "referrerPolicy": "strict-origin-when-cross-origin",
  "body": "{\"layout\":{\"nodeType\":\"root\",\"uuid\":\"root\",\"children\":[{\"children\":[{\"uuid\":\"6e4aac04-cb0d-4431-87df-afaec573375a-slot-column_one\",\"name\":\"column_one\",\"nodeType\":\"slot\",\"children\":[{\"children\":[],\"nodeType\":\"component\",\"type\":\"sdc.experience_builder.image\",\"uuid\":\"1b4b7c65-b356-4b22-abb9-d1bccaad4135\"}]},{\"uuid\":\"6e4aac04-cb0d-4431-87df-afaec573375a-slot-column_two\",\"name\":\"column_two\",\"nodeType\":\"slot\",\"children\":[{\"children\":[],\"nodeType\":\"component\",\"type\":\"sdc.experience_builder.heading\",\"uuid\":\"b1b626de-6653-453c-aa26-32e2a79c2c0e\"}]}],\"nodeType\":\"component\",\"type\":\"sdc.experience_builder.two_column\",\"uuid\":\"6e4aac04-cb0d-4431-87df-afaec573375a\"}]},\"model\":{\"6e4aac04-cb0d-4431-87df-afaec573375a\":{\"name\":\"Two Column\",\"width\":25},\"b1b626de-6653-453c-aa26-32e2a79c2c0e\":{\"name\":\"Heading\",\"text\":\"A heading element\",\"style\":\"primary\",\"element\":\"h1\"},\"1b4b7c65-b356-4b22-abb9-d1bccaad4135\":{\"name\":\"Image\",\"image\":{\"src\":\"https://placehold.co/600x400.png\",\"alt\":\"Boring placeholder\",\"width\":600,\"height\":400}}},\"name\":\"Two Column section\"}",
  "method": "POST",
  "mode": "cors",
  "credentials": "include"
});

It seems that the FE is already connected

It isn't? 🤔 That's what this issue is supposed to do!

AHHHHHHHH, I see now! @jessebaker added this in #3459229: Allow saving component compositions as sections (frontend only):

    getSections: builder.query<any, void>({
      query: () => `/xb/api/config/pattern`,
    }),
    saveSection: builder.mutation<
      { html: string },
      { layout: RootNode; model: ComponentModels; name: string }
    >({
      query: (body) => ({
        url: '/xb/api/config/pattern',
        method: 'POST',
        body,
      }),
    }),

and assumed that that the current client-side data model for component trees would automatically and transparently work for Pattern config entities too. That's not the case though, because that data model is known to be in need of evolution: #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc..

Problems with the current front-end code cited in #6:

1. The required label for a section is missing. Or are we saying that they should not have labels? 🤔 If so, happy to remove that from the server-side logic 👍
2. Last paragraph of #6: the shape of the data uses the current client-side data model, which is not necessarily a good thing. See #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc..
3. NO error handling! 😅 The server side can return validation errors!

Now, I did anticipate that second point. That's why there's

    // ⚠️ For now, there's no denormalization. This may change in the future.
    $denormalized = $decoded;

… in \Drupal\experience_builder\Controller\ApiConfigControllers::post() and ::patch().

It seems like the most pragmatic approach is to:

1. not block this on #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc.
2. apply the same denormalizations/transformations from the client to the server (some of the work in #3478565: Add Entity Update controller will help) upon POST and PATCH
3. apply the same normalizations/transformations from the server to the client upon GET
4. Follow-up for the UI to support error responses.

Missing context: I expected #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. to have been completed weeks ago, and that'd have likely removed the need for bringing the same client-to-server-transformation-and-back functionality to this MR that we already have for editing content entities' component trees.

That's why I'm a bit hesitant, and why I'd like explicit confirmation from @lauriii.

The required label for a section is missing. Or are we saying that they should not have labels? 🤔 If so, happy to remove that from the server-side logic 👍

The API should enforce a label. We should prevent saving sections without a label.

Last paragraph of #6: the shape of the data uses the current client-side data model, which is not necessarily a good thing. See #3467954: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc..

This is not necessarily urgent. I was hoping we could demo this at DrupalCon in case that this was really close to being done but it seems that it isn't the case. I'd be fine with us postponing this on #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. and re-evaluating closer to 0.2.0 if that hasn't been done.

NO error handling! 😅 The server side can return validation errors!

I wonder if this is covered by the global error handling? When I tested this, I was getting a fairly graceful handling of errors within the modal.

The API should enforce a label.

Okay! Then the client must provide a label. We can't auto-guess a label. But for now I'll go with Section N, with N auto-incrementing. That avoids being blocked on the client side.

I'll do a time-boxed iteration on this to see if I can get the pragmatic solution I outlined in #7 done in an hour.

@f.mazeikis told me a few days ago he was working on it — I see that the issue metadata was out of date 😅

I talked with @f.mazeikis about this issue today

I wanted to see if the logic we added in ClientDataToEntityConverter could be used in this case also. When added in [##3489994] it hardcoded assumptions that we would be updating an entity with a ComponentTreeItem which of course in sections we won't be. But we did have most of the logic for converting a `layout` and `model` from the client to a `tree` and `props` that server can save.

I start with his branch 3486203-integrate-saving-sections, merged in 0.x and then

Moved most of this logic to a new ClientServerConversionTrait::convertClientToServer. These leaves both ClientDataToEntityConverter::convert and ApiConfigControllers::decode very thin as they can both rely on the the new convertClientToServer

I talked with @f.mazeikis about we need to figure out if we can centralize the conversion from client to the server and then back. But this at least puts the logic for client to server in 1 place for converting a `layout` and `model` from the client to a `tree` and `props`. there really wasn't any new code in here just moving code around that already went through review

Thanks for helping out here, @tedbow! What you did is exactly what I expected 😊👍

Perhaps we should land @tedbow's !444 MR first, because it simplifies the rest of the work in this issue and reduces the accumulation of conflicts with other MRs? 😊

For that reason, I am working to get the MR to green so I can approve it — at which point either of you feel free to merge it if you like that!

Using this Create section button on a two column component instance containing one component per column:

… and then reloading the page (needs follow-up: the UI needs to refresh the list after creating a section!), it actually works:

(Although you can see that the preview is not yet working.)

IMHO once this passes tests, this MR can be merged (for DrupalCon Singapore demo purposes), and the preview + refreshing of the list can happen in subsequent MRs in either this issue or a follow-up.

Still needs work for updating XbConfigEntityHttpApiTest::testPattern() to send request bodies shaped like the client-side data model.

going to work on this

Thank, @f.mazeikis!

That made the preview look less broken, but not yet actually working:

Self-assigning because @tedbow hasn't been pushing anything. This feels close, and it overlaps significantly with work I helped land on both ApiComponentsController (the default_markup stuff @f.mazeikis just added) and ApiPreviewController.

I am reviewing those bits specifically for their cacheability correctness, and hopefully to get the preview to fully work in a short amount of time :)

Markup-only preview of Sections/Patterns is now working, but looks off due to missing CSS+JS:

Next: CSS + JS.

Now previews are fully operational:

I wrote

IMHO once this passes tests, this MR can be merged (for DrupalCon Singapore demo purposes), and the preview + refreshing of the list can happen in subsequent MRs in either this issue or a follow-up.

… but that's before I saw the extensive changes to ApiConfigControllers, which explain why XbConfigEntityHttpApiTest::testPattern() is not passing. This MR as-is is too incomplete to be mergeable; it'd leave XB in a very confusing state: parts of the UI start working, but the thorough HTTP API test coverage no longer passes because the refactor towards the client-side data model is good, but partial.

RE #22 I did a hands-on review where I pushed commits to show the direction I'd like to see rather than describing it, which would've been far more ambiguous.

I've already ">roughly implemented the decode-vs-denormalize split, the same thing will be needed for ::encode() (a new ::normalize() method etc, with a @todo to convert to NormalizerInterface in a future MR).

Now the MR has largely passing \XbConfigEntityHttpApiTest::testPattern(), but the post() method is not yet calling ::normalize(), ::patch() is not yet calling neither ::denormalize() nor ::normalize() and ::get() and ::list() still need to be updated to call ::normalize().

I think introducing all the DenormalizerInterface + NormalizerInterface infrastructure in this MR will lead us too far. IOW: I think we should write this using method names matching https://symfony.com/doc/current/serializer.html#the-serialization-proces..., per config entity type. That will make it easy to convert to that Symfony infrastructure in a subsequent MR :)

Chatted with @wim leers and created an issue to extract out just the changes for ClientServerConversionTrait and ClientDataToEntityConverter #3492312: Provide a utility method to convert client layout and model to server tree and props

Good to see this progressing towards a green CI pipeline!

Posted an intermediary review. 🏓 Would love to see this get to an RTBC'able state! 😄

The bit that @f.mazeikis researched I confirmed and extracted into a separate issue+MR that make this MR slightly simpler :) — see #3493388: Remove pointless asset library dependencies/overrides on `core/once`.

This cannot land until that other issue lands, but there's plenty of other remarks that are not yet addressed — by tackling those, this should become RTBC'able! :)

#3493388: Remove pointless asset library dependencies/overrides on `core/once` is in, this can be simplified now 👍

Great to see this approaching the finish line! 😄🏁

I’ve added ednpoints to openapi.yml, but I’ve intentionally stopped myself from adding all the methods and examples, as I am not even sure if that’s reasonable thing to do, if FE doesn’t use it yet. Happy to expand this, if that will be the in the review feedback.
The other thing is !505 was merged in and resulted in conflict that forced me to make some changes to ApiConfigControllers and XbConfigEntityHttpApiTest , so changes in MR are slightly beyond just addressing the previous review feedback.

I'd like explicit feedback from the /openapi.yml owner, @traviscarden, because I am concerned that it in the current state it will cause confusion, whereas it should be providing clarity and increasing team velocity. Travis, see write-up on the MR.

While awaiting @traviscarden's OpenAPI feedback; there definitely are outstanding things @f.mazeikis can already fix until Travis wakes up :)

@wim leers, there were actually some pretty significant problems with our openapi.yml--most notably, schema-related specification violations. (It doesn't surprise me it hasn't been doing everything we want it to.) I've fixed them all, and Swagger Editor is happy now. I've also identified a few opportunities for improvement, but it took kind of a long time to get the existing file passing validation, so I haven't had time to actually implement any of them. Here's an overview:

1. Figure out why our tests didn't catch the specification validation errors on openapi.yml. I believe that's what OpenApiSpecValidationTest is supposed to be doing.
2. Specify all responses, including errors. We haven't been rigorous about specifying error conditions. I believe there are some ways to specify defaults or templates that could reduce boilerplate.
3. Specify parameter/property minimum, maximum, default, minItems, and maxItems where appropriate. See Data Types for more possibilities.
4. Add allowEmptyValue where possible.
5. See if there are any other primitive schema types that can be changed from free values to enum because there are a definite, limited number of valid values.
6. Specify (serialization) style on array parameters. See Describing Parameters > Schema vs Content. I haven't looked closely at the places this would apply yet to see how much value it would actually provide.
7. Add more (and more complete/robust) examples.
8. Add missing descriptions. I added stubs for them with TODO values for now.

That's what I've got for now. I don't know what you want to accomplish within this issue specifically, @wim leers. Anything that doesn't seem urgent could be moved to #3472617: Document where the cut-off lies of where OpenAPI validation ends and entity validation begins, if you like.

@traviscarden in #37: Thanks! 😊

Why did \Drupal\Tests\experience_builder\Unit\OpenApiSpecValidationTest not catch those significant problems? You wrote that in your first point! 😄

Agreed on all counts. Bumped #3472617: Document where the cut-off lies of where OpenAPI validation ends and entity validation begins to Critical, since it would've accelerated this MR by days!

I don't know what you want to accomplish within this issue specifically, @wim leers.

Well, as you would probably expect: I want this issue to deal only with Pattern config entities, as the title describes. So actually, the commits you pushed are out-of-scope and should've landed in a separate MR, because this now complicates reviewing this MR.

Fortunately, @f.mazeikis doesn't mind. So let's be pragmatic and keep the out-of-scope changes.

#3491021: Leverage HTML comment annotations, remove wrapping div elements landed yesterday, which this conflicted with. Plus, upstream broke our CI, so this needed #3499602: CI: DEVizzent/cebe-php-openapi 1.1.3 broke the build merged in, too.

I vow this MR will land today!

Turns out that

There was 1 error:
1) Drupal\Tests\experience_builder\Kernel\ApiPendingChangesControllerTest::testApiPendingChangesController
PHPUnit\Framework\Exception: Uncaught PHP Exception League\OpenAPIValidation\PSR7\Exception\Validation\InvalidBody: "Body does not match schema for content-type "application/json" for Response [get /xb/api/autosave 200]" at /builds/project/experience_builder/vendor/league/openapi-psr7-validator/src/PSR7/Exception/Validation/AddressValidationFailed.php line 31

was caused by a KeywordMismatch::fromKeyword exception for langcode on the PageTemplate config entity's auto-save entry returned by ApiPendingChangesController being NULL, whereas the OpenAPI spec requires it to be a string.

@traviscarden did not change that bit of the definition of AutoSaveEntry in /openapi.yml, so it must've been a pre-existing bug, that was being masked by subtle problems in /openapi.yml; otherwise #3494114: Implement auto-save of the page template config entity would've run into this and would've been forced to fix it instead of this MR.

I thought I was wrong in #39, because after making langcode nullable: true, the error still occurred, identically! 🤪

The error message is obviously … not very detailed, and hence quite hard to interpret. So … I dug deeper. 👷

Root cause for that turns out to be \Drupal\experience_builder\EventSubscriber\ApiMessageValidatorBase::onMessage(), which does:

    catch (ValidationFailed $e) {
      $this->logFailure($e);
      // @todo Surface exception details better for front-end display.
      // @see https://www.drupal.org/project/experience_builder/issues/3470321
      throw $e;
    }

… and while the ::logFailure() logs the detailed information, the throw $e means that \Drupal\experience_builder\EventSubscriber\ApiExceptionSubscriber has to re-implement this.

That changes the error message to be much more helpful:

Body does not match schema for content-type "application/json" for Response [get /xb/api/autosave 200]. [Keyword validation failed: Value cannot be null in node:2:en->owner->avatar]

Turns out #3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX is the pre-existing issue for that … and it's already been critical for almost 4 months, but it's just never been worked on! 😬

So, while @traviscarden's commits on this MR were well-intentioned, they were very out-of-scope. I tried keeping them, because it's still a much-needed improvement to /openapi.yml. But unfortunately, they revealed hitherto unknown flaws in /openapi.yml, as explained in #39 and #40.

I decided to still keep those because of how many are being negatively impacted productivity-wise by the absence of OpenAPI validation errors — see #3472617-3: Document where the cut-off lies of where OpenAPI validation ends and entity validation begins and -4.

I think both #3472617 and #3470321: Surface verbose OpenAPI errors in the UI — for better bug reports and faster DX are urgent to keep this team moving forward, and I've asked @effulgentsia to prioritize both issues. But meanwhile, at the cost of more chaos in this issue and a significant amount of time spent by me decyphering the situation, I merged 99% of what @traviscarden did here, because those are all net improvements! 🥳

The sole bit I reverted is the reordering, and for that I created the more cohesive #3499703: Make all XB HTTP API routes consistently prefixed, ensure they all have OpenAPI specs, and tests to keep it so, which @tedbow independently raised earlier today.

The test coverage in \XbConfigEntityHttpApiTest::testPattern() needed to carefully balance testing OpenAPI validation (data shape only) against validation constraint violation (complete, detailed validation, including internal consistency beyond the data shape). I refined that.

GIF of this in action added to the issue summary!