-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New PSR for standardizing Validator (ValidatorInterface) #1337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@Crell @KorvinSzanto I guess you would want to at least look at it |
Testability and integration with dependency injection containers. | ||
Consistent error format for user feedback, translation, or error mapping. | ||
Clean separation of simple (bool) and extended (structured, multi-error) validation. | ||
This pattern avoids having to deeply couple code to a specific validator ecosystem (as in Symfony) and dramatically reduces refactoring when switching stacks or updating validation strategies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just about specific applications switching stacks. It's about me being able to use a validation routine in a stand-alone library and not care if it's going to be used with Symfony, Slim, or Laravel. Eg, in Serde I can just tell people "if you want fancier validation, go use PSR-Whatever and leave me alone." (Or integrate a PSR-Whatever bridge into Serde if I feel like it.)
|
||
Every violation contains: | ||
|
||
- Machine-readable code (for programmatic matching, i18n, etc.). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree here. The class name is the machine-readable code. We don't need more opaque inconsistent strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Crell class name doesn't work well for exposing it in APIs i.e. JavaScript or Android/iOS clients should not be dealing with PHP fully qualified class names.
|
||
### 3.5. Swappability & DI | ||
|
||
Core purpose — to allow hot-swapping validator engines and compositions via DI or configuration, zero code refactor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
### 3.6. Scope | ||
|
||
Only single-value validation in scope; not object/collection/nested. | ||
Contextual validation may be layered by implementors using extensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think through what compound validation would look like. That should be included.
The final implementations MAY decorate the objects with more | ||
functionality than the one proposed but they MUST implement the indicated | ||
interfaces/functionality first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is unnecessary.
* | ||
* @return ValidatorResponseInterface | ||
*/ | ||
public function validate(mixed $value): ValidatorResponseInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the validation passed, we don't need detailed information. We can likely short-circuit that to something like true|ValidatorErrorInterface
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Crell that would create more code handling it. Having a single return value works better in this case.
* | ||
* @return bool | ||
*/ | ||
public function isValid(): bool; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a property now, not a method. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are zero benefits making it a property. Just drawbacks.
Ie. If it is a property you are forcing the implementation to be a value object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those drawbacks should be gone with PHP 8.4 though. Not sure if we want to target PHP 8.4 for this standard just to gain access to hooked properties. That would as of today be rather strict and hurt the adoption of the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there are still no benefits... =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- No, it does not force it to be a value object. It just changes the spelling to be simpler and more concise.
- The benefit is conceptual integrity. See the talk I gave at PHPTek this year on the new world properties open up for us.
- We should target 8.5, frankly. This wouldn't be ready before 8.5 is released anyway, and we've learned our lesson in the past that targeting an older-than-latest release always backfires and just makes more work for all involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should target 8.5, frankly.
For Symfony this would mean that we cannot easily implement the standard before version 9 which will be released in November 2027. That's quite a stretch.
* Thrown ONLY if validator is misconfigured or cannot process request. | ||
* MUST NOT be thrown if $value was actually tested, no matter the result. | ||
*/ | ||
class ValidatorException extends \InvalidArgumentException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe FIG convention is to us an interface here to mark exceptions from this package, not a base class.
* | ||
* @return string | ||
*/ | ||
public function getMessage(): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be a property.
* | ||
* @return string | ||
*/ | ||
public function getCode(): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As specified, this is extraneous and unhelpful.
As I have said many times, get the WG together first, design details second. 😄 |
Also: The current proposal doesn't seem to go far enough. How do I link the validator to metadata? Would, for instance, an attribute implement this interface? If so, what does it do if it needs a service to do its validation? (Eg, "does this record exist in the db?") Then the validation metdata (attribute) and the validation logic (a service) need to be separate. We don't have to mandate how that is handled, but we do need to ensure that it can be handled in a clean and portable fashion. |
Hello and thank you for working on this proposal. I believe and common interface for validators can be useful, especially for interoperability with libraries and even external systems like JS frontends. A few remarks from my side. I'll might add some more later when I have more time. Source of violationWhy I validate a complex object, a violation with a human-readable message is often not enough. Imagine, the object represents a form submitted by the user. I want to validate the whole object and receive a list of violations. If the form is invalid, I want to render the message next to the field with the invalid input. In order to facilitate that, I need a machine readable representation of the property path. This is how Symfony does it. I'm not saying you should copy the way Symfony does it, but please take that use-case into consideration. TranslationRegarding the error message, you're documenting it as…
This basically means that the validator has to be aware of the target locale when conducting the validation. This also means that I can only target a single locale which I have to determine before calling the validator. It would be great if the violation object would be translatable in some way. Unfortunately, we don't have a PSR standard for translators or translatable objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I see it an interesting thing to standardize. Here's what we have in Yii3 about the topic:
|
||
To satisfy both lightweight and complex needs, two interfaces are provided: | ||
|
||
- `SimpleValidatorInterface` — exposes only `isValid(mixed $value): bool`. Suitable for basic needs (is it a valid phone? email? int in range?...) and maximizing performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that running a single validation rule against the value? If so, I'd name it differently... like RuleInterface
or something.
To satisfy both lightweight and complex needs, two interfaces are provided: | ||
|
||
- `SimpleValidatorInterface` — exposes only `isValid(mixed $value): bool`. Suitable for basic needs (is it a valid phone? email? int in range?...) and maximizing performance. | ||
- `ExtendedValidatorInterface` — exposes `validate(mixed $value): ValidatorResponseInterface`, for structured results (validation status, errors, error codes/messages/etc.). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `ExtendedValidatorInterface` — exposes `validate(mixed $value): ValidatorResponseInterface`, for structured results (validation status, errors, error codes/messages/etc.). | |
- `ValidatorInterface` — exposes `validate(mixed $value): ValidatorResponseInterface`, for structured results (validation status, errors, error codes/messages/etc.). |
for the reason in https://github.com/php-fig/fig-standards/pull/1337/files#r2213153862
|
||
Every violation contains: | ||
|
||
- Machine-readable code (for programmatic matching, i18n, etc.). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Crell class name doesn't work well for exposing it in APIs i.e. JavaScript or Android/iOS clients should not be dealing with PHP fully qualified class names.
|
||
### 3.6. Scope | ||
|
||
Only single-value validation in scope; not object/collection/nested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd ensure that these fall into the given interfaces well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in reality, single value validation without context is not enough.
* | ||
* @return ValidatorResponseInterface | ||
*/ | ||
public function validate(mixed $value): ValidatorResponseInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually there's a context that matters, such as in the case when you need a password to be repeated exactly.
* MUST return true if $value passes. | ||
* MUST return false if $value fails. | ||
* MUST NOT throw exceptions if $value fails. | ||
* SHOULD throw ValidatorException if the Validator itself could not tell if the value passes or not (e.g. Validator misconfigured) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use generic \RuntimeException
for this case?
* | ||
* @return ValidatorResponseInterface | ||
*/ | ||
public function validate(mixed $value): ValidatorResponseInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Crell that would create more code handling it. Having a single return value works better in this case.
/** | ||
* A single validation violation. | ||
*/ | ||
interface ViolationInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For intl, there is a need for:
/**
* Returns parameters used for {@see $message} translation.
*
* @return array A mapping between parameter names and values.
*/
public function getParameters(): array
@samdark Please don't use "Reviews". They break threading badly. 😄 Having a single Re using the class name as the ID: The ID is an opaque string currently. It wouldn't be useful to some other system anyway. Honestly I'm not even sure what purpose it serves to exist at all, so unless that can be demonstrated I would object to having it. (Not something that has to be resolved before a WG forms, of course. The WG vote should likely include a lot fewer details than what is listed here.) |
Here's a proposal of new PSR for standardizing Validators to one interface
Continuation of long-lasting discussions on official Discord channel
Goal
To unite
earth and watermany separated validator libraries under one interface so the switching and DI would be easier, protect from potential vendor-locks and reduce coupling.