[5.3] Rework form validation

[wilsonge]

Summary of Changes

This is a relatively major rework on form & field validation. There are a variety of ideas here for allowing Models to implement exceptions going forwards - that means that Form no longer returns exception objects. In order to facilitate we've introduced the concept of a Constraint (very loosely inspired from the Symfony validation package). These tell the user what checks have been applied and provide helper methods for checking what's failed - along with the error string. The rule system is then modified to take advantage of the new constraints system

A nice side effect of constraints - is that it allows server side validation of form field properties not currently validated - such as the max field length.

In this PR I've migrated the SubformRule to the new RuleInterface (as it's a special case) and also the PasswordRule so that you can see the change. Note that I've left the other rules for now to allow easy testing to prove that old style Form Rules also work.

There are no practical b/c changes right now with the \Joomla\CMS\Form\Form::$setModernValidationResponse (and associated helper method), the intention will be in J6 to change this to enabled by default and in J7 remove the ability to swap back to the current Legacy system

The only b/c change here is the return type change of \Joomla\CMS\Form\Field::validate - but that is unavoidable to migrate to this new structure and is practically unlikely to affect people - I'm only aware of a very small number of users who currently override this method and all the cases I've found so far don't use the parent return value (they add in extra checks and we still support that method returning true & false in case)

Testing Instructions

Check form fields are correctly validated before and after this is merged. There shouldn't be any changes.

Link to documentations

Please select:

• Documentation link for docs.joomla.org:

• No documentation changes for docs.joomla.org needed

• Pull Request link for manual.joomla.org: To come when this is given a clean bill of health by people

• [] No documentation changes for manual.joomla.org needed

[brianteeman]

This should be a string specific to the field shouldnt it? So in this case it woujld be JLIB_FORM_VALIDATE_FIELD_FIELDLENGTH_INVALID

[wilsonge]

For now I left the errors as they are. Because most this validation is done client side in many cases (maybe API excepted?) end users shouldn't see this unless they are doing malicious things - so I decided to keep the errors very generic. Technically this is a new error (unlike the other comment below - so I'm happy either way)

[brianteeman]

By making it specific it would be easier (i think) to have specific error messages with advice on how to resolve the problem. See #41140 for a real world use case

[brianteeman]

This should be a string specific to the field shouldnt it? So in this case it woujld be JLIB_FORM_VALIDATE_FIELD_REQUIRED_INVALID

[wilsonge]

This is the same as the current value https://github.com/joomla/joomla-cms/blob/4.4-dev/libraries/src/Form/FormField.php#L1170-L1175

[Fedik]

Please help to understand the changes 😉
Currently we have:

sequenceDiagram
Controller->>Model: validate()
Model->>Form: process()
Form->>Form: validate()
Form->>Field: validate()
Field->>Rule: test()
Rule->>Field: returns <br>bool or exteprion
Field->>Form: returns <br>true or exception
Form->>Model: returns bool, <br>and keep exceptions in $errors
opt in case of "invalid form"
Model-->>Form: getErrors()
Note over Model, Form: store form errors <br>in $model->setError()
end
Model->>Controller: returns false <br>or data array
opt in case of "invalid form"
Controller-->>Model: getErrors()
Note over Controller,Model: set errors <br>in $app->enqueueMessage()
end

Loading

How it is planed to be with new approach in the end? (for people who not familiar with Symfony validation)

[wilsonge]

The contents of the interface are the only things in inspired by Symfony. The process in your diagram is the same - just some the return type hints chaining and better support for fields doing their own validation. What website you using to do that diagram (or is it just paint?)

[brianteeman]

better support for fields doing their own validation.

that's the bit I like

[Fedik]

What website you using to do that diagram (or is it just paint?)

It is this https://mermaid.js.org/syntax/sequenceDiagram.html, Harald added it to our Manual repo.
Github support it in MD markup also.

The process in your diagram is the same - just some the return type hints chaining and better support for fields doing their own validation

What currently I do not like, that we put all errors in to $app->enqueueMessage() in the end.
And no way to get an error per field, so it not possible to render validation error within the field.

In one of my component I have custom form class that store errors per field in $form->errors[$group.$fieldName] = $exception;
Later one, while the field rendering I can do kind of $form->getError($fieldName, $group), and render this error within the field.

Would be nice if new approach allows to do it in the core (I mean getting error per field).
(I did not look your code in details for now, maybe missed it.)

[brianteeman]

it not possible to render validation error within the field.

Being able to render the error with the field would be a huge accessibility gain

[dgrammatiko]

@wilsonge I have also a request (is it still xmas? 😅). So, client side Native HTML validation has some quirks with the one that stands out in the case of J: server side rendered form elements that HAVE a value the value is interpreted as VALID! I guess you understand what I will ask: empty the value of any invalid form elements in the data object. (Then a thin layer ontop of the native element.checkValidity(), etc can replace the archaic validate.js)

[wilsonge]

