Finalize how we ascertain a `type: array` (aka multiple-cardinality) SDC/JS prop value is empty

I didn't grok this issue summary at first, but by reading https://git.drupalcode.org/project/experience_builder/-/merge_requests/8... too, I did grok it. Added some clarifications.

Related original MR thread: https://git.drupalcode.org/project/experience_builder/-/merge_requests/8...

#3467870: Support `{type: array, …}` prop shapes landed. But, it's impossible right now to close this issue, because per #3467870-28: Support `{type: array, …}` prop shapes, selecting multiple values doesn't quite work yet.

Related:

• #3529788: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases
• #3537945: Narrow the conditions under which we allow a prop to be an empty string

This could be worked on now correct?

#3546869: Editing support for `type: array`: support multiple-cardinality Field Widgets (`StaticPropSource`s) stops the editing but we could still write back tests to confirm whatever approach we decide on works

This code is already removed in #3529788: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases. If this is still a problem, it would be helpful if someone can please add the Steps to reproduce 🙏.

Hello @larowlan, It seems this issue is outdated.

It's still needed.

#3529788: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases introduced

        // Required string component props that evaluate to '' must be retained:
        // while the empty string is NOT considered a valid value, this is the
        // fallback behavior XB opts for to enhance the user experience: it
        // allows a component to render even at the point in time where a
        // Content Author has *emptied* the string input, as they're thinking
        // about what string they do want.
        // ⚠️ This won't work for components whose logic specifically checks for
        // an empty string and refuses to render then.
        // @todo Expand to support multiple-cardinality.
        if ($required && $evaluated === '' && $this->getExplicitInputDefinitions()['shapes'][$prop]['type'] === JsonSchemaType::STRING->value) {
          // Confirm that *if* this weren't special-cased, that this would
          // indeed enter the next branch, which would cause it to be skipped.
          // @todo Consider adding a new `GracefulDegradationPropSource` to
          // encapsulate this similarly to `DefaultRelativeUrlPropSource`.
          assert(!$source instanceof StaticPropSource || ($source->fieldItemList->count() > 0 && $source->fieldItemList->isEmpty()));
        }

It also introduced a regression, which was fixed in #3537154: Regression in #3529788: can publish component instances with empty required props, and then adde3d another @todo:

        try {
          \assert(\array_key_exists('expression', $raw_prop_source) && \array_key_exists('value', $raw_prop_source) && \array_key_exists('sourceType', $raw_prop_source));
          StaticPropSource::isMinimalRepresentation($raw_prop_source);
        }
        catch (\LengthException $e) {
          // During previews, empty values are intentionally allowed. Those must
          // be filtered away when validating, which then in turn MAY trigger an
          // error from ComponentValidator::validateProps() — if this is for a
          // required prop.
          // In other words: let a prop source being emptier than it portrays
          // result in the appropriate validation errors at the component level.
          // @see \Drupal\canvas\PropSource\StaticPropSource::withValue(allow_empty: TRUE)
          // @todo Expand to support multiple-cardinality.
          unset($inputValues[$component_prop_name]);
          continue;
        }

Both of those need to be fixed here.

Discussed MR713 with @tedbow. Approved his test additions on the MR and provided a pointer.

However, that actually is only tangentially related to the original intent/scope of this issue.

• MR713 is about an edge case for invalid example values in an SDC or code component. ← this is a data integrity concern, and is AFAICT basically an edge case we missed in #3568264: Update code component props schema to support 'array' type
• The original intent/scope: correctly handle "allowed to be empty during preview" for multiple-cardinality (type: array) props. See IS + #11.

FYI, I have pushed up https://git.drupalcode.org/issue/canvas-3516754/-/tree/3516754-test-module which contains a test module that allows editing auto-save directly. using it for debugging locally I think because other multi-value issues are still in motion and I tried emptying out an array field from the UI and was not able to.

Since #11 the code @wim-leers referenced has changed in #3546869: Editing support for `type: array`: support multiple-cardinality Field Widgets (`StaticPropSource`s) again and

// @todo Expand to support multiple-cardinality.

Was removed.

Now the code is

try {
          \assert(\array_key_exists('expression', $raw_prop_source) && \array_key_exists('value', $raw_prop_source) && \array_key_exists('sourceType', $raw_prop_source));
          StaticPropSource::isMinimalRepresentation($raw_prop_source);
        }
        catch (\LengthException $e) {
          // During previews, empty values are intentionally allowed. Those must
          // be filtered away when validating, which then in turn MAY trigger an
          // error from ComponentValidator::validateProps() — if this is for a
          // required prop.
          // In other words: let a prop source being emptier than it portrays
          // result in the appropriate validation errors at the component level.
          // @see \Drupal\canvas\PropSource\StaticPropSource::withValue(allow_empty: TRUE)
          // If a StaticPropSource's field item list is empty, consider it not
          // set at all.
          if ($source->isEmpty()) {
            unset($inputValues[$component_prop_name]);
            continue;
          }
        }

Also in that issue \Drupal\canvas\PropSource\StaticPropSource::isEmpty was also added

