Problem/Motivation

Two bugs in component forms.

1. Broken #states. Field formatters in slots build their #states selectors against the field UI form structure, and nothing rewrites them. FieldFormatterSource already says so:

// Should we use FormHelper::rewriteStatesSelector() ?
// like in FieldBlock::formatterSettingsProcessCallback.

2. Lost submitted values. ComponentForm::elementValidate rebuilds the value with setValueForElement(), discarding anything missing from the raw input, such as unchecked checkboxes.

Steps to reproduce

- Install demo_umami, enable ui_patterns
- Go to /admin/structure/types/manage/article/display/full/layout
- Add a ui_patterns block with a timestamp field formatter in a slot
-- Source: [Entity] ➜ [Field], Field: Changed
-- Source: [Field] Formatter, Formatter: Default
- "Display as a time difference" does not toggle its dependent settings (bug 1)
- Check it, save, reopen: the setting is gone (bug 2)

Proposed resolution

Rewrite the selectors with FormHelper::rewriteStatesSelector(). Stop overwriting the form state with a rebuilt value, and unset only the keys that are not part of the configuration.

Issue fork ui_patterns-3549924

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:

About issue forks

Comments

herved created an issue. See original summary.

herved’s picture

herved commented
Issue summary: View changes

herved opened merge request !444

herved’s picture

herved commented
Issue summary: View changes

Created MR, this solves states not working, but the "enabled" key is not saved when using the TimestampFormatter, causing PHP notices.

Warning: Undefined array key "enabled" in Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampFormatter->viewElements() (line 319 of core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php).

It looks like there is something not quite right with how FieldFormatterSource builds the form/subform.
Maybe we need to compare and check exactly how \Drupal\layout_builder\Plugin\Block\FieldBlock does it.

Edit: Ooof, not an easy debug, \Drupal\ui_patterns\Element\ComponentForm::elementValidate is very suspicious and seems to cause the loss of that checkbox value.
Before drupal hits this method, the checkbox value is in form state, but then that method overrides the values in the form state via $form_state->setValueForElement(). That code is very odd and likely exists to work around another underlying bug.

herved’s picture

herved commented

Unfortunately there is no test coverage for this ComponentForm::elementValidate
So I'm not sure what to make of it...
Looking at the gitlog, it originates from:
- https://git.drupalcode.org/project/ui_patterns/-/commit/509f99793542456e...
- https://git.drupalcode.org/project/ui_patterns/-/commit/b2216dcb5daa8653...

Moving to review to get some feedback.

herved’s picture

herved commented
Status: Active » Needs review
herved’s picture

herved commented
Status: Needs review » Needs work
herved’s picture

herved commented
Status: Needs work » Needs review

Could you please review this? It seems there are 2 issues:
1. states api not working
2. and I made an attempt for the ComponentForm::elementValidate and related code

herved’s picture

herved commented
Status: Needs review » Needs work

Moving back to needs work, the LB configure form closes unexpectedly with this when adding something in a slot.
I have to dig further, possibly split off the 2nd issue.

herved’s picture

herved commented
Status: Needs work » Needs review

It seems the main goal of ComponentSlotForm::elementValidate is to remove some values (add_more_button, _remove) from the final submitted form values which ultimately ends up in config.

I believe form elements ['#value'] should not be changed like this.
Nor should $form_state->setValueForElement() be used on the whole component_form element in ComponentForm::elementValidate otherwise we loose such empty checkbox values which are present in form_state.
But instead use $form_state->unsetValue() in ComponentSlotForm::elementValidate.
There are some examples in core where this pattern is used to cleanup unwanted values in form_state.
So I updated accordingly.

I'm not 100% sure on this or that it won't have side effects in ui_patterns so a thorough testing is needed.
I do see some calls to $form_state->setValueForElement() in ui_patterns_ui.

herved’s picture

herved commented

phpmd job seems rogue

just_like_good_vibes’s picture

just_like_good_vibes
Location PARIS
commented
Assigned: Unassigned » just_like_good_vibes

hello,
thank you for reporting that error and for contributing.
we will check this soon.

herved’s picture

herved commented
Title: Use FormHelper::rewriteStatesSelector after rendering field formatter settings forms » Broken #states in formatter settings forms and loss of submitted form values in component forms
herved’s picture

herved commented

MR rebased and applied a few changes to align with 2.0.x

Also, third_party_settings holds an array, so it cannot be carried in a hidden input: it renders as value="Array" and comes back as a string. It needs #type => 'value', core's element for data that must survive a submit without being sent to the browser. Fixed here for slot sources. UiPComponentFormDisplayForm and SourceComponentWidget have the same problem, left out of scope.

herved’s picture

herved commented
Issue summary: View changes