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
Per @hooroomoo π
To confirm: this is what this issue is about?
We've got kind of a meta sprawl going on here π π i.e.: what is the difference in intent/scope between:
- this issue
- vs π± [META] 7. Content type templates β aka "default layouts" β clarify the tree+props data model Active
- vs π [PoC] Introduce a `ContentTypeTemplate` config entity Active
So together with @f.mazeikis and @thoward216, clarifying the scope of this meta! π
- Per @lauriii months ago, we won't be building exposed slots until after 1.0.
- Per @lauriii and @effulgentsia, the
ContentTemplate
s scope for 1.0 is limited to onlyNode
entities β since https://git.drupalcode.org/project/experience_builder/-/commit/843e7b0b1.... - no XB-native view mode creation UI
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.
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));
}
@hooroomoo already confirmed: #3529836-29: Enable starting with an empty XB UI (so without first having to create an entity with a component tree) β ππ
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
π₯³
π 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 π)
β¨ 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) β .
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 :)
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
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 .
wim leers β created an issue.
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.
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 :)
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:
Thanks to @hooroomoo's test of this together with their PoC, I can now land this with confidence π₯³
Out of scope:
- making these API responses cacheable β if the need ever arises, we'll do that then
- 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
- 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 .
- 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.
wim leers β created an issue.
wim leers β created an issue.
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. π
That was fast!
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 π€
I think this actually all was finished back in #75 π€
Yay, thanks everyone! π₯³
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:
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...
Thanks, all, and especially @larowlan & @bnjmnm!
- Added π Integer BlockComponent inputs (i.e. block settings) are incorrectly saved as strings Active .
- Closed π Define how block settings should be presented in the UI Active .
- Landed π ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active , which allowed closing π Auto-saving of blocks needs to handle string-to-bool fixing Active .
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! π₯³
Perfect, I have everything I need now to finish this up :)
Thanks for following the pattern set by π External AI Chatbot Functionality Active β and adding the missing test coverage! π€© π
@lauriii, see #8.
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
.)
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 :)
Provide an access control handler for personalization segments using our single permission βmanage personalizationβ.
This was approved by @lauriii?
If so: π
How does this relate to
π
Provide a locked Default segment
Active
? Will that have a special reserved weight that no other Segment
can set?
Showing this working, lifting the GIF from #3523496-34: Block component instance form values not processed by validation/submit handlers β :
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 π
Manually tested this again on #3523496, and I can confirm that this is fixed there! π₯³
Added π Rename `ComponentInputsForm` to `ComponentInstanceForm`, to match the route name Active .
wim leers β created an issue.
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 .
wim leers β created an issue. See original summary β .
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β¦
Given #3520484-76: [META] Production-ready ComponentSource plugins β , this actually won't be blocking a stable (1.0) release.
This is causing significant confusion and delays in π ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active , see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
Ahhhh! Then I get it! π
Thanks β looks great now! π
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.
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.
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()
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...
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.
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.
βΉοΈ @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.
"wisely" and "AI" in the same sentence π€π€£
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.)
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 π€
Very close!
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?
Neha, could you check if this is still relevant? See #7 for why. Thanks! π
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?
(Or perhaps #config_schema
, rather than #config_target
, because #config_target
is designed to automatically update the target config too.)
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?
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 .
Thanks :)
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.
hooroomoo β credited wim leers β .
balintbrews β credited wim leers β .
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
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:
- #7 is now being handled in π ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active .
- #9 points to #3500795-8: [PP-2] Implement client-side validation of block settings β , which proposes to do what @larowlan's MR at π ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active is already doing!
All good IMHO π
π 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 ablock
component instance?
So: IMHO we should close this issue β client-side validation for block component instances just doesn't make sense.
Thanks! Thatβs what I thought/hoped β but apparently failed to link π
Oh, right. Great, then #6 is a solid plan! π Thanks :)
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.)
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?
@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:
\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 π§π½ββοΈ
Doesn't this become obsolete thanks to π ComponentSourceInterface::submitConfigurationForm and validateConfigurationForm are never called Active ? π€
#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.)
+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.
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.)
need the ability to render any entity type through an existing content template
As of https://git.drupalcode.org/project/experience_builder/-/commit/843e7b0b1..., ContentTemplate
s 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)
I'm fine with adding this, but am worried we'll break it in the future. Is there a way we can test this?
Great, that means @lauriii confirms #8.2 π
Thanks!