/**
   * Determines if this prop source is empty.
   *
   * Uses the field type's own isEmpty() logic (via filterEmptyItems()) to
   * determine which items to drop, rather than a PHP-level falsy check. This
   * correctly handles all field types, including those where a value of 0,
   * FALSE, or '' is valid and must be retained.
   *
   * Intended for multiple-cardinality prop sources that arrive in a
   * "mid-input" state during preview/auto-save, where the user may have left
   * some entries blank while still filling in others.
   *
   * @return bool
   *   If the prop is empty.
   */
  public function isEmpty(): bool {
    $filtered = clone $this->fieldItemList;
    $filtered->filterEmptyItems();
    return $filtered->isEmpty();
  }

and test coverage was added to \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\SingleDirectoryComponentTest, specifically testValidateComponentInputFiltersEmptyItemsForMultiCardinalityProps and testClientModelToInputRetainsEmptyArrayForRequiredMultiCardinalityProp

I am still looking this to see if the current is still relavent or if need to add more test covergae

So I pushed up another MR. @wim-leers I am not sure if will agree this relates to the scope of this issue but I do think it relates to the title "Review how we ascertain a `type: array` (aka multiple-cardinality) SDC/JS prop value is empty"
but the issue summary says

The goal is to ensure that empty states don't break the live preview or lead to data loss during the authoring experience.

This would imply this issue is only dealing with the authoring experience so I am not sure this mean to cover this but..

Basically I found that programmatically you can save required prop of the type array with an empty array \Drupal\Tests\canvas\Kernel\Plugin\Field\FieldType\ComponentTreeItemListTest::testEntityLevelValidationAllowsEmptyRequiredMultiCardinalityProp
required in json schema terms just means the key is test not that the array can't be empty. You could enforce it with minItems: 1 but we don't allow that. But it seems from our code comment we don't allow minItems: 1 because we assume `required` would enforce the same rule, which is does not.

We do enforce array props that are required in the UI. I have figured this out through manual testing. I think we should enforce the rules on the API level too, which we currently are not. Or least document that we aren't

See my MR comments and code comments in the MR

#17: I think it definitely relates. That's exactly why we have this issue!

summary update to reflect the current state of !923 and the fact that todos mentioned have actually been removed

I pretty sure this will fail tests be will see

Looking into phpunit test failures first

@isholgueras thanks for the review, I have addressed you feedback

@tedbow asked me to review this.

I had one key concern (see link for details, summarized: What's the point of marking a type: array prop as required if it can be empty? Then it might as well not be required, right?.

Then I saw that this MR is updating the code component editor UI to ensure that whenever a prop is marked as required, minItems: 1 is also set. That's exactly what I proposed!

But I proposed to do this generically: to impose this at the prop-definition-in-JSON-Schema level, i.e. for both code components and SDCs. For code components, this is already making it so. For SDCs, it is not. That's easily solved.

So:

1. I agree with the analysis in the issue summary+MR
2. I disagree with the conclusion: to allow both minItems: 1 and no minItems. If a type: array prop is required without minItems: 1, then it should simply be optional!
3. Code components that have already been created with type: array props need an update path

In my proposal:

Behavioral Changes

Summarized my changes on the MR: https://git.drupalcode.org/project/canvas/-/merge_requests/923#note_811732

Per https://git.drupalcode.org/project/canvas/-/merge_requests/923#note_811745, #3572553: Save Multi-Value Prop Configuration for Code Components should land first.

My concerns in #23 may seem big, but they're not. They're essentially just: "your analysis is right, your solution is right, but you went too far in supporting what does not make sense; we should instead require the sensible thing you applied to code components to everything".

So: fantastic work here! 🤩👏 It resolves a piece of technical debt that I introduced in #3467870: Support `{type: array, …}` prop shapes (I did not handle the "empty" edge case at all in there).

This MR even significantly improves shape matching against (required) multi-value entity fields, resulting in more structured data becoming available in Canvas! 💙

Pushed several more "update tests" commits. I think they illustrate how the slight shift in #23 significantly improves cohesion and reduces complexity.

The only remaining test failures are those in JavaScriptComponent(Validation)Test + JsComponentTest, which illustrate the need for an update path.

I'll leave that for @tedbow & co to address.

Paired with @tedbow on this issue/MR.

He had an insight I was missing: all of the "multi-value prop" functionality was gated behind the canvas_dev_mode module. Hence no code components could've been created that would need an update path!

(The only exceptions could be: code components could've used type: array since #3568264, which shipped in https://www.drupal.org/project/canvas/releases/1.3.0 on March 21. We'll work with @balintbrews to figure out if somebody could've conceivably already used it, and if so, we'll add an update path in a subsequent MR. To be clear: nobody could have created this through the Canvas UI! That's why #3577946: [PP-2] Remove Canvas Dev Mode flag for Multi-Value Props support exists: to actually enable this for everyone. And that's why this issue blocks that one.)

CR pair-written: https://www.drupal.org/node/3587384.

Thanks, every one!