Compatibility between SDC and the Form API

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies → .

(if you need form API assistance message chx on Drupal Slack. Response times should be below 24 hours but usually below 4 hours.)

Hello,

@ghost of drupal past, thanks for the proposal.

First POC, trying to tell Element::children to got looking into #slots if the element is a component make the form element inside the component to be prepared by the Form API, but:

1. It remains duplicated on rendering (one textfield outside the component, one inside)
2. The 2 elements have the same name attribute, so when submitting the form it is the element in the component which value is retrieved but I think it i only a side effect of having duplicated name.
3. "parent" mechanism in the form API may be confused/malformed.

So next step will be to try to put slots as component element child and so adapt SDC (and later table element too).

But previous results makes this approach very side effect prone.

Yes, that sounds risky, but it worth to check.

Following, I wrote a number of form components for ui_suite_uswds but really serves no purpose :(

Following, I wrote a number of form components for ui_suite_uswds but really serves no purpose :(

They will be usable once 📌 Define form elements from SDC Active is done.

We are actively working on both issues and we already have very promising results.

I think I did use the checkbox component for uswds in the checkbox.html.twig file but it was messy. SO yay to here that!

Yesterday session is online: https://www.youtube.com/live/6AvlzYN17Rs?t=25741s

Until presentation support is posted somewhere you can go give a look.

From @nod_ feedbacks, I am:
- creating another branch with containing only core changes.
- creating a dedicating plugin instead of altering ComponentElement
- need to find a way to make submitted values work without using #name

In MR https://git.drupalcode.org/project/drupal/-/merge_requests/11866, I removed what is only for 📌 Define form elements from SDC Active so that in this issue, we can focus only on SDC and Form API compatibility.

#name is not usable as it is right now because of Ajax.

Currently poking a field widget which will be able to use components, the settings form is broken.

Ok, we found a way to get rid of the #name. Just need to ensure no numeric key.

Before:

$form['component_card'] = [
    '#type' => 'component',
    '#component' => 'core_sdc_form:card',
    'content' => [
      '#type' => 'component',
      '#component' => 'core_sdc_form:card_body',
      'content' => [
        [
          '#type' => 'textfield',
          '#title' => $this->t('Textfield in content'),
        ],
      ],
    ],
];

After:

$form['component_card'] = [
    '#type' => 'component',
    '#component' => 'core_sdc_form:card',
    'content' => [
      '#type' => 'component',
      '#component' => 'core_sdc_form:card_body',
      'content' => [
        'foo' => [
          '#type' => 'textfield',
          '#title' => $this->t('Textfield in content'),
        ],
      ],
    ],
];

This is looking promising, adding various review tags.

This proposal is close to completion, we keep this "Needs work" until we check existing tests and add new ones.

Most of the remaining work is happening in 📌 Define form elements from SDC Active where I am cgecking the SDC philosophy will be "respected":

• UI only, business agnostic, logic and concepts
• Front-dev friendly
• Sharable and design system oriented

Demo gif in #3508641-12: Define form elements from SDC → .

Pushing current work state before cleanup.

With the fix from 🐛 Incorrect render structure in Navigation PageContext Active , the proposed changes in Renderer and ComponentElement do not break existing tests.

Grimreaper's proposal is to allow slots as children to make them work with Form API:

 #type: component
#component: 'provider:foo'
#props:
  prop_1: ''
  prop_2: ''
slot_1: {}
slot_2: {}

And then, in ComponentElement, we move the slots to the expected position in #slots:

 #type: component
#component: 'provider:foo'
#props:
  prop_1: ''
  prop_2: ''
#slots:
  slot_1: {}
  slot_2: {}

Is it the opportunity to deprecate #slots and to promote render children as the expected way of settings slots in SDC ?

Problem with SDC on component with multiple inputs, solved by using [] on name in the component.

We tried to create a lat/lon component with geofield, not working because of applicative
logic into the field type.

Checkbox in form API directly: OK
Checkboxes in form API directly: OK

Usage of names to handle multiple inputs.

Problem of unchecked checkbox currently handled in Checkbox.php form element, workaround in field widget in UIP 2.

TODO:
- field storage and field setting mapping into the component
- test checkboxes as field widget
- test with multiple values fields in field widget
- handle data type constraints?

Hello,

Is there still enough time to make this issue pass for Core 11.2?

Regarding MR https://git.drupalcode.org/project/drupal/-/merge_requests/11866, What would be the new tests to write?

I would say a form with form elements inside components like:

$form['component_card'] = [
      '#type' => 'component',
      '#component' => 'core_sdc_form:card',
      'header' => [
        '#markup' => (string) $this->t('Card'),
      ],
      'content' => [
        '#type' => 'component',
        '#component' => 'core_sdc_form:card_body',
        'content' => [
          'foo' => [
            '#type' => 'textfield',
            '#title' => $this->t('Textfield in content'),
          ],
          'bar' => [
            '#type' => 'select',
            '#title' => $this->t('Select in content'),
            '#options' => [
              'option_1' => $this->t('Option 1'),
              'option_2' => $this->t('Option 2'),
            ],
            '#empty_option' => $this->t('Empty option'),
          ],
        ],
      ],
      'footer' => [
        '#type' => 'textfield',
        '#title' => $this->t('Textfield in footer'),
      ],
    ];

Check that form elements are correctly rendered. And possible to submit the form.

More stuff?
Kernel or Functional test?

And I forgot, comment 8. Thanks @chx for your help proposal :)

Hi,

I made a MR with the tests only: https://git.drupalcode.org/project/drupal/-/merge_requests/12030

And added the test on the main MR: https://git.drupalcode.org/project/drupal/-/merge_requests/11866

I think the Nightwatch fail on the main MR is not related to the changes.

Nightwatch fails often, you can rerun it.

As for test only, you don't usually need a separate branch, there is a test only job, it's at the bottom of the list of tests.

If you run that it will run your tests without the fix or feature.

It should fail.

grimreaper → closed merge request !12030

@godotislate, review feedbacks addressed. Thanks!

@nicxvan thanks, I totally forgot the test only was already out-of-the-box in pipelines.

And yes, now Nightwatch tests are green.

Only 12 lines (once test removed) of code for such an useful feature. That's great.

Let's check if we can merge this for 11.2.

There is 2 last subjects I would like to check:

• As said in #26 📌 Compatibility between SDC and the Form API Active , is it the opportunity to deprecate #slots and to promote render children as the expected way of settings slots in SDC ? Do we decide this before the merge or do we open an issue for 11.3 ?
• We have created a dedicated ticket for the issue found in navigation module: 🐛 Incorrect render structure in Navigation PageContext Active but we still ahev the change in this MR? Do we keep it like that?

We should commit 🐛 Incorrect render structure in Navigation PageContext Active before this issue so that the git history is clearer for people in the future then this can be rebased. I think it's included here so that tests pass.

One question on the MR.

Reviewed the MR, agree with @catch about the type specific logic. We can do this in the render element itself - linked to an example.
If we can't get that done before the alpha deadline it could go in like this with a followup to move it to the element plugin.

Hi,

Thanks everyone for the reviews and feedback.

Looking at it to update MR.

Hi,

Requested changes addressed.

I got a random failure on a CKE5 JS test, after retriggering the job it is ok.

I will test changes against https://git.drupalcode.org/project/drupal/-/merge_requests/11345, to check for 📌 Define form elements from SDC Active .

You can re-review in the meantime if you want.

Other MR https://git.drupalcode.org/project/drupal/-/merge_requests/11345 rebased and updated.

It still works!

Build failed. Mayb pipelines had a transient error, and the build needs rerunning?

Also, the MR should be moved out of draft if it is ready.

No, the MR to look for for this issue is https://git.drupalcode.org/project/drupal/-/merge_requests/11866 which is green.

MR https://git.drupalcode.org/project/drupal/-/merge_requests/11345 was to test this issue and 📌 Define form elements from SDC Active as it has overlapping changes and to ensure changes in this issue will not be incompatible with the other.

godotislate → changed the visibility of the branch 3494634-compatibility-between-sdc to hidden.

I am not very comfortable with the changes done today because the renderer service is now injected and executed in the render element:

    $context = new RenderContext();
    $element['#children'] = $this->renderer->executeInRenderContext($context, function () use (&$element, $context) {
      ...

Instead of doing this complex (from 12 to 55 lines of code) early rendering in a pre_render hook, it seems better to use the render element to only build the expected renderable and let Drupal execute the rendering only once, later, before wrapping the HTML response.

So, in my opinion, the initial proposal was better.

However, we still need to fix this feedback from @catch:

Not sure how much of a real concern this is, but this is the only place in Renderer where we do something #type specific. Is there another way we could detect that this needs to happen, so that it theoretically would work with another element type doing something similar?

I will add a comment tomorrow with a potential proposal.

Pushed a new approach that should be more amenable and also stops the Renderer knowing about a special case for ComponentElement plugin.
Given the net difference my changes from my last review were one unset call and the removal of the special case in the renderer I think I'm still eligible to RTBC this so going to do so. We still have some 'Needs {x} review' tags that need to be removed. I assume @pdureau can do the 'Needs frontend framework manager review' one. Would be good to get a +1 from the SDC maintainers too.

Latest on here looks great. Happy to commit this although additional +1s would be good.

Thanks @larowlan!

Indeed much simpler.

I haved test it with 📌 Define form elements from SDC Active and ✨ Add ui_patterns_field_widgets sub-module Active , POCs on those issues are still working.

So +1 for me.

Looks good to me 👍 Thanks a lot @larowlan & @grimreaper

Committed 1cf4278 and pushed to 11.x. Thanks!