Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Account created on 26 December 2006, over 18 years ago
  • Senior Principal Software Engineer β€” Drupal Acceleration Team at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

For 1.0, we're targeting a subset of the entire scope: 🌱 [META] Content templates Active . At a high level (see #3541000-25: [META] Content templates UI for 1.0: only nodes, no exposed slots, no replacement for the view mode/display UI β†’ ), we're leaving the following for later:

  • support for other content entity types besides Node (among others, blocked on πŸ“Œ Allow XB to be used on all node types Active )
  • support for exposed slots (i.e. there's no per-content entity component trees for content entity type bundles that have a ContentTemplate)
  • XB-native alternatives for core UIs such as view modes
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Per @hooroomoo 😊

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

We've got kind of a meta sprawl going on here πŸ˜…πŸ™ˆ i.e.: what is the difference in intent/scope between:

So together with @f.mazeikis and @thoward216, clarifying the scope of this meta! πŸŽ‰

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I doubt that we'd be okay with only supporting populating SDC-sourced component instances with structured data β€” we of course also want to be able to do that for code components. Which means that we have to do #3503038. See #3503038-16: Enable candidate `DynamicPropSource` suggestions for code components: refactor `GeneratedFieldExplicitInputUxComponentSourceBase` and `FieldForComponentSuggester` to need only SDC's ComponentMetadata, not SDC plugin instances β†’ for details.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This is going to block 🌱 [META] Content templates Active , and πŸ“Œ Move `PropSourceEndpointTest` into new `XbConfigEntityHttpApiTest::testComponent()` Active made it very apparent πŸ˜…

Quoting \Drupal\Tests\experience_builder\Functional\ApiUiContentTemplateControllersTest::testSuggestionsClientErrors():

   *           ["node/article/js.xb_test_code_components_with_link_prop", 400, "Code components are not supported yet."]
   *           ["node/article/js.xb_test_code_components_with_no_props", 400, "Code components are not supported yet."]

So, starting point:

diff --git a/src/Controller/ApiUiContentTemplateControllers.php b/src/Controller/ApiUiContentTemplateControllers.php
index 85bcf22f9..3f3909582 100644
--- a/src/Controller/ApiUiContentTemplateControllers.php
+++ b/src/Controller/ApiUiContentTemplateControllers.php
@@ -112,11 +112,6 @@ final class ApiUiContentTemplateControllers extends ApiControllerBase {
       throw new BadRequestHttpException('Only components that define their inputs using JSON Schema and use fields to populate their inputs are currently supported.');
     }
 
-    // @todo Add support for suggestions for code components in https://www.drupal.org/i/3503038
-    if (!$source instanceof SingleDirectoryComponent) {
-      throw new BadRequestHttpException('Code components are not supported yet.');
-    }
-
     if ($this->entityTypeManager->getDefinition($content_entity_type_id, FALSE) === NULL) {
       throw new NotFoundHttpException(sprintf("The `%s` content entity type does not exist.", $content_entity_type_id));
     }
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oh I see that ✨ Move entity type and ID from base path and into routing parameters Active is already planned to be worked on this sprint!

@hooroomoo in chat confirmed:

Yes I agree it makes sense to just close that issue and have #3502887 address the frontend part. I think Jesse is gonna pick it up when he's back from PTO

πŸ₯³

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ Move `PropSourceEndpointTest` into new `XbConfigEntityHttpApiTest::testComponent()` Active is in πŸ₯³ The test coverage for that revealed one bug in shape matching, which isn't a hard blocker, but needs to be fixed at some point. So created a new section.

✨ Move entity type and ID from base path and into routing parameters Active is unblocked because ✨ Allow working with a new (unsaved) entity Active landed! (Which means that one was missing, so added it πŸ˜‡)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

✨ Allow working with a new (unsaved) entity Active just landed. Note that the FE pieces of #3529836 still need to happen, and it might make sense to just tackle all of that here See #3529836-27: Enable starting with an empty XB UI (so without first having to create an entity with a component tree) β†’ .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That's in! Thanks, @thoward216! 😊

This unblocked ✨ Move entity type and ID from base path and into routing parameters Active .

The work that still remains is updating the UI (it currently just "loads" XB forever as demonstrated in #4), so changing the issue queue component.

However … we might also just want to close this and let ✨ Move entity type and ID from base path and into routing parameters Active take care of that. πŸ€” Doubly so because that issue literally states

I defer to @jessebaker :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Please follow the patterns set in πŸ“Œ Move `PropSourceEndpointTest` into new `XbConfigEntityHttpApiTest::testComponent()` Active :

  • add new method to ApiUiContentTemplateControllers
  • add success test coverage + comprehensive client error test coverage ApiUiContentTemplateControllersTest
  • expand OpenAPI spec
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The very last obstacle is supremely silly:

FILE: tests/src/Functional/ApiUiContentTemplateControllersTest.php
------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 20 | ERROR | [x] Doc comment star missing (Drupal.Commenting.DocCommentStar.StarMissing)

… but that all is there and it DOES comply! πŸ™ƒ

Just removing the comment, don't want this to be held up by that β€” created supported request upstream: πŸ’¬ False `Drupal.Commenting.DocCommentStar.StarMissing` report? Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

wim leers β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I think #5 is actually wrong β€” this is not blocked on adapters. Because at least a common subset is in principle possible to realize already.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I'd have liked to see a more comprehensive end-to-end test, but I see now that the issue summary was quite specific about this: so πŸ‘

It is very nice to see such a nice little expansion of an existing test though, so merging this; more expansive test coverage will be necessary in πŸ“Œ [later phase] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed anyway :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Starting point for making this more strict and hence actually useful for development:

diff --git a/openapi.yml b/openapi.yml
index a9b71ddfb..71b3a0321 100644
--- a/openapi.yml
+++ b/openapi.yml
@@ -2027,6 +2027,7 @@ components:
       required:
         - detail
         - source
+      additionalProperties: false
       properties:
         detail:
           type: string
@@ -2205,14 +2206,14 @@ components:
             type: object
             required:
               - errors
+            additionalProperties: false
             properties:
               errors:
                 type: array
                 description: Error items
+                minItems: 1
                 items:
-                  type: object
-                  schema:
-                    $ref: '#/components/schemas/Error'
+                  $ref: '#/components/schemas/Error'
     AuthenticationErrorResponse:
       description: Access denied response.
       content:
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks to @hooroomoo's test of this together with their PoC, I can now land this with confidence πŸ₯³

Out of scope:

  1. making these API responses cacheable β€” if the need ever arises, we'll do that then
  2. code component support is out of scope β€” we have had πŸ“Œ [PP-1] Refactor GeneratedFieldExplicitInputUxComponentSourceBase and FieldForComponentSuggester to need only SDC's ComponentMetadata, not SDC plugin instances Postponed for >6 months now to do exactly that
  3. fixing a shape matching bug that the test coverage I added unearthed: πŸ› Shape matching of an optional SDC image prop fails to find optional field instances Active .
  4. while updating the OpenAPI spec, I noticed that for all 4xx responses, the generated docs say
    {
      "errors": [
        {}
      ]
    }
    

    is an example value. Which makes no sense, because it should be something like

    {
      "errors": [
        "The 'bla bla' permission is required."
      ]
    }

    But … this is using the ClientErrorResponse already in HEAD, which clearly is wrong for many (not all, but many) routes πŸ˜… It's somewhat right for validation errors for data sent in various API request bodies, but it's wrong for (most?) other cases.

    Created πŸ› OpenAPI spec's `ClientErrorResponse` etc are incorrect for most API routes Active for this.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Paired with @thoward216. Turns out that Page's

"add-form" => "/xb/xb_page",

is a long-lost ambition: it doesn't work at all: the JS UI crashes right away (just like in #4).

It's also obsolete now, thanks to the experience_builder.api.content.create route being called first.

We may choose to restore this in the future, but for now, being able to start using XB without any entity (/xb working rather than only /xb/node/5 or /xb/page/4) is more important. Which is why I rescoped it as such in #2 (original title was: ).

For now, just removing that altogether, including the test coverage at \Drupal\Tests\experience_builder\Kernel\Entity\Routing\XbHtmlRouteProviderTest::testAddFormRoute(), seems more sensible β€” because it's testing that HTML is served that results in a crashing JS application. πŸ˜…

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That was fast!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Especially important now that we've discovered (and fixed) πŸ› Data loss: `ComponentTreeItem::setValue()` and `::onChange()` may inappropriately upgrade `component_version` to the latest version Active (and kinda also πŸ“Œ Disallow component trees with `component_version: active` Active ). This would be the finishing touch to boost our confidence πŸ€“

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I think this actually all was finished back in #75 πŸ€“

Yay, thanks everyone! πŸ₯³

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I won’t have time to finish this MR today β€” there’s still a bunch of loose ends to clean up, but it works.

@hooroomoo, please test, and confirm that this is what you want/need! πŸ™ πŸ˜‡

You'll get something like:

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The issue summary already talks about nodes only, but I just wanted to reinforce that at this stage we're only targeting node bundles, no other entity types are in scope.

FYI this is enforced since https://git.drupalcode.org/project/experience_builder/-/commit/843e7b0b1...

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks, all, and especially @larowlan & @bnjmnm!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Walked @bnjmnm through my findings and changes, and he then double-checked the comments I added. He's +1 so … yay, let's get this in! πŸ₯³

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Perfect, I have everything I need now to finish this up :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks for following the pattern set by πŸ› External AI Chatbot Functionality Active  β€” and adding the missing test coverage! 🀩 πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@lauriii, see #8.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This then will likely beat the block ComponentSource to the finish line, because this would become the first implicit input. (Issue for block: πŸ“Œ Handle block contexts Active .)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

It's clear that when rendering the component, for the purpose of this issue we

+1 to both

We might try doing a single component source, where the inputs are not mandatory. But it might need to become two different component sources.

Let's find out :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Provide an access control handler for personalization segments using our single permission β€œmanage personalization”.

This was approved by @lauriii?

If so: πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

How does this relate to πŸ“Œ Provide a locked Default segment Active ? Will that have a special reserved weight that no other Segment can set?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

So:

  1. added docs to clarify the (pre-existing!) messy situation with $client_model = ['resolved' => …] to hopefully save the next person time, and retitled (already existing issue) πŸ“Œ Evolve the organically grown `::getClientSideInfo()` into something cohesive Active to convey this is a system (DX) problem in the XB codebase that will continue to slow us down
  2. was able to actually remove ::fixBooleansUsingConfigSchema() because it was clearly dead code β€” which makes my #31 obsolete
  3. to do my due diligence in having done that, re-tested πŸ› Auto-saving of blocks needs to handle string-to-bool fixing Active and I can indeed no longer reproduce it β€” I stepped through the entire thing with both an admin menu block and the site branding block β€” works perfectly πŸ₯³
  4. the fact that BlockComponent::(validate|submit)ConfigurationForm are still untouched/unused even after making all this work (they were introduced as no-ops in πŸ“Œ Define `props` in the context of Block components Active ) just proves that we won't need these, because the component instance form will NEVER be a traditional server-side form, so: removed
  5. follow-up: πŸ› Integer BlockComponent inputs (i.e. block settings) are incorrectly saved as strings Active β€” pre-existing problem in HEAD, this doesn't make it worse
  6. follow-up: πŸ“Œ Rename `ComponentInputsForm` to `ComponentInstanceForm`, to match the route name Active β€” pre-existing problem in HEAD, this doesn't make it worse

Given this is so clearly such an important leap forward, even if there may be imperfections, I am trying to meet with @bnjmnm in the next 30 mins, otherwise I'm going ahead and merging πŸ˜‡

Just see the lovely results:

@bnjmnm, feel free to reopen this issue if you have lingering concerns, we can do follow-up MRs πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Manually tested this again on #3523496, and I can confirm that this is fixed there! πŸ₯³

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Apparently #32 already is the case in HEAD. Created new issue for it: πŸ› Integer BlockComponent inputs (i.e. block settings) are incorrectly saved as strings Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Placing the admin menu block and configuring it to start at level 1 and show 3 levels works and renders and is valid and saves, but … I see this getting stored for my content-defined component tree:

{"level":"1","depth":"3","expand_all_items":true,"label":"Administration","label_display":"0"}

πŸ› Strings, not integers!

And if I create a Pattern out of it (to create a config-defined component tree):

uuid: d5b012be-b463-46b9-9558-eb88ac87ce7b
langcode: en
status: true
dependencies:
  config:
    - experience_builder.component.block.system_menu_block.admin
id: sadfsadf
label: sadfsadf
component_tree:
  -
    uuid: 0ad04f78-0cb9-4634-9419-6091d1fb4376
    component_id: block.system_menu_block.admin
    component_version: 02b80c9f6cbacff5
    inputs:
      label: Administration
      label_display: '0'
      level: '1'
      depth: '3'
      expand_all_items: true

πŸ› Again strings, not integers!

🀯 I'd expected config schema validation to catch this, but while BlockComponent::validateComponentInput() does call config schema validation, it considers these valid! (Because it does the necessary casting based on the schema.)

Yet if I place the exact same block plugin as a block and use the exact same form to set the same settings, I get the following Block config entity:

uuid: eb9d7af5-d296-4481-a1f7-ddfc106f699d
langcode: en
status: true
dependencies:
  config:
    - system.menu.admin
  module:
    - system
  theme:
    - olivero
id: olivero_administration
theme: olivero
region: header
weight: 0
provider: null
plugin: 'system_menu_block:admin'
settings:
  id: 'system_menu_block:admin'
  label: Administration
  label_display: visible
  provider: system
  level: 1
  depth: 3
  expand_all_items: true
visibility: {  }

πŸ‘† No strings, but integers.

Will investigate…

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Given #3520484-76: [META] Production-ready ComponentSource plugins β†’ , this actually won't be blocking a stable (1.0) release.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Ahhhh! Then I get it! πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks β€” looks great now! πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Related: ✨ Opt into static_cache for component config entities Active β€” this issue is about making querying them faster.. #3540075 is for making (repeated) loading of a specific Component (with known ID) faster.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Related: πŸ“Œ Implement `lookup_keys` in Component for optimized queries Active β€” this issue is for making (repeated) loading of a specific Component (with known ID) faster. #3528847 is about making querying them faster.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Virtually all validators must allow NULL by default to avoid crashing hard on optional values.

I think you're right that we're missing a NotNull constraint. That's also what type: _core_config_info etc. do.

IIRC, alternatively, we could mark type: config_entity fully validatable β€” I vaguely remember that causing the NotNull constraint to be added automatically for required key-value pairs, and any type: mapping automatically gets all key-value pairs without requiredKey: false set to be required.
Yep, confirmed, see:

      if ($root_type_has_opted_in) {
        $data_definition->setRequired(!isset($data_definition['nullable']) || $data_definition['nullable'] === FALSE);
      }

β€” \Drupal\Core\Config\TypedConfigManager::buildDataDefinition()

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Yay, πŸ“Œ Define how block settings should be presented in the UI Active is now indeed closed, but I was wrong about πŸ“Œ [PP-1] Implement client-side validation of block settings Active (see #16 and onwards there): while this issue's MR adds validation, it doesn't add client-side validation, and that'd still be a boon for UX. See the discussion there for concrete proposals for how to achieve that.

The adoption of #tree broke BlockComponent::fixBooleansUsingConfigSchema() β€” see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Related: πŸ“Œ [PP-1] Implement client-side validation of block settings Active , because that's what fixBooleansUsingConfigSchema() points to as the issue where that method will be removed.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thinking about this more, I wonder if XB should do something like entity.memory_cache for its entities; and specifically for the PageRegion (this issue) and Component config entities (not this issue, but by far the one XB needs most of).

The entity.memory_cache.slots: 1000 container parameter that was added in Drupal 11.2 ( CR β†’ ) seems an interesting way to keep memory consumption under control (thus negating the concern I surfaced in #10), while still reducing I/O, and would make it tunable.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

ℹ️ @lauriii decided to descope a stable ComponentSource plugin API to after 1.0. It'll still be around for contrib to experiment, but we won't have the time to actually make it a stable PHP API with a BC promise before DrupalCon Vienna.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

"wisely" and "AI" in the same sentence πŸ€”πŸ€£

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

APCu would likely not be big enough to hold all Component config entities.

Can you please add static caching to only PageRegion::loadForActiveTheme() and observe the impact of that?

(I'm curious what would happen if we'd decorate core's \Drupal\Core\Config\Entity\Query\Query::loadRecords() and do in-memory caching there, but I suspect it'd make memory consumption go up massively.)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I don't think we need an e2e test for this; we've got a functional test in tests/src/Kernel/Controller/ExperienceBuilderControllerTest.php, which combined with any development we all do would quickly reveal if this ever gets broken πŸ€“

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Very close!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Ah, I think I understand now β€” the concern here is about race conditions caused by in-progress AJAX requests/responses/commands.

You're actively working on the related πŸ“Œ Replace the postPreview action with atomic equivalents Active , but AFAICT that wouldn't solve this problem. Do I understand that correctly?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Neha, could you check if this is still relevant? See #7 for why. Thanks! πŸ™

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Great β€” this now just needs updates to 3 tests, which apparently rely on the fact that there has not been static caching so far.

@mglaman: that screenshot in the issue summary, which route is that for, and for how big of a component tree is it?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

(Or perhaps #config_schema, rather than #config_target, because #config_target is designed to automatically update the target config too.)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I explained in #18 that I don't see how this can reliably work.

AFAICT the only way to do do this reliably (PHP AST analysis does not seem realistic) based on config schema constraints, we'd need additional metadata specified by the block plugin author β€” basically what @larowlan described at #3484669-5: Define how block settings should be presented in the UI β†’ , quoted here to make the conversation simpler:

Is there a third option somewhere in the middle here where some components are flagged (e.g. via hook_block_alter for blocks) as being 'simple' in so far as keys in ::blockForm map 1:1 to their settings. Even if they're not fully validatable.
For these we can call ::blockForm and make use of the existing FormState react functionality to keep the store in sync.
For items that we review and determine that there's more going on in ::blockSubmit ::blockValidate we make a round-trip back to the server to resolve props from form values.
I think there's going to be enough use cases for something like this that we can make it generic on the Component config entity, i.e. the UI should be simple and not need to know the difference between blocks and SDC and anything else added later, it should just know that some components need a round trip to Drupal to transform form values into settings/props - whilst others don't. There are cases where we will need this for field widgets (e.g. ::massageFormValues) and places we will need it for SDC (think Media widget where we resolve an image URL via a hard-coded attributes approach atm).
Thoughts?

(Emphasis mine.)

I think that's fraught with risks and problems. I think having block plugin authors specify HTML5 client-side validation is the pragmatic choice, perhaps expanded with something like an explicit data-drupal-config-schema-type="block.settings.search_form_block:page_id", to enable the block plugin developer to convey through metadata that this form element's value MUST comply with the validation constraints for type: block.settings.search_form_block's page_id key-value pair.

πŸ’‘ This reminded me of something …
We introduced #config_target for simple config forms to gain validation support with minimal effort 1.5 year ago, see https://www.drupal.org/node/3373502 β†’ . So … AFAICT the best way to achieve "that last 10% (nice UX is of course important!) do is make Experience Builder leap ahead of core, and allow block plugin forms to use #config_target too:

diff --git a/core/modules/search/src/Plugin/Block/SearchBlock.php b/core/modules/search/src/Plugin/Block/SearchBlock.php
index f0e829c11c5..12e1792a925 100644
--- a/core/modules/search/src/Plugin/Block/SearchBlock.php
+++ b/core/modules/search/src/Plugin/Block/SearchBlock.php
@@ -112,6 +112,7 @@ public function blockForm($form, FormStateInterface $form_state) {
       '#options' => $options,
       '#empty_option' => $this->t('Default'),
       '#empty_value' => '',
+      '#config_target' => 'block.settings.search_form_block:page_id',
     ];
 
     return $form;

πŸ‘† That would be the signal to transform config schema (as closely as possible, because many validation constraints can run only server side) to JSON Schema.

The relevant config schema is:

    page_id:
      type: string
      label: 'Search page'
      # Optional: falls back to the default search page.
      # @see \Drupal\search\Form\SearchBlockForm::buildForm()
      # @see \Drupal\search\SearchPageRepositoryInterface::getDefaultSearchPage()
      nullable: true
      constraints:
        ConfigExists:
          prefix: search.page.

Here, we would then know it must be a string, but it is allowed to be NULL, and … the sole explicitly specified constraint (ConfigExists) we cannot translate to JSON schema for client-side validation.

Thoughts?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Updating issue metadata based on feedback by @larowlan and @bnjmnm, plus I don't see how this is postponed on #3513589 anymore β€” it might've been at the time of #6, because a lot more was unclear at the time (it reminds me of #3462241-8: [PP-1] Decorate the SDC plugin manager and allow components defined in code β†’ , where we were still considering using the same auto-generated forms-from-SDC-JSON-schema for block plugins).

Now this is just postponed on πŸ“Œ ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I agree that that last 10% would be very nice.

But how can you achieve that reliably? Block plugins’ forms may very well use a very different structure than the associated config schema. It might:

  • Use completely different keys
  • Use form elements that do not map in an obvious way to the config schema type
  • Et cetera

For example: a dropdown in the block plugin’s form may result in multiple key-value pairs in the block plugin’s settings getting updated thanks to the validation and/or submit handlers.

In fact, that is precisely one of the reasons to use the block plugins’ forms: to avoid the need to expose 1:1 to the content author concepts that make sense to the block plugin developer: the form should map the technical mental model to a human content author mental model.

I know many/most block plugins don’t do this, but some do, and it should be encouraged. In fact, XB likely will incentivize many contrib developers to do exactly that for their block plugins!

That’s why we chose to NOT auto-generate forms for block plugins from their (fully validatable) config schema.

So, I don’t see how we’d do this; it’d require somehow analyzing the block plugin form code. It would be achievable to nudge block plugins forms to add HTML5 validation.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

We long ago decided to use the forms provided by block plugins, not generate ones, and so IMHO we should close this. But let's make sure we leave no loose ends:

All good IMHO πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active is progressing and basically does

So … @larowlan, what do you think about exploring what it'd take to refactor \Drupal\experience_builder\Form\ComponentInputsForm to use the same logic as the tab when submitting a form for a block component instance?

So: IMHO we should close this issue β€” client-side validation for block component instances just doesn't make sense.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks! That’s what I thought/hoped β€” but apparently failed to link πŸ˜…

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oh, right. Great, then #6 is a solid plan! πŸ‘ Thanks :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This MR looks equals parts impressive and scary β€” in #23 I articulated my main concern, but then was forced to withdraw it :D (aka concluding I was wrong!)

I did spot a number of other concerning things though β€” concerning because I'm struggling to grok them πŸ˜…

The most important ones:

  1. the use of resolved but that then not matching the exact inputs expected by block-sourced components: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
  2. +1 for your suggestion to lift some things into a new service, which then both BlockComponent and ClientDataToEntityConverter can use β€” suggestion at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... β€” without this, I worry about maintainability, so IMHO it should happen in this MR.
  3. concerns about ::fixBooleansUsingConfigSchema() still being around despite your remark at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... β€” see
  4. I don't understand something at all that's at the heart of this MR: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1..., and while I could step through it to grok it completely, I worry about the complexity
  5. It seems that simply merging in upstream has caused Playwright tests to fail β€” because πŸ“Œ Port Cypress tests to Playwright Active added a Playwright test after this MR was last updated:
      3) [webkit] β€Ί tests/src/Playwright/blockForm.spec.ts:47:7 β€Ί Block form β€Ί Block settings form values are stored and the preview is updated 
        Error: Timed out 10000ms waiting for expect(locator).toBeChecked()
        Locator: locator('[data-testid="xb-contextual-panel"] [data-drupal-selector="component-inputs-form"] input[data-drupal-selector="edit-xb-component-props-78c73c1d-4988-4f9b-ad17-f7e337d40c29-use-site-logo"]')
        Expected: checked
        Received: <element(s) not found>
        Call log:
          - Expect "toBeChecked" with timeout 10000ms
          - waiting for locator('[data-testid="xb-contextual-panel"] [data-drupal-selector="component-inputs-form"] input[data-drupal-selector="edit-xb-component-props-78c73c1d-4988-4f9b-ad17-f7e337d40c29-use-site-logo"]')
          61 |       `[data-testid="xb-contextual-panel"] [data-drupal-selector="component-inputs-form"] input[data-drupal-selector="edit-xb-component-props-${componentUuid}-use-site-logo"]`,
          62 |     );
        > 63 |     await expect(siteLogoCheckbox).toBeChecked();
    

    β€” https://git.drupalcode.org/project/experience_builder/-/jobs/6186837

    (It fails consistently.)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Whoops, I read over this bit in @GΓ‘bor Hojtsy's comment:

Unfortunately when I try to enter something in the webform autocomplete that is not quite correct (eg. I start typing to wait for autocomplete to return), I get a red error in place of the form saying it did not render. I tried looking in the console logs but no useful data there. I tried reloading but it got into an "undo last action" loop.

@bnjmnm is right in #24: that is expected: invalid component inputs SHOULD result in a failure to render, but not in a way that it's a hard crash β€” see the screenshots in πŸ“Œ Improve server-side error handling Active . That must be what GΓ‘bor is seeing.

@GΓ‘bor: why do you expect that an invalid/nonsensical value would result in automatically all user input getting ignored and instead the block falling back to the default?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@bnjmnm's issue summary update in #11 is superb β€” thanks, Ben! 🀩

πŸ“Œ Redux-aware form input components should be aware of direct element manipulation Active landed yesterday. @GΓ‘bor Hojtsy, could you please re-test #19 β€” I've merged upstream changes into this MR πŸ‘

@larowlan: AFAICT this would make πŸ› Auto-saving of blocks needs to handle string-to-bool fixing Active obsolete β€” do you concur?

#15:

  • In \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::buildConfigurationForm we're only calling ::blockForm and we probably should be calling ::buildConfigurationForm - current code is not calling the code for blocks that override that method - e.g. \Drupal\layout_builder\Plugin\Block\InlineBlock::buildConfigurationForm

I disagree.

πŸ“Œ Define `props` in the context of Block components Active added the call to ::blockForm(). It did that because it method actually specified on the block plugin: \Drupal\Core\Block\BlockPluginInterface::blockForm().

::buildConfigurationForm() is not guaranteed to be implemented, and is specifically tailored towards Block config entities, because:

  1. \Drupal\Core\Block\BlockPluginTrait::buildConfigurationForm() is what implements it β€” an optional (but encouraged) trait, also used by \Drupal\Core\Block\BlockBase
  2. it adds a whole lot of noise to the component instance form for block-sourced components, 100% of which is irrelevant when used in XB; it's only relevant for Block config entities:
      public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
        $definition = $this->getPluginDefinition();
        $form['provider'] = [
          '#type' => 'value',
          '#value' => $definition['provider'],
        ];
    
        $form['admin_label'] = [
          '#type' => 'item',
          '#title' => $this->t('Block description'),
          '#plain_text' => $definition['admin_label'],
        ];
        $form['label'] = [
          '#type' => 'textfield',
          '#title' => $this->t('Title'),
          '#maxlength' => 255,
          '#default_value' => $this->label(),
          '#required' => TRUE,
        ];
        $form['label_display'] = [
          '#type' => 'checkbox',
          '#title' => $this->t('Display title'),
          '#default_value' => ($this->configuration['label_display'] === BlockPluginInterface::BLOCK_LABEL_VISIBLE),
          '#return_value' => BlockPluginInterface::BLOCK_LABEL_VISIBLE,
        ];
    
        // Add plugin-specific settings for this block type.
        $form += $this->blockForm($form, $form_state);
        return $form;
      }
    

    πŸ‘† Of those:

    1. provider is pure noise β€” nope I mixed that up with πŸ“Œ [PP-1] Deprecate unused `provider` exported property from Block config entities Postponed
    2. admin_label is used only at /admin/structure/block, to allow site builders to relabel placed block [config entities] to make sense to them, in that particular UI
    3. label (and its sibling label_display) is used by block.html.twig, which XB doesn't use, because that's associated with Block config entities

    😬🫠 … except that apparently type: block_settings dictates that label, label_display etc. must all be present always 🫠

    … which means this is the right thing to do, and this should pave the path for fixing πŸ› block__content div is omitted from preview when "Use Experience Builder for page templates in this theme." is enabled Active , too.

    Plus, you've sidestepped the UX nightmare I identified by working your magic in BlockComponent::buildAnonymousFormForBlockPlugin() to remove those 4 form fields πŸ§™πŸ½β€β™‚οΈ

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Doesn't this become obsolete thanks to πŸ“Œ ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active ? πŸ€”

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#6++ β€” which is why this is tagged for 🌱 [META] 7. Content type templates β€” aka "default layouts" β€” clarify the tree+props data model Active . πŸ‘

(It's not a stable blocker in and of itself is what Lauri meant β€” but it is a blocker for #3455629.)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

+1 to removing this exception.

But let's convert rather than delete existing test coverage to expect the fallback exception? i.e. this one:

throw new \LogicException("This entity does not have an XB field!");

I might be missing something obvious, but I don't understand why we wouldn't keep that.

Ah … I guess that is what #4 is implicitly referring to β€” I think removing that catch (and the other one in the same file) is the right thing to do.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Note: this PoC is a continuation from before #3525746-6: Update the React client preview view β†’ was the agreed upon consensus for how to build this β€” consensus between @effulgentsia, @penyaskito and @jessebaker.

(Linking to improve discoverability even though this is closed, because @jessebaker and @penyaskito are using this closed issue/MR to get this functionality going again.)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

need the ability to render any entity type through an existing content template

As of https://git.drupalcode.org/project/experience_builder/-/commit/843e7b0b1..., ContentTemplates are only allowed for node content entities, as proposed β†’ by @effulgentsia as a realistically achievable goal for 1.0. @lauriii and I both +1'd this proposal, and made it so.

That did not implement all of ✨ [PP-1] Validate that content templates are attached to entity bundles that fulfill XB's requirements Postponed , but allowed us to make it no longer stable-blocking.

(Fun fact: you created #3518272, @phenaproxima! :D)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I'm fine with adding this, but am worried we'll break it in the future. Is there a way we can test this?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Great, that means @lauriii confirms #8.2 πŸ‘

Thanks!

Production build 0.71.5 2024