October: Client-side form validation can break setting model forms

[jimcottrell]

• OctoberCMS Build: 467
• PHP Version: 7.2

Description:

Not sure that client-side form validation was completely thought through, even with #5088. Any settings form that has been using the required flag for fields that might be hidden would be broken in 467.

Steps To Reproduce:

Create a placeholder settings model with the following form fields YAML:

tabs:
    fields:
        field1:
            type: checkbox
            label: Optional field on tab 1
            tab: Tab 1

        field2:
            label: Required Field
            type: text
            required: true
            tab: Tab 2

Without doing anything, attempt to save the settings. This silently fails, but you can see the following console error from the hidden field's attempted client-side validation check:

It seems like settings models should at least only use client-side validation if opting in to that behavior. And presumably the required form field config needs to be updated to mention that it affects validation and is no longer UI-only in 467+.

[LukeTowers]

@jimcottrell does changing the code in Backend\Classes\FormField->getAttributes() from

// Field is required, so add the "required" attribute
        if ($position === 'field' && $this->required && (!isset($result['required']) || $result['required'])) {
            $result['required'] = '';
        }

to

// Field is required, so add the "required" attribute
        if ($position === 'field' && $this->required && !$this->hidden && (!isset($result['required']) || $result['required'])) {
            $result['required'] = '';
        }

resolve the issue for you?

[jimcottrell]

It doesn't, no, as it's not a hidden field to October. It's just not visible at the moment of validation because its tab is inactive. That might be the right thing to do with fields marked with the hidden config though.

[LukeTowers]

Hmm, is there a way to catch that error and ignore it, or perhaps do a better job at making the field visible if necessary first?

[jimcottrell]

You'd probably have to fist use checkValidity instead of reportValidity and update visibility on any thrown invalid event before calling reportValidity on that element, possibly also disabling fields using trigger to show/hide so the browser doesn't validate them. As-is though, the client-side validation is not compatible with the full range of October form functionality.

[LukeTowers]

Yeah, looking into it further it looks like Drupal has been trying to solve this issue for the past 8 years (see https://www.drupal.org/project/drupal/issues/1797438#comment-13718769), so I don't think we're going to get any farther than they have. I'll go ahead with disabling it by default on the settings controller and the scaffolding, as well as documenting the issues with it. We can look at removing it from the other existing core controllers if it becomes a problem down the road, but we'll leave basic support for it in framework.js.

Ideally whenever @bennothommo gets time to finish #4322, that will provide a much better client experience overall than HTML5 validation anyways, so I'm fine not putting any more effort into making basic HTML5 validation work better with October.

[LukeTowers]

Also see https://www.drupal.org/project/field_group/issues/2969051, which is basically the same issue but in Drupal, and illustrates just how deep the whack a mole game can get to be when trying to overcome the browser limitations when it comes to the HTML5 form validation.

Thanks @jimcottrell for the detailed report! Fix will be available in Build 469 along with the Laravel 6 update.