Conversation
|
There were recent attempts to properly reflect functionality like this in interfaces. Now |
|
I'm not quite sure about the move to the trait as well, but as both 'validatePartial()` implementations were the same it seemed reasonable to me. |
|
You've got a point there. My first thought yesterday was because
Only in case a form is sent but not submitted, partial validation kicks in. Now delegated by the form to the fieldset. So let's take another look at this. To partially validate a fieldset, which is a We could easily support this using a recursive Let me know what you think :) |
|
I tried implementing the approach and if I understand it right, personally I like it a lot. Separating the concerns of validating and yielding through the form elements tree seems reasonable to me. What exactly are your concerns of adding such a yield function to the interface? |
jrauh01
force-pushed
the
fix/partially-validate-fieldset-elements
branch
from
e960f05 to
4e52f84
Compare
|
For one, extending the interface. But also that I already mentioned that What about |
|
Sounds even better and would avoid interface changes completely. |
jrauh01
force-pushed
the
fix/partially-validate-fieldset-elements
branch
2 times, most recently
from
b2a6bee to
dbeb0cb
Compare
| * @return $this | ||
| */ | ||
| public function validatePartial() | ||
| public function validatePartial(): static |
There was a problem hiding this comment.
Not sure actually that this belongs here. It's either already done by a dedicated PR or not possible due to high risk for a bc.
nilmerg
left a comment
nilmerg
left a comment
There was a problem hiding this comment.
Otherwise it looks fine I think.
jrauh01
force-pushed
the
fix/partially-validate-fieldset-elements
branch
from
dbeb0cb to
a858f34
Compare
|
Should I squash the commits before this gets merged? |
jrauh01
changed the title
FieldsetElement::validatePartial() to fix required errorsForm::yieldElements() to fix partial validation for FormElements
When a form is submitted via autosubmit, only partial validation runs. For fieldset elements, a single child having a value was enough to trigger full validation of the entire fieldset, causing required errors to appear for fields that had just been revealed and were still empty. Add a `validatePartial()` method to `FieldsetElement` that checks each child element individually for whether it has a value before validating it, and recurses into nested fieldsets. `Form::validatePartial()` is updated to detect fieldset elements and call `validatePartial()` on them.
When a form is submitted via autosubmit, it is only partially validated. For sub elements implementing the `FormElements` interface it was enough for one child to have a value to trigger full validation of the entire container, causing "field is required" errors on fields that had just appeared and were still empty. `validatePartial()` is declared to the interface to future implementors are required to provide it as well, and added to the trait to satisfy the declaration from the interface with a default implementation.
Instead of requiring `validatePartial()` in the `Contract\FormElements` interface (and thus implementing it in the `FormElements` trait), introduce a recursive `yieldElements()` generator that yields all sub form elements, delegating into nested `FormElements` containers. Compared to the previous approach, `validatePartial()` no longer needs to be part of `Contract\FormElements`. The earlier implementation checked `instanceof FormElements` to recurse manually. `yieldElements()` makes that recursion reusable and separates traversal from validation logic.
Partial validation is driven solely by `Form`, via `hasBeenSent()` and `hasBeenSubmitted()`. Exposing `yieldElements()` on the `FormElements` interface would force every implementor to provide a method that currently only makes sense in a form's validation context. Moving it to `Form` as a protected helper method keeps traversal an implementation detail of the form. Unlike the trait based approach, `FormElements` implementors are no longer required to satisfy a contract that has no relevance to them.
jrauh01
force-pushed
the
fix/partially-validate-fieldset-elements
branch
from
a858f34 to
45ebe6d
Compare
Describe the recursive behavior of the method explicit in its name.
Tests cover two cases: - A fieldset with mixed children: only the child that has a value should be validated, while the empty required sibling should produce no error. - A doubly nested fieldset: `validatePartial()` should recurse all the way down and validate the nested element.
lippserd
changed the title
Form::yieldElements() to fix partial validation for FormElementsForm::yieldElementsRecursive() to fix partial validation for FormElements
3 participants
When a form is submitted via autosubmit, only partial validation runs. For elements containing other form elements (
FormElementsimplementation), a single child having a value was enough to trigger full validation of the entire element, causing "field is required errors" to appear for fields that had just been revealed and were still empty.Because initiating validation is the concern of
Form, ayieldElementsRecursive()method is added there. It recursively yields all sub form elements and recurses into nestedFormElementscontainers. This enables the form to check each individual element and validate only if it has a value.fixes #190