Overview

In #3487284: [META] Add explicit end-to-end test coverage for using all core field widgets including multiple cardinalities in the "Page Data" (content entity) form we're adding e2e tests for core widgets.

Proposed resolution

Add explicit e2e coverage to entity-form-field-types-test.cy.js for boolean checkbox widget

User interface changes

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

larowlan created an issue. See original summary.

omkar-pd made their first commit to this issue’s fork.

omkar-pd’s picture

Status: Active » Needs review
penyaskito’s picture

Status: Needs review » Needs work
omkar-pd’s picture

Status: Needs work » Needs review
larowlan’s picture

Pushed the required fix here.

The nuance is this in FormBuilder::handleInputElement

// Get the input for the current element. NULL values in the input need
        // to be explicitly distinguished from missing input. (see below)
        $input_exists = NULL;
        $input = NestedArray::getValue($form_state->getUserInput(), $element['#parents'], $input_exists);
        // For browser-submitted forms, the submitted values do not contain
        // values for certain elements (empty multiple select, unchecked
        // checkbox). During initial form processing, we add explicit NULL
        // values for such elements in FormState::$input. When rebuilding the
        // form, we can distinguish elements having NULL input from elements
        // that were not part of the initially submitted form and can therefore
        // use default values for the latter, if required. Programmatically
        // submitted forms can submit explicit NULL values when calling
        // self::submitForm() so we do not modify FormState::$input for them.
        if (!$input_exists && !$form_state->isRebuilding() && !$form_state->isProgrammed()) { // 👈️👈️👈️👈️👈️👈️👈️
          // Add the necessary parent keys to FormState::$input and sets the
          // element's input value to NULL.
          NestedArray::setValue($form_state->getUserInput(), $element['#parents'], NULL);
          $input_exists = TRUE;
        }

With \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields we're doing a $form_state->setProgrammed() so that set value never happens. Therefore the old code that was unsetting (or rather filtering out) unticked checkboxes actually needs to be setting them to NULL rather than removing them.

With that change in place, the test passes 🙌

bnjmnm made their first commit to this issue’s fork.

bnjmnm’s picture

Status: Needs review » Needs work

The MR looks good, but could this be expanded to include a checkbox with a default value unchecked, in addition to the existing one with ['value' => 1],?

wim leers’s picture

Assigned: Unassigned » larowlan
Issue tags: +Needs tests

#9: great call! This one is tantalizingly close!

larowlan’s picture

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

Rebased, addressed review comments and #9

wim leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

@bnjmnm's feedback in #9 has been addressed 👍

  • wim leers committed bc8ce91c on 0.x authored by omkar-pd
    Issue #3518313 by larowlan, omkar-pd, bnjmnm, wim leers, penyaskito,...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed
wim leers’s picture

Assigned: Unassigned » bnjmnm
Status: Fixed » Needs review

Looks like this somehow caused consistent failures in entity-form-field-types-test.cy.js despite multiple CI runs on this MR being green— see https://git.drupalcode.org/project/experience_builder/-/commit/bc8ce91c5....

wim leers’s picture

OMG I AM AN IDIOT 😭🙈

wim leers’s picture

Assigned: bnjmnm » Unassigned
Status: Needs review » Reviewed & tested by the community

Ben's my hero of the day! 🦸

  • wim leers committed 80a29aaf on 0.x authored by bnjmnm
    Issue #3518313 by bnjmnm: Update new test to use `/xb/api/…` → `/xb/api/...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Thanks so much!

Status: Fixed » Closed (fixed)

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