Overview

We need a component that allows testing form backend + frontend integration for the forms. This is a similar idea to #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" but with the goal of simplifiying the development / styling of form elements.

Proposed resolution

Create a testing component that has props for every form field supported out of the box. If there are fields that are not working out of the box, we should open follow-up issues to address any challenges.
Update the existing testing component sdc_test_all_props:all-props to:

  1. ✅ render all its props (and actually render at all: #15)
  2. ✅ have default values for most props
  3. ✅ have a Component config entity that is automatically installed when the sdc_test_all_props module gets installed, for all prop shapes that have a working field type+widget (see \Drupal\Tests\experience_builder\Kernel\PropShapeRepositoryTest::testStorablePropShapes(), introduced by #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings) — this automatically configures all appropriate field types, storage settings and widgets (until #3462636: Preview component when selecting in left sidebar lands)

User interface changes

Before
Most props do not have default values and the all-props component fails to render anything (root cause: #15, upstream issue: #3465228: Twig disallows dashes in variable names, so SDC should disallow it in prop names and slots
After

(Note the invisible field widget labels — this will be fixed by #3465140: Regression: prop labels present on field widgets, but invisible.)
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

lauriii created an issue. See original summary.

wim leers’s picture

This component already exists, for the very reasons cited here: tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml.

It's existed for \Drupal\Tests\experience_builder\Kernel\SdcPropToFieldTypePropTest, precisely for If there are fields that are not working out of the box, we should open follow-up issues to address any challenges. — but long prior to us having the ability to generate widgets, just finding viable storage candidates.

(It has even existed before the 0.x branch was created!)

And I'm literally on the path towards making exactly the intent of this issue feasible: #3461499-16: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings.

wim leers’s picture

wim leers’s picture

Status: Active » Postponed (maintainer needs more info)

So, can you please check if anything is missing from that all-props component? If you can't find anything missing, we can mark this issue as obsolete and unpostpone the issue it is blocking.

lauriii’s picture

Assigned: Unassigned » bnjmnm

Great, thank you for pointing that out!

@bnjmnm can you confirm if this meets the requirements or if more work is needed?

wim leers’s picture

Status: Postponed (maintainer needs more info) » Needs review
bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Needs review » Active

If this winds up providing all the necessary fields, then there should be at least an install hook added to sdc_test_all_propsso it is available as a Page Builder component as soon as the module is installed.

I attempted to add the "All Props" SDC as a Page Builder component and got
ValueError: "" is not a valid backing value for enum Drupal\experience_builder\SdcPropJsonSchemaType in Drupal\experience_builder\SdcPropJsonSchemaType::from()

If the error 👆 is addressed then I can review how this renders in the edit form and recommend if anything additional is needed.

(I also noticed sdc_test_all_props.info.ymlhas a dependency on sdc:sdc which shouldn't be needed now that it is in core)

bnjmnm’s picture

Assigned: Unassigned » bnjmnm

Addressed "" is not a valid backing value and digging into additional errors that surfaced once that was addressed.

bnjmnm’s picture

I'm getting an error on any props of type: integer

InvalidArgumentException: Field test-integer is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 616 of  
                                         /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php).      

Which happens after calling getWidget() in formTemporaryRemoveThisExclamationExclamationExclamation

wim leers’s picture

Assigned: bnjmnm » wim leers

I'll test this with #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings — that issue should make this one much easier to push forward 🤞

wim leers’s picture

wim leers’s picture

Title: Create a component for testing form backend + frontend integration » [PP-1] Create a component for testing form backend + frontend integration
Assigned: wim leers » Unassigned
Status: Active » Postponed

Working on it, and as of commit e83470a84c89907bcdd73898485e5a78f03a383f, you should be unblocked here, @bnjmnm. For now, it'll require you to have #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings applied.

But really, the point here is that such an "all possible shapes" component exists (which it already does) and has a working right sidebar (which will be the case after #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings).

So: there's nothing left to be done here for now, except wait until #3461499 is done, and then revisit.

wim leers’s picture

Title: [PP-1] Create a component for testing form backend + frontend integration » [PP-1] Use the `all-props` component for testing form backend + frontend integration

I've been working on expanding the test coverage over at #3461499 to verify that we truly can generate all the necessary widgets for the all-props component. I've already found a number of edge cases that indicate why @bnjmnm could not possibly have gotten this to work 😅

This is definitely hard-blocked on #3461499, and soft-blocked on #3463999 — we won't quite need that to be finished too, because I can just attach a sample config entity here 👍

Stay tuned.

wim leers’s picture

Title: [PP-1] Use the `all-props` component for testing form backend + frontend integration » Use the `all-props` component for testing form backend + frontend integration
Assigned: Unassigned » wim leers
Status: Postponed » Active
wim leers’s picture

Issue tags: +Needs upstream bugfix

LOLOL an hour worth of debugging only to find out that SDC prop names may contain dashes but Twig variable names must not: https://github.com/twigphp/Twig/issues/1967 — for example

    {{ test-REQUIRED-string }}

is compiled by Twig to:

        // line 5
        yield $this->extensions['Drupal\Core\Template\TwigExtension']->escapeFilter($this->env, ((($context["test"] ?? null) - ($context["REQUIRED"] ?? null)) - ($context["string"] ?? null)), "html", null, true);

… which means that the DX of SDC is leaving a lot to be desired: if Twig doesn't support this, and SDC supports only Twig, then I expect SDC to not allow this either. This is so utterly absurd 🤪

Tagging Needs upstream bugfix to file an upstream SDC bug report.

wim leers’s picture

Voila, @bnjmnm is now unblocked 😊

wim leers’s picture

Issue summary: View changes
Status: Needs work » Needs review

@bnjmnm Feel free to merge when you like — this is ready as far as I'm concerned.

If you want, you can add more props to test against. But if this suffices, then I think it's ready for you to confirm that you can trigger the above (by installing sdc_test_all_props), then you and others working on the front end can test/improve field widgets using this SDC 😊

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Needs review » Needs work

I can't add this as a Page Component, but I suspect it will be easy to address as the error specifies a line that was committed only yesterday

 Warning: Undefined array key "#markup" in Drupal\experience_builder\Controller\SdcController->preview() (line 370 of                                            
                                          /Users/ben.mullins/Sites/drupal/modules/custom/experience_builder/src/Controller/SdcController.php) #0                                                          
                                          /Users/ben.mullins/Sites/drupal/core/includes/bootstrap.inc(108):
wim leers’s picture

Assigned: Unassigned » wim leers

Odd. Will double-check tomorrow with a fresh install.

wim leers’s picture

Assigned: wim leers » bnjmnm
Status: Needs work » Needs review
composer require drush/drush
vendor/bin/drush si standard --account-name=root --account-pass=root --yes
vendor/bin/drush pm:install experience_builder --yes
vendor/bin/drush pm:install sdc_test_all_props --yes

Followed by creating node 1, then going to /xb, results in the screenshot I posted in #17. I didn't need to change anything.

Any chance you just checked out this branch and already had the sdc_test_all_props installed, which would've caused the optional config to not get installed, which would explain I can't add this as a Page Component? 🤔

(Fixed the warning in #19 though!)

bnjmnm’s picture

Assigned: bnjmnm » Unassigned

Starting from scratch gets the component to install and I can see it in the XB UI and it does its job nicely

The problem I ran into earlier (or something similar to it) seems to be happening still - the edit/add form for this SDC isn't working. This is less urgent since it installs with the module now, but here's what I'm running into when I try to edit in admin/structure/component/edit/sdc_test_all_props:

If I try to edit the all-props component when I click submit on the form, the submission is actually blocked by JS and there are console errors: An invalid form control with name='sdc_test_all_props+all-props[<<daterange field|UUID field>>][field_type]' is not focusable. ​. It was reasonably easy to conclude this is due to the date range and UUID props not having field types selected - but still required more sleuthing than I'd expect a normal user to do (I could see this being out of scope here since it is a test-only component)

Setting the field types in the form allowed me to submit the form, but I got the error

Error: Call to a member function massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation() on null in  
                                          Drupal\experience_builder\Form\ComponentEditForm->copyFormValuesToEntity() (line 317 of                              
                                          /Users/ben.mullins/Sites/drupal/modules/custom/experience_builder/src/Form/ComponentEditForm.php).    

Since this is test only and VERY helpful to have, I think it's reasonable to address the edit form issues in another issue. For the time being, I recommend adding something specific to the edit form for this component that warns about this and perhaps makes the edit fields unavailable "This test-only component is provided as-is and cannot be edited" so we don't run into contributors losing their (or our) time assuming this would work.

wim leers’s picture

Assigned: Unassigned » bnjmnm
StatusFileSize
new75.73 KB
new64.85 KB

the edit/add form for this SDC isn't working.

+

[…] when I click submit on the form, the submission is actually blocked by JS and there are console errors […]

Ahhh, now I get why you said it wasn't working 😅

That's known. Because configuring each component's field type + widget etc in detail using a UI is non-desirable per @lauriii. That's why #3461499: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings ignored that limitation, and why #3463999: Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria is next, and why once that is done, #3464025: Remove Component config entity's add/edit form will happen.

TL;DR: the current ComponentEditForm only works for components whose props are simple enough, not for arbitrarily complex SDC props.

I think it's reasonable to address the edit form issues in another issue

Ahead of you — it will disappear shortly, in #3464025: Remove Component config entity's add/edit form 😊

For the time being, I recommend adding something specific to the edit form for this component that warns about this and perhaps makes the edit fields unavailable "This test-only component is provided as-is and cannot be edited" so we don't run into contributors losing their (or our) time assuming this would work.

Good point, we should make sure people don't waste time until #3464025 lands, done:

… and just to make sure: other components do not get this warning:

  • bnjmnm committed 117913d3 on 0.x authored by Wim Leers
    Issue #3463583 by Wim Leers, bnjmnm: Use the `all-props` component for...
bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Needs review » Fixed

I tried this out and it works great. If anything is missing NBD - we just add to it later - as-is this unlocks a bunch of things we've been aiming to do.

wim leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.