Regression in #3529788: can publish component instances with empty required props

I did some debugging on this.

I think what is happening is that "Required" string property does not error if an empty string is sent back

Tracked this down to \JsonSchema\Constraints\UndefinedConstraint::validateCommonProperties

/ Verify required values
        if ($this->getTypeCheck()->isObject($value)) {
            if (!($value instanceof self) && isset($schema->required) && is_array($schema->required)) {
                // Draft 4 - Required is an array of strings - e.g. "required": ["foo", ...]
                foreach ($schema->required as $required) {
                    if (!$this->getTypeCheck()->propertyExists($value, $required)) {
                        $this->addError(
                            ConstraintError::REQUIRED(),
                            $this->incrementPath($path, $required), [
                                'property' => $required
                            ]
                        );
                    }
                }
            } 

So it just checks if the property exists.
I checked and added minLength: 1 to heading.component.yml for "text" then it does fail validation on publish with an empty string.

Not sure if this is the expected for SDC validation

I tested tests/modules/xb_test_sdc/components/containers/two_column/two_column.component.yml and got rid of the `$ref: json-schema-definitions://experience_builder.module/column-width` for "width" so that it would just be treated as regular integer which would allow not filling out this field. I did get a "The property width is required." error when trying to publish in this case.

I am not sure what the correct solution is here. We could do some shenanigans on the server-side to unset string properties that are empty and required.
Or we could add minLength: 1 to all string properties we want to be non-empty
Or we could add a new json-schema-definitions://experience_builder.module/non-empty-string` schema
Or we could add some sort of higher level validator that would make sure all require strings are not empty

> Or we could add minLength: 1 to all string properties we want to be non-empty

That's my fav of the proposals. We would need to verify that there is not a minLength already set, and of course SDC authors shouldn't need to do this if they already specified 'required'

... but we are limited by

a) metadata is read-only in core, we cannot alter it.
b) Ted tried this, and of course previews will show as broken until the value is filled, which isn't acceptable. Which means even if we could do that, we would need a way to only do it on publishing.

So the less elegant options

• I am not sure what the correct solution is here. We could do some shenanigans on the server-side to unset string properties that are empty and required.
• Or we could add some sort of higher level validator that would make sure all require strings are not empty

are the only realistic options right now.

I am looking into solutions in #5

Started a rough MR. Manually testing it also stops publishing an empty string prop. Added basic test coverage

addressing the feedback

This is one we'll want to cherry pick to 0.x after it lands in 1.x, so tagging for that.

I think the root cause of this was these two commits added late to #3534601: Add a Video prop type to the Code Component editor; requires adding `file` as default `StaticPropSource`, `FileWidget` support and various infrastructure improvements

1. The changes to StaticPropSource here
2. The test changes here to make the above change pass

I think we should consider reverting these two commits (the first one only for changes to StaticPropSource).

Additionally, I think we should revisit the approach we took 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 - I've reopened that and commented over there with my reasons - tl;dr I don't think we should allow the user to bypass clientside validation by editing any other field.

I added a different approach in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1342 which is basically #12 and #3529788-21: 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

Responded in-depth at #3529788-24: 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 why I think the alternative MR is the wrong direction — tl;dr: this is a usability regression for the reasons @lauriii cited at #3529788-23: 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.

The mistake I made 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 (besides it not being sufficiently narrow in special-casing allowing the empty string — follow-up MR on that issue to fix that) is that while StaticPropSource::withValue() would complain hard and loud upon trying to load/edit a component tree with a required type: string, minLength: 0 SDC prop, it SHOULD have prevented it from being saved in the first place.

@tedbow's test coverage nicely reproduces that, and it's the test coverage I should've added in #3529788. My bad. 🙈

The IMHO correct solution was actually pretty simple, and what I suggested ~24 hours ago (aka 2 MRs ago — MAN, this issue is moving fast 🙃 🤯) worked fine:

1. commit 1: port Ted's test coverage — fails hard 👍
2. commit 2: do an extra check in ::isMinimalRepresentation(), similar to what #3529788 did for ::withValue(), which SHOULD work because ::buildPreviewRenderable() already is ignoring any validation errors precisely because it's for previewing — reduces the massive fail to a narrow one: a message mismatch
3. commit 3: make the message match the expected one by making the check in StaticPropSource::isMinimalRepresentation() throw a different exception that ::validateComponentInput() can catch, to allow this in turn to trigger the exceptions from SDCs themselves — thus replacing the hideous/confusing validation error linked above with the one we've been seeing all this time: the one from \Drupal\Core\Theme\Component\ComponentValidator::validateProps() — should be green

Thoughts? :)

Added a fourth (!) approach in MR 3537154-how-about-non-blank

I think that if invalid data is not flagged by calling validate then we have reduced trust in our systems.

The first approach I took was to add the NonBlank constraint to StringItemOverride on the value property. But then I realised we never call validate on the field item list we have inside StaticPropSource so that didn't do anything.

So then I added a validate method to PropSourceBase and had \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::validateComponentInput call that. But this blew up spectacularly because the field item list we have there has no entity parent and \Drupal\options\Plugin\Field\FieldType\ListItemBase::getSettableOptions assumes we do.

So then I backed all that out and instead injected the basic validator into GeneratedFieldExplicitInputUxComponentSourceBase and validated against a NonBlank constraint for our required props in ::validateComponentInput - which gets to the root of the issue - that json schema doesn't consider '' as invalid for a required items - whilst we do.

That got the test from @tedbow to pass.

Sorry for adding another approach, I'm just concerned about the number of workarounds we are doing here.

I closed my MR. I think @larowlan's or @wimleers is probably the way to go. I think I understand how @wimleers' https://git.drupalcode.org/project/experience_builder/-/merge_requests/1346 works and it does solve the current problem

@larowlan's https://git.drupalcode.org/project/experience_builder/-/merge_requests/1352 seems promising because it gets to root of the problem, we consider required to mean not blank. but right now there are test failing around validation so assume it makes a bunch of other validation fail.

Ah in #20 I had looked at the wrong MR and missed @wim leers's

That approach is fine with me, it is leaning on existing definitions of `isEmpty` in the field items inside the prop source, which is kind of what I was trying to achieve with the initial approaches of calling validate on that field item list in #20

I'll close my MR, sorry for the crossed wires.

Thanks, @larowlan! 😊

I'm very relieved that this simple follow-up MR addresses the data integrity pieces — whew! 😇

I do very much agree we should do what you proposed at #3529788-29: 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! 👍

@balintbrews is doing the cherry-picking to 0.x. This should pick cleanly.

I have tested the changes and confirmed that props text validation is working as expected. Users are now prevented from publishing when required text fields are left blank.

However, I observed the following issue: if a required text field is left blank and a publish attempt is made, the system correctly throws an error. But after entering a value into the text field and trying to publish again, the user is prompted to refresh the page with the message:

“Your latest change was not saved because the content was modified elsewhere since you loaded the page. Please refresh your browser to receive the latest changes and continue.”

Additionally, the publish tool displays this error:

“An item in the publish request did not match the expected format or value. Please refresh your page and try again.”

Refreshing the page resolves the issue, but this workflow could be improved so that the refresh message does not appear every time.

Thanks!

Reproduced:

Investigated that and:

1. the first message is coming from ui/src/features/editor/ConflictWarning.tsx, introduced in #3526907: For content entities add client support conflict detection via the `autoSaves' key
2. the second message is coming from \Drupal\experience_builder\Controller\UnmatchedItemInPublishRequest, introduced in #3520067: [Needs design] Confusing messages when publishing immediately after making changes: auto-save race condition

Digging deeper now.

I couldn't reproduce #31 + #32 every time, but almost every time.

It's just a race condition in the auto-save system. It's literally what both of the referenced issues were about. We'll just need to harden against that. Created a follow-up: #3537866: Auto-save conflict if an item is already selected for publishing and another change is made.

This is still ready for backporting. The data integrity issue that this fixed is still real.