- Issue created by @larowlan
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This AFAIK ties into follow-ups opened for 📌 Add support for Blocks as Components Active that were never followed up on. A quick scan suggests:
- 📌 Define how block settings should be presented in the UI Active
-
📌
[PP-1] Implement client-side validation of block settings
Active
(see
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::fixBooleansUsingConfigSchema()
) - etc.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
This should be fixed before 1.0, but not necessarily before 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active — because A)
ComponentSource
plugins won't be a stable API yet, B) I don't see how this could impact the data storage. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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); } /**
- First commit to issue fork.
- 🇭🇺Hungary Gábor Hojtsy Hungary
I'm running into this in 🐛 Autocomplete machine name handling does not match to machine name Active with the webform block.
Webform uses
Drupal\Core\Entity\Element\EntityAutocomplete
which has avalidateEntityAutocomplete()
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). - 🇺🇸United States bnjmnm Ann Arbor, MI
The
validateConfigurationForm
/submitConfigurationForm
methods inExperienceBuilder/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. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@bnjmnm Ah, right, this also needs
ComponentInputsForm::validateForm()
andComponentInputsForm::submitForm()
to callComponentSourceInterface::(validate|submit)Form
😅 - 🇬🇧United Kingdom f.mazeikis Brighton
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.tsxI 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.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I have some thoughts, I'll push this along a bit
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Pushed up a POC but we've got a couple of open threads/questions
* 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
* We have issues with #tree - BlockForm in core uses sub-form state and sets#tree => TRUE
on the form. If we do don't do that we end up with a mismatch in how the form works between here and normal block forms. But if we do that, we end up with extra values in the input names - e.g. see\Drupal\system\Plugin\Block\SystemBrandingBlock::blockSubmit
which expects to seeblock_branding
in the values. We don't currently have that because we're working with the schema for the config (as seen in\Drupal\system\Plugin\Block\SystemBrandingBlock::defaultConfiguration
where there is noblock_branding
. We also have\Drupal\experience_builder\Form\ComponentInputsForm::applyElementParents
to mess with#parents
.I suspect the way forward will be to get
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::buildConfigurationForm
into alignment withBlockForm
and use sub-form state and properly pass alongsettings
and#tree => TRUE
to the FE, then in::clientModelToInput
we can undo that in a repeatable fashion, so that we can call::blockSubmit
for blocks likeSystemBrandingBlock
The failing e2e catches these issues 🙌 so that block was a good choice for test cases!
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
larowlan → changed the visibility of the branch 3523496-componentsourceinterfacesubmitconfigurationform-and-validateconfigurationform to hidden.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
larowlan → changed the visibility of the branch 1.x to hidden.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
@gábor hojtsy any chance you could test this branch with webform to confirm it resolves the issue?
The test module I've added has a block form with an entity autocomplete widget so 🤞 it does.If we're lucky, it's this simple:
Famous last words 😂, this one turned out to be quite the battle
- 🇭🇺Hungary Gábor Hojtsy Hungary
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?
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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.
- 🇫🇮Finland lauriii Finland
We need to test 📌 Machine name JS does not work in Page Data/Component Instance form Postponed after this because per @bnjmnm, this issue should address that too 👏
- 🇺🇸United States bnjmnm Ann Arbor, MI
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.
- 🇧🇪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 towardsBlock
config entities, because:\Drupal\Core\Block\BlockPluginTrait::buildConfigurationForm()
is what implements it — an optional (but encouraged) trait, also used by\Drupal\Core\Block\BlockBase
- 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 forBlock
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:
provider
is pure noise — nope I mixed that up with 📌 [PP-1] Deprecate unused `provider` exported property from Block config entities Postponedadmin_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 UIlabel
(and its siblinglabel_display
) is used byblock.html.twig
, which XB doesn't use, because that's associated with Block config entities
😬🫠 … except that apparently
type: block_settings
dictates thatlabel
,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 🧙🏽♂️
- In
- 🇺🇸United States bnjmnm Ann Arbor, MI
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 isNULL
, 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. - 🇧🇪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 🇧🇪🇪🇺
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:
- the use of
resolved
but that then not matching the exact inputs expected byblock
-sourced components: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... - +1 for your suggestion to lift some things into a new service, which then both
BlockComponent
andClientDataToEntityConverter
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. - concerns about
::fixBooleansUsingConfigSchema()
still being around despite your remark at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... — see - 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
- 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.)
- the use of
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Also, AFAICT this:
- does a superset of #3500795 — see #3500795-15: [PP-2] 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
- 🇺🇸United States bnjmnm Ann Arbor, MI
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.
- 🇧🇪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
brokeBlockComponent::fixBooleansUsingConfigSchema()
— see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... - 🇧🇪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 🇧🇪🇪🇺
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 🇧🇪🇪🇺
So:
- 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 - was able to actually remove
::fixBooleansUsingConfigSchema()
because it was clearly dead code — which makes my #31 obsolete - 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 🥳
- 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 - 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
- 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 👍
- added docs to clarify the (pre-existing!) messy situation with
- 🇧🇪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! 🥳
-
wim leers →
committed acf682ef on 1.x authored by
larowlan →
Issue #3523496 by wim leers, larowlan, bnjmnm, gábor hojtsy, f.mazeikis...
-
wim leers →
committed acf682ef on 1.x authored by
larowlan →
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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?
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@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 📌 ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active .
Verified the changes :
@wim-leers, please check attached video for TC3, failed test case.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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
) totest
(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 → .
- 🇺🇸United States bnjmnm Ann Arbor, MI
#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:
- A label is manually entered into an entity autocomplete widget that doesn't have a corresponding entity
- 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 📌 Publish checks validation constraints, but not form validation Active but targeted to blocks, so content with invalid field values aren't publishable.
Created 📌 Prevent publishing when block values do not #validate Active for what was spotted in #40
Created 📌 Provide better info than "an unexpected error has occurred" Active 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"