Add FieldsetElement::validatePartial() to fix required errors#191
Add FieldsetElement::validatePartial() to fix required errors#191
FieldsetElement::validatePartial() to fix required errors#191Conversation
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.
|
There were recent attempts to properly reflect functionality like this in interfaces. Now |
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.
|
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? |
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.
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
from
8749c4e to
b2a6bee
Compare
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
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.
2 participants
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 toFieldsetElementthat 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 callvalidatePartial()on them.fix #190