[PP-1] Implement client-side validation of block settings

This to me is definite polish, a nice-to-have, not a must-have for 🌱 Milestone 0.2.0: Experience Builder-rendered nodes Active .

Why?

Because:

• 99% of BlockPluginInterface implementations' forms do not have any validation at all — client nor server-side. The range of possible settings values allowed through the UI is constrained only by the HTML input element they picked (<select>, <input type="checkbox">, etc.)
• It's only since a few months (since 📌 Make Block config entities fully validatable Fixed ) that a few block plugins are fully validatable …
• … and it's only those few block plugins that have a precise config schema that would even allow for this! 😰
• I'm convinced that >50% of placed blocks contain invalid/nonsensical block settings, that only sort of work due to PHP's automatic typecasting — having worked on https://www.drupal.org/project/acquia_migrate → , and having seen the incredibly weird/convoluted/puzzling "settings" many placed blocks were accepting without complaining.
• Actually, even outside a migration context, the only reason block plugins' settings forms work at all is precisely because both the PHP typecasting magic in the form definition is matched with the Form API submission typecasting magic, and is matched with the block plugin's ::build() method's interpreting of the saved values

📌 Implement saving block settings forms Active is in, but I imagine we'll at least want to do 📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active first?

📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active is in, too.

📌 Decouple generated source plugins from SDC plugins Active blocks this in my book

Per #3513589-6: [PP-1] Decouple generated source plugins from SDC plugins → , that is PP-1.

Just had a long chat with @bnjmnm about this … interesting problem space.

He had a very interesting proposal/observation/idea! 😄

What if … we don't do what this issue is proposing:

Attempt to add a translation layer from block configuration schema to json schema and send this schema for block plugins.

(which would be a huge pile of complexity!)

What if instead, we'd apply an approach similar to what we do for the (aka content entity) form? i.e.:

• use the regular FormBuilder-powered form submission logic
• hence use the same (PHP/server-side) validation logic (for those block plugins that do have it)

That would avoid the need for new APIs, new transformation layers, and would likely result in some shared infrastructure between the and block component forms.

Especially because the form already uses most (if not a superset!) of the #types listed at #3520484-70: [META] Production-ready ComponentSource plugins → that we currently have little confidence (because no explicit tests and no block plugins exercising them) in all of those working correctly.

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 for \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent?

wim leers → credited bnjmnm → .

Yes I agree, I wrote something similar in 📌 Define how block settings should be presented in the UI Active yesterday - basically that we need to be calling ::blockValidate and ::blockSubmit from ::clientModelToInput in the block source plugin.

We can use this issue or that one to do that work

Oh, great! 🥳

That other issue seems to be the closer fit, so let’s do it there. I’ll point back here 👍

Untagging "stable blocker" because I don't see why we couldn't add client-side validation at any time, including after a 1.0.0 release, but please re-tag if you disagree. We already have 📌 ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active as a stable blocker for the server-side part.

📌 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.

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

I'm 90% in agreement here. It's arguably essential that the Block Component Instance form use form API based submit and validation callbacks like the page data form does, and that provides the necessary validation for these forms.

However (the annoying 10%), I think there's a level of clientside validation that would be nice to have & feasible. Since block requires constraints to qualify as a component-block + some settings like required might be easily mapped, there are some use cases where we might be able to associate a JSON schema specification with a given input and leverage the existing logic for SDC settings. It's not critical to add this, but it could provide faster feedback for content authors.

Yes, I agree with #16 - if we can provide nice UX ™️ we should
But I think with that other issue this is normal, not major - because we have validation at least

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.

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 .

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?

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