Block component instance form values not processed by validation/submit handlers

This AFAIK ties into follow-ups opened for #3475584: Add support for Blocks as Components that were never followed up on. A quick scan suggests:

• #3484669: Define how block settings should be presented in the UI
• #3500795: [PP-1] Implement client-side validation of block settings (see \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::fixBooleansUsingConfigSchema())
• etc.

This should be fixed before 1.0, but not necessarily before #3515932: Milestone 1.0.0-beta1: Enable creation of non-throwaway sites — because A) ComponentSource plugins won't be a stable API yet, B) I don't see how this could impact the data storage.

If we're lucky, it's this simple:

diff --git a/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php b/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php
index f0a7b094c..8e7fa4ba4 100644
--- a/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php
+++ b/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php
@@ -325,14 +325,14 @@ final class BlockComponent extends ComponentSourceBase implements ContainerFacto
    * {@inheritdoc}
    */
   public function validateConfigurationForm(array &$form, FormStateInterface $form_state): void {
-    // @todo Implementation.
+    $this->getBlockPlugin()->validateConfigurationForm($form, $form_state);
   }
 
   /**
    * {@inheritdoc}
    */
   public function submitConfigurationForm(array &$form, FormStateInterface $form_state): void {
-    // @todo Implementation.
+    $this->getBlockPlugin()->submitConfigurationForm($form, $form_state);
   }
 
   /**

I'm running into this in #3535986: Autocomplete machine name handling does not match to machine name, webform picking does not work with the webform block.

Webform uses Drupal\Core\Entity\Element\EntityAutocomplete which has a validateEntityAutocomplete() which is where the mapping happens. That is defined as an #element_validate on the element. I tried the proposed fix in #4, that did not seem to help (change anything at all with what it did).

The validateConfigurationForm / submitConfigurationForm methods in ExperienceBuilder/ComponentSource/BlockComponent aren't called at all, so the additions in #4 unfortunately won't work. This isn't a huge shock as (like the Page Data form) there isn't a traditional submit that would invoke the validate/submit methods in the way we're accustomed to.

I attempted to submit the block form similar to how the entity form is programmatically validated/submitted in \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields and didn't have any luck - if elements in the form had #element_validate callbacks they weren't called. I suspect this is because of how complex it is to do this (a delicate balance of form state and form and input values) and not because it's an unworkable approach.

@bnjmnm Ah, right, this also needs ComponentInputsForm::validateForm() and ComponentInputsForm::submitForm() to call ComponentSourceInterface::(validate|submit)Form — that's what the issue title was literally reporting, forgot to call that out in my diff above 😅

Currently, as per @bnjmnm comment #7, we do not call methods proposed by @wim-leers in #4. I've tried adding suggested code as per #7, but that isn't called either.
In fact, ComponentInputsForm::buildForm() has the following comments on L102-L106:

// Prevent form submission while specifying values for component inputs,
// because changes are saved via Redux instead of a traditional submit.
// @see ui/src/components/form/inputBehaviors.tsx

I do not think we should classify this as a bug, as we've never built form submission/validation handling mechanism. We build form to allow user to input prop values, but the only validation performed is on FE side. At least until an attempt to publish/save is made, at which point some form inputs turn out to be invalid, as they were never massaged by "submission" mechanisms.
I suspect that this will be much more involving than just adding code proposed in #4 and #7. Specifically, adapting FE application to do submissions and validation via Drupal methods.
Considering that current UI allows user to enter some text into autocomplete, select a value and click away - there might be some interesting challenges to figure out what constitutes "form submission" or "form requiring validation" if there are no additional UI/UX changes introduced along with this.

And even when such changes are in, I suspect we will need thorough manual and automated testing to find potential edge cases.

Tried the latest MR in xb-demo. I unapplied the change to the block to make it a select box, so it is back to needing this fix, then applied this fix. 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.

The logs have these:

Referrer https://xb-demo.ddev.site/xb/xb_page/1/editor/component/ded7bed0-4370-41...
Message AssertionError occurred during rendering of component ded7bed0-4370-4186-bb97-2ec6490cf548 in Page Awesome Demo Experience Builder (1), field components: Cannot load the "webform" entity with NULL ID.

followed by

AssertionError: Cannot load the "webform" entity with NULL ID. in assert() (line 262 of /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php).

#0 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(262): assert()
#1 /var/www/html/web/modules/contrib/webform/src/Plugin/Block/WebformBlock.php(259): Drupal\Core\Entity\EntityStorageBase->load()
#2 /var/www/html/web/modules/contrib/webform/src/Plugin/Block/WebformBlock.php(112): Drupal\webform\Plugin\Block\WebformBlock->getWebform()
#3 /var/www/html/web/core/lib/Drupal/Core/Block/BlockPluginTrait.php(188): Drupal\webform\Plugin\Block\WebformBlock->blockForm()
#4 /var/www/html/web/core/lib/Drupal/Core/Block/BlockBase.php(36): Drupal\Core\Block\BlockBase->traitBuildConfigurationForm()
#5 /var/www/html/web/modules/contrib/experience_builder/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php(377): Drupal\Core\Block\BlockBase->buildConfigurationForm()
#6 /var/www/html/web/modules/contrib/experience_builder/src/Form/ComponentInputsForm.php(114): Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent->buildConfigurationForm()
#7 [internal function]: Drupal\experience_builder\Form\ComponentInputsForm->buildForm()

Line 259 in WebformBlock is the middle of this:

  protected function getWebform() {
    return $this->entityTypeManager->getStorage('webform')->load($this->configuration['webform_id']);
  }

so if this gets NULL, that does not fulfill what the code expects assuming the default config for the block which is:

  public function defaultConfiguration() {
    return [
      'webform_id' => '',
      'default_data' => '',
      'redirect' => FALSE,
      'lazy' => FALSE,
    ];
  }

Is this considered in the code/XB?

Is this considered in the code/XB?

Yes, it is:

      $settings = [
        // We are using strict config schema validation, so we need to provide
        // valid default settings for each block.
        'default_settings' => [
            // The generic block plugin settings: all block plugins have at least
            // this.
            // @see `type: block_settings`
            // @see `type: block.settings.*`
            // @todo Simplify when core simplifies `type: block_settings` in
            //   https://www.drupal.org/i/3426278
          'id' => $id,
          'label' => (string) $definition['admin_label'],
          // @todo Change this to FALSE once https://drupal.org/i/2544708 is
          //   fixed.
          'label_display' => '0',
          'provider' => $definition['provider'],
        ] + $block->defaultConfiguration(),
      ];

— \Drupal\experience_builder\Plugin\BlockManager::setCachedDefinitions()

Will review & investigate #19.

We need to test #3513742: Machine name JS does not work in Page Data/Component Instance form after this because per @bnjmnm, this issue should address that too 👏

We need to test #3513742: Machine name JS does not work in Page Data/Component Instance form after this because per @bnjmnm, this issue should address that too

To clarify: the issue as reported (which has to do with the JS logic in machine-name.js) is not something that would be fixed here, but it likely already works due to other recent changes.

There was an unrelated bug mentioned in #3513742 that is what #21 is referencing. That bug is related to problems with webform block autocomplete values that expect a machine name. It's also is what @gábor hojtsy tested in #19 - so it's already part of this evaluation - not something additional.

@bnjmnm's issue summary update in #11 is superb — thanks, Ben! 🤩

#3537872: Redux-aware form input components should be aware of direct element manipulation landed yesterday. @Gábor Hojtsy, could you please re-test #19 — I've merged upstream changes into this MR 👍

@larowlan: AFAICT this would make #3509265: Auto-saving of blocks needs to handle string-to-bool fixing 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. But per what's below, I was forced to agree nonetheless 😅

#3484666: Show configuration forms for Block components added the call to ::blockForm(). It did that because it is the only 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 which has a core issue to refactor it away — nope I mixed that up with #3426278: Deprecate unused `provider` exported property from Block config entities
   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 #3526118: CSS assuming block-plugin-rendered-in-a-wrapper-for-the-block-config-entity does not apply to XB: either A) stay the course, B) simulate the same markup structure, too.

   Plus, you've sidestepped the UX nightmare I identified by working your magic in BlockComponent::buildAnonymousFormForBlockPlugin() to remove those 4 form fields 🧙🏽‍♂️👍

Might sound like splitting hairs, but IMO an important distinction to figuring this out:

What @gábor hojtsy is encountering in #19 is something introduced by the MR. In other words it's not part of the underlying problem that this MR failed to account for.

We're now invoking the submit/validate callbacks on the block form. That's overall a good thing, it's needed to massage values to the expected end result.

BUT... The problem is due to logic in \Drupal\webform\Plugin\Block\WebformBlock::blockSubmit, which is now invoked as early as adding the Webform Block to the layout.

public function blockSubmit($form, FormStateInterface $form_state) {
    $values = $form_state->getValues();
    $this->configuration['webform_id'] = $values['webform_id']; // 👈HERE IS THE PROBLEM!
    $this->configuration['default_data'] = $values['settings']['default_data'];
    $this->configuration['redirect'] = $values['settings']['redirect'];
    $this->configuration['lazy'] = $values['settings']['lazy'];
  }

It's looking for $values['webform_id'], which is NULL, instead of the default value which is an empty string.

NULL will fail the !is_null() assertion in \Drupal\Core\Entity\EntityStorageBase::load, but an empty string is handled fine.

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 #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log). 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?

Nope, that's the block component instance form that failed to render, not the corresponding block component preview. Which means I've got nothing to add to Ben's comment :D But if the webform block submission logic wouldn't crash on this, then you'd see what I just described above (and struck through).

🙏 Gábor, next time please post screenshots, that makes it much easier to understand what you're referring to 😇

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 #3540095: Port Cypress tests to Playwright 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.)

Also, AFAICT this:

• does a superset of #3500795 — see #3500795-15: [PP-1] Implement client-side validation of block settings
• makes #3484669 obsolete — see #3484669-12: Define how block settings should be presented in the UI for why

I made a small change that (at least in my manual testing) addresses the Webform Block problem mentioned in #19. 🤞 this doesn't result in a bunch of exploding tests, but easy enough to revert if it does.

Yay, #3484669: Define how block settings should be presented in the UI is now indeed closed, but I was wrong about #3500795: [PP-1] Implement client-side validation of block settings (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...

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 in the inputs field property 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…

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

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) #3521041: [META] Refactor the organically grown Component metadata mess in PHP (`::getClientSideInfo()`) and JS (`BaseComponent`, `DynamicComponent`, `PropSourceComponent` etc.) 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 #3509265: Auto-saving of blocks needs to handle string-to-bool fixing 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 #3484666: Show configuration forms for Block components) 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: #3541125: Integer BlockComponent inputs (i.e. block settings) are incorrectly saved as strings — pre-existing problem in HEAD, this doesn't make it worse
6. follow-up: #3541131: DX: Rename `ComponentInputsForm` to `ComponentInstanceForm` — 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:

webform block (Lee in #30)

auto-save changes disappear when reverting back to the original (boolean) block settings

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

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! 🥳

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

I enabled the Test Block Components module to verify this issue. While testing, I encountered an error—please see the attached video for details.

Console error:

{
    "status": 500,
    "data": {
        "message": "Drupal\\experience_builder\\Plugin\\ExperienceBuilder\\ComponentSource\\BlockComponent::clientModelToInput(): Argument #3 ($client_model) must be of type array, null given, called in /var/www/html/web/modules/contrib/experience_builder/src/Form/ComponentInputsForm.php on line 102",
        "file": "/var/www/html/web/modules/contrib/experience_builder/src/Plugin/ExperienceBuilder/ComponentSource/BlockComponent.php",
        "line": 403,
        "trace": [
            {
                "file": "/var/www/html/web/modules/contrib/experience_builder/src/Form/ComponentInputsForm.php",
                "line": 102,
                "function": "clientModelToInput",
                "class": "Drupal\\experience_builder\\Plugin\\ExperienceBuilder\\ComponentSource\\BlockComponent",
                "type": "->"
            },
        ]
    }
}

@wim-leers, could you please provide the testing steps required to verify this issue?

@mayur-sose Test using the https://drupal.org/project/webform contrib module — before this MR that should fail, with this MR it should work — see the screenshot in #30.

Verified the changes :

@wim-leers, please check attached video for TC3, failed test case.

Nice find, @mayur-sose!

AFAIK, "undo last action" should've in principle worked: it should've undone the change from Contact (contact) (i.e. successfully autocompleted and executed #element_validate => EntityAutoComplete::validateEntityAutocomplete) to test (which was not a valid autocomplete result).

But it didn't.

This gets too far into FE territory for me to determine appropriate next steps, but I'm pretty sure @bnjmn knows 🤓😊 So handing this over to him! See also his excellent overview comment at #3513742-12: Machine name JS does not work in Page Data/Component Instance form. I suspect he'll want to repurpose #3513742 to tackle this remaining bit?

P.S.: closed #3535986 at #3535986-4: Autocomplete machine name handling does not match to machine name, webform picking does not work.

#40 TC doesn't seem particularly Front End to me - it looks like we're allowing invalid values to be published, which can result in errors when attempting to render.

Based on the description / video:

1. A label is manually entered into an entity autocomplete widget that doesn't have a corresponding entity
2. An error then occurs when attempting to publish the content with the no-corresponing-entity chosen

AssertionError: Cannot load the "webform" entity with NULL    
                                                       ID. in assert() (line 262 of                                  
                                                       /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Entity/  
                                                       EntityStorageBase.php). 

From the looks of it, the block is being published despite not meeting validation criteria. When we attempt to do this in OG Drupal, it's not possible to submit:

I suspect we need to do something like #3521759: Publish checks validation constraints, but not form validation but targeted to blocks, so content with invalid field values aren't publishable.
Created #3542102: Prevent publishing when `block` component instance values do not #validate for what was spotted in #40
Created #3542101: Provide better info than "an unexpected error has occurred", to increase bug report quality because it's unfortunate how much helpful info is literally just sitting there to make our lives easier, but we only get "an unexpected error has occurred"

Looks like we missed an edge case: #3561919: Block plugins with checkboxes that are not in the top level of the form do not get the default values assigned.