Initial work on stateless validation [RFC]#183
Conversation
Adds a `validate()` method to the input filter and input types that will each return a subtype of `ValidationResultInterface`. The result carries error messages, the un-filtered input and the filtered values in success and failure conditions. If this is implemented alongside the existing `isValid()` stateful validation process, it can provide a path for input filter to become more-or-less stateless. One advantage is that translation can be implemented externally by passing the result objects into a translation tool. The way that validation of missing/empty values is handled here is by inserting an obvious message into the error stack for the relevant input as opposed to mutating the validator chain. In future, we will be able to drop this as users move towards explicitly adding a `NotEmpty` validator to their specifications, in turn, meaning we can drop the complexity of `allow_empty`, `required`, `continue_if_empty` option shenanigans. Still to do: - Implement validation groups - More thorough tests - Implementation in collections to be checked Signed-off-by: George Steel <george@net-glue.co.uk>
|
@Slamdunk - I'd like to hear what you have to say about this WRT |
Slamdunk
left a comment
There was a problem hiding this comment.
stateless validation
My goodness yes please 😍
I am unable to do a proper analysis of the code, but I'm all in favour of this PR, also deprecating all the stateful code in v3 and doing the same in the upcoming next major of laminas-form
| * @param iterable<array-key, mixed>|null $data | ||
| * @param array<array-key, mixed>|null $context | ||
| */ | ||
| public function validate(iterable|null $data, array|null $context = null): InputFilterValidationResult; |
There was a problem hiding this comment.
| public function validate(iterable|null $data, array|null $context = null): InputFilterValidationResult; | |
| public function validate(iterable $data, array|null $context = null): InputFilterValidationResult; |
Can you delegate to the user the typecast to iterable?
There was a problem hiding this comment.
Yes - probably. This was mainly about sticking to the signature of setData() IIRC, but there's no reason to do that. I'd also say that $context could be array $context = [] and we drop the null union.
I was about to implement the following variation: --- a/src/InputFilterInterface.php
+++ b/src/InputFilterInterface.php
@@ -92,8 +92,9 @@ interface InputFilterInterface extends Countable
*
* @param iterable<array-key, mixed>|null $data
* @param array<array-key, mixed>|null $context
+ * @param array<array-key, mixed>|null $validationGroup
*/
- public function validate(iterable|null $data, array|null $context = null): InputFilterValidationResult;
+ public function validate(iterable|null $data, array|null $context = null, array|null $validationGroup = null): InputFilterValidationResult;And even though it's easy to get it working, it's a mess to convey the correct usage to the user, because the presence of We can let the user continue consuming Instead, what about... dropping everything related to values in the new major? interface InputFilterInterface
{
- public const VALIDATE_ALL = 'INPUT_FILTER_ALL';
-
public function add(InputInterface|InputFilterInterface|array $input, int|string|null $name = null): static;
public function get(int|string $name): InputInterface|InputFilterInterface;
public function has(int|string $name): bool;
public function remove(int|string $name): static;
-
- public function setData(iterable|null $data): static;
-
- public function isValid(array|null $context = null): bool;
-
- public function setValidationGroup(int|string|array $name): static;
-
- public function getInvalidInput(): array;
-
- public function getValidInput(): array;
-
- public function getValue(int|string $name): mixed;
-
- public function getValues(): array;
-
- public function getRawValue(int|string $name): mixed;
-
- public function getRawValues(): array;
-
- public function getMessages(): ErrorMessages;
+
+ public function validate(iterable|null $data, array|null $context = null, array|null $validationGroup = null): InputFilterValidationResult;
}This approach would ease refactoring No deprecation is possible in this scenario: only a well crafted upgrade guide will be provided, because having a new MINOR with both paths available would be a nightmare. WDYT? |
|
Getting rid of all that stuff is the ultimate goal, so I'm all in. If we do it now, it's less work in the long run at the expense of potentially bigger refactorings for users. Given the myriad BC breaks already present, we may as well bite the bullet!! For others following along, what this looks like for users is instead of: $inputFilter->setData($someInput);
if (! $inputFilter->isValid()) {
// Present Failure
$errors = $inputFilter->getMessages();
}
$data = $inputFilter->getValues();
// Do something with data
$inputFilter->isValid($otherData); // Oh no, all/some state has been reset - I no longer have access to the previous validation resultsThey will have: $result = $inputFilter->validate($someInput);
if (! $result->valid()) {
// Present Failure
$errors = $result->getMessages();
}
$data = $result->value();
// Do something with data
$result2 = $inputFilter->validate($otherInput); // Woo hoo, $result is unaffected by subsequent validationSomething else that is missing here is the template Ideally |
Could we mitigate this with a few rules for Rector? In other words, provide assistance for migration? |
Maybe? I think that's a question for @samsonasik because I'm not very knowledgable about rector beyond the basics |
|
@gsteel if there is before vs after code/docblock transformation example for it, I may can create rule for it |
|
Oh my days. Yes, this is undoubtedly cleaner. But without an automated upgrade path one hell of a lot of work for existing users. Validation groups are an evil: too many times I’ve found them leaving out the CSRF. I’d be very happy to see those go - so long as stuff can still be removed from the form before validation - no matter the work involved my side. |
This has been in my mind too for years, I didn't bring it up so far because there are already a lot of BC breaks involved, but yeah validation groups are bad imho. From https://docs.laminas.dev/laminas-form/v3/collections/#validation-groups-for-fieldsets-and-collection the reason behind them is the following:
This doesn't make sense to me: I've always had many different But the main reason validation groups are evil is that they allow brittle, insecure code as the group passed can be dynamic, while with hardcoded classes the developer is forced to code the exact expected validation workflow. I vote to drop them altogether too 🙋 |
|
I'm on board with dropping validation groups. This would prevent bloating the new result-type returned by In all the years of using I also agree with all the sentiment about brittle insecure code. It may be worth documenting short-cuts for code reusability, for example, I create small invokable classes for defining inputs and call these from /** @psalm-import-type InputSpecification from InputFilterInterface */
final readonly class DomainNameInputSpec
{
/** @return InputSpecification */
public function __invoke(bool $required, string|null $emptyError = null): array
{
$filters = [
['name' => StringTrim::class],
['name' => StringToLower::class],
['name' => ToNull::class],
];
$validators = [];
if ($required) {
$validators[] = [
'name' => NotEmpty::class,
'break_chain_on_failure' => true,
'options' => [
'messages' => [NotEmpty::IS_EMPTY => $emptyError ?? 'Please enter a domain name'],
],
];
}
$validators[] = [
'name' => Hostname::class,
'break_chain_on_failure' => true,
'options' => [
'message' => 'Please provide a valid domain name',
'allow' => Hostname::ALLOW_DNS,
],
];
$validators[] = [
'name' => HostWithPublicIPv4Address::class,
'break_chain_on_failure' => true,
'options' => [
'message' => 'This domain name is not reachable on the Internet',
],
];
return [
'required' => $required,
'allow_empty' => ! $required,
'continue_if_empty' => false,
'validators' => $validators,
'filters' => $filters,
];
}
}
final class SomeInputFilter extends InputFilter
{
public function init(): void
{
$this->add(
(new DomainNameInputSpec())(true),
'someInputName',
);
}
}These specifications encapsulate common configurations, affording re-use without sacrificing flexibility. I'd be interested to hear other approaches that could be documented for v3 as examples for users |
Adds a
validate()method to the input filter and input types that will each return a subtype ofValidationResultInterface.The result carries error messages, the un-filtered input and the filtered values in success and failure conditions.
If this is implemented alongside the existing
isValid()stateful validation process, it can provide a path for input filter to become more-or-less stateless.One advantage is that translation can be implemented externally by passing the result objects into a translation tool.
The way that validation of missing/empty values is handled here is by inserting an obvious message into the error stack for the relevant input as opposed to mutating the validator chain. In future, we will be able to drop this as users move towards explicitly adding a
NotEmptyvalidator to their specifications, in turn, meaning we can drop the complexity ofallow_empty,required,continue_if_emptyoption shenanigans.Still to do: