UI change in 3529788 allows publishing components with empty required fields

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 ✨ Add a Video prop type to the Code Component editor Active

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 🐛 Hero component failed to render after removing text Active - 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` 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` 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` 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 🐛 Hero component failed to render after removing text Active (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 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.

tedbow → closed merge request !1334

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.

larowlan → closed merge request !1352

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, minLength: 0` props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases → ! 👍

wim leers → closed merge request !1342

@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 📌 [PP-1] Enforce conflict enforcement outside of tests and e2e tests Postponed
2. the second message is coming from \Drupal\experience_builder\Controller\UnmatchedItemInPublishRequest, introduced in 🐛 Messages in \Drupal\experience_builder\Controller\ErrorCodesEnum are not helpful Active

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: 📌 Auto-save conflict upon populating an empty required `type: string` SDC prop and immediately publishing Active .

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