I'm not touching server side validation in this PR - sorry :D it's already a big enough PR as it is

[wilsonge]

Would be nice if new approach allows to do it in the core (I mean getting error per field).
(I did not look your code in details for now, maybe missed it.)

Yes it does (kinda) because you have access to the full constraint object and have the method to see which of them failed validation.

[wilsonge]

Hopefully this looks about right:

sequenceDiagram
Controller->>Model: validate()
Model->>Form: process()
Form->>Form: validate()
Form->>Field: validate()
Field->>Rule: test()
Rule->>Field: returns <br> ConstraintInterface[] <br>bool or exception (supported for b/c until J7)
Field->>Form: returns <br>FieldValidationResponse (which contains the constraints above) <br>true or exception (supported for b/c until J7)
Form->>Model: returns<br>FormValidationResponse (which contains all the fields above) <br> bool, <br>and keep exceptions in $errors
opt in case of "invalid form"
Model-->>Form: <br> no longer required - Model now uses the information contained in the FormValidationResponse <br> getErrors()
Note over Model, Form: store form errors <br>in $model->setError()
end
Model->>Controller: <br> to be determined outside the scope of this PR but probably throws some sort of new Exception (can still be the current way where it returns false <br>or data array)
opt in case of "invalid form"
Controller-->>Model: getErrors()
Note over Controller,Model: set errors <br>in $app->enqueueMessage()
end

Loading

[dgrammatiko]

I'm not touching server side validation in this PR

Did you mean client side? Confused

[wilsonge]

Yes I mean client side. This is what happens when I'm up at 6 and still haven't had coffee nearly 3 hours later :)

[Fedik]

This will need to have a group also, kind of

sprintf(
  'Field %s is valid', 
  $field->group ? ($field->group . '.' . $field->fieldname) : $field->fieldname
);

[Fedik]

Use $this->fieldname instead of $this->name,
$this->name returns whole name attribute jform[foo][bar]
$this->fieldname returns only name bar

[wilsonge]

I considered doing it that way - but this way is more consistent with how the rest of the JForm methods work.

[Fedik]

Please check again, I think you missing something.

Any JForm method with $name parameter actualy referers to name attribute of the field (not input name),
Example for <field name="bar" /> it will be bar.
However the $field->name (property of JField) is a input name jform[bar], wich is different thing.

Also input name already contain a group, and so having both $field->name and $field->group does not make sense in your code.

For

<fields name="foo">
  <field name="bar" />
</fields>

$field->name will be input name jform[foo][bar] (consist with form control, field group, and field name);
$field->group will be field group foo;
$field->fieldname will be field name bar;

[Fedik]

you did not tested your code, did you? 😏

Hopefully this looks about right

Thanks for the visualisation

Yes it does (kinda) because you have access to the full constraint object and have the method to see which of them failed validation.

hmhm, I see, I do not know how to say it, but it will not really work 😉 (in current stage)

As you know, Joomla controller process for content editing is:
layout=edit => task=article.save => redirect => layout=edit

And when controller doing redirect then validation result is lost.
To keep track of validation result we then need to have FormValidationResponse serialisable, wich we can pick after the redirect and somehow set it back to the form instance, so the code can check the field validity during FormField::renderField() (not in the scope of this PR, but the direction).

Because you placing FormField instance in to ConstraintInterface, we can forget about serialisation.
Addittionaly to FormValidationResponse::getInvalidFields() it will be much usefull to have something like FormValidationResponse::isFieldValid($name, $group)

UPD, well it probably also can be some new interface/class/implementation, that allows to exctract FormValidationResponse result and serialise.

[wilsonge]

I thought I had - but i was redirecting my virtualhost to the wrong place :D :D which helpfully allowed it to work really well!!

But i'm not super sure what your issue is though - after very minor fixes and code style I've got a test of adding $form->setModernValidationResponse(true); into Joomla\CMS\MVC\Model\FormBehaviorTrait::loadForm after we load the form. Then in FormModel I've added this in as extra:

        if ($return instanceof FormValidationResponse) {
            if ($return->isValid()){
                throw new \LogicException('Should only be received here if this failed!');
            }

            foreach ($return->getInvalidFields() as $invalidField) {
                $invalidFieldObject = $return->getField($invalidField['name'], $invalidField['group']);

                foreach ($invalidFieldObject->getInvalidConstraints() as $invalidConstraint) {
                    $invalidConstraintObject = $invalidFieldObject->getConstraint($invalidConstraint);

                    $this->setError($invalidConstraintObject->getErrorMessage());
                }
            }

            return false;
        }

and basic form validation stuff seems to work to me. I've forced fields to be invalid and it shows correct errors (both editing a select fields value and also forcing the length of a text field).

I mean my long term plan is to throw an exception in the model rather than the code above - but i haven't got round to working on that part yet - and I think this is good enough standalone to justify.

[HLeithner]

This pull request has been automatically rebased to 5.2-dev.

[HLeithner]

This pull request has been automatically rebased to 5.3-dev.