-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Support enums in Choice constraints #43047
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
Conversation
e5a6566
to
1bb884d
Compare
Regarding CI:
|
Ignore it for now. That will be fixed in the future when running Psalm on PHP 8.1. And it won't impact the future PRs, as the CI job uses the existing codebase as the baseline. |
We could silence this error in psalm.xml though. |
src/Symfony/Component/Validator/Constraints/ChoiceValidator.php
Outdated
Show resolved
Hide resolved
If you perform a rebase on 5.4, Psalm should not complain about the enum classes anymore. |
d4f9553
to
62940fc
Compare
Thank you @derrabus - it works 🙂 |
Can you elaborate, when would I use this validator? Your example was: class CardDTO
{
#[Choice(choices: Suit::class)]
public string $suit;
} But my understanding of enums is that they're type-safe. Why would I prefer a string field over using the actual enum type like in your other example: class Card {
public Suit $suit;
} Alternatively: class Card {
#[Type(Suit::class)]
public $suit;
} |
@derrabus my guess is that this would be used on a string field in a DTO receiving user input (which cannot be the enum instance and can contain invalid data, which is the whole point of using the validator), for which you want to make sure that you can use |
We could argue hydrating raw payloads/userdata into objects generally, i believe. The alternative route to me is #40830 + #42502, thus hydrating valid state, or validating invalid hydrated state. From a DIY POV, this makes sense. (you need the constraint for raw payloads either way, if decoupled from Serializer). |
Yes, @stof summed it up well. If you try to unserialize an invalid value to the enum property directly, the serializer won't be happy and you'll end up with either a 500 or a 400 error. By pre-validating it through a DTO, you keep 1 source of truth about the valid values, and you can get a more accurate constraint validation error which will tell you the exact path where it failed. Besides, if you just type-hint your property as Here's another example, like altering a enum PostStatus: int {
case DRAFT = 0;
case PUBLISHED = 1;
case ARCHIVED = 2;
}
#[ApiResource(input: PostInput::class)]
class Post {
public Ulid $id;
public string $title;
public PostStatus $status;
}
class PostInput {
public string $title;
#[Choice(choices: PostStatus::class)]
public int $status;
} The following request: PUT /posts/01FEB4F1F374ZZWVD1XWHS2SEM
Content-Type: application/json
{"title": "foo", "status": 42} would get the following HTTP 422 unprocessable entity response: {
"@context": "\/api\/contexts\/ConstraintViolationList",
"@type": "ConstraintViolationList",
"hydra:title": "An error occurred",
"hydra:description": "status: The value you selected is not a valid choice.",
"violations": [
{
"propertyPath": "status",
"message": "The value you selected is not a valid choice.",
"code": "8e179f1b-97aa-4560-a02f-2a8b42e49df7"
}
]
} |
@ro0NL I didn't notice the existence of @lyrixx's #42502 - it looks indeed that we addressed similar issues :-) However, I think his approach is more like a global setting you would apply to your whole API, whereas mine targets specific properties usable in DTO contexts. Another use case, with the enum Feature: string {
case LIGHT_DETECTION = 'light';
case MOVEMENT_DETECTION = 'movement';
}
#[ApiResource(input: DeviceInput::class)]
class Device {
/**
* @var array<Feature>
*/
public array $features;
}
class DeviceInput {
/**
* @var array<string>
*/
#[Choice(choices: Feature::class, multiple: true)]
public array $features;
} |
AFAIK, what we pursue is #[DeserializeFromRequest(request, #[Assert\Collection(...)])
controller(Infra\Request $request, Domain\Data $domainObject): mixed {
try {
return $domainObject->dataOfInterest; // typed and serializable
} catch (ConstraintViolations $e) {
throw $e; // typed, serializable and translatable
}
} and be done. In that sense, to me the "controller" is the source of truth. Transforming validated payload is another interesting area, e.g. ID => entity |
src/Symfony/Component/Validator/Constraints/ChoiceValidator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Constraints/ChoiceValidator.php
Outdated
Show resolved
Hide resolved
That would've been my idea as well. This is a serialization/hydration issue. Storing the enum instance as a string defeats the purpose of enums. If we want to pursue this approach anyway, I'd suggest to introduce a new property (e.g. |
There's no such thing as an enum "instance" - my only options to refer to an enum's cases are Since an enum actually is a choice (and something more exhaustive / reliable than an array), I don't really get the point of not being able to use it in a choice constraint. I didn't think about a |
|
There's a misundertanding - the "instance" here is one of the available cases, not a list of all allowed cases (which is what we want for the |
Correct and each case is an instance of the enum. We can use PHP's type system to check this.
No, what you define as cases here are actually the scalar values that are used to back the enum. My understanding of enums is that the backed value exists for storage and transport serialization. Deserializing an object with an enum should yield an instance of the enum class and not the backed value. That's what I meant when I wrote that this is a serialization/hydration concern.
In other words: enums exists so we don't need to perform this kind of validation anymore. Don't get me wrong, I won't block this PR. All I'm saying is that I don't feel we need it. |
@derrabus IMHO validator constraints provide i18n friendly error messages, in APIs, forms, everywhere. In that sense, i'm waiting for a |
Enums currently address a single value - but it's quite easy to validate an array of values against an enum, if you want to keep 1 source of truth for allowed values (and benefit from static analysis when processing values). class DeviceInput {
/**
* @var array<string>
*/
#[Choice(choices: Feature::class, multiple: true)]
public array $features;
} |
Coming late on this, but I don't really understand the arguments here. I don't agree that this is enough when there is still a need for using the Unless we choose to deprecate the |
Kind of, yes.
You're trying to disprove an argument nobody makes. The If you read and understood all of this and still think you need this feature, please describe your use-case and we'll talk about it. |
I you agree with this why don't you accept that everyone wants to set the choices with enums 🤔, they are basically designed for such use case.
It looks like a poor alternative to me since the default message will tell that value X is not typed (enum) Y.
The use a case is simple: a string needs to be validated against a set of values. There is no hydration, no serialization, no form, just a string validated among other values validated with the validator. |
Enums are designed to be hydrated. They are not designed to work with the backing value all the time.
and that's not what PHP enums are. They are not strings but objects. |
Yes of course, the scope of this feature is the backed enum (string or int), which by design is always backed, and fits the needs to use iterable cases to define an immutable set of valid choices for string or int values to be validated. The enum can then also be safely hydrated in other layers if needed. |
So the recommended way for what used to be #[Assert\Choice(choices: ['foo','bar'], message: 'Choose either foo or bar')] ...is now?: #[Assert\Type(type: FooBarEnum::class, message: 'Choose either foo or bar'))] I wouldn't have expected that, so I'd like to add an enum example to https://symfony.com/doc/current/reference/constraints/Type.html and even a note about enums at https://symfony.com/doc/current/reference/constraints/Choice.html |
If you want to validate a string against a list of allowed strings, this is still the way to go. |
Enum has the advantage of single source of truth, IMO |
Personally, I've already used: new Choice([
'choices' => array_map(
static fn (MyEnum $myEnum): string => $myEnum->value,
MyEnum::cases(),
),
]) many times already and I'd prefer to be able to use the enum class directly. |
@fancyweb Do you actually hydrate the enum elsewhere, e.g. by calling |
Yes, but once the value has been validated. My personal use case is:
So before PHP 8.1: $content = json_decode($request->getContent(), true);
$this->myCustomValidatorService->validate($content, new Collection([
'foo' => new Choice([MyFakeEnum::FOO, MyFakeEnum::BAR]),
]));
$obj = new MyClass($content['foo']); // MyClass::$foo type is string What I expected on PHP 8.1: $content = json_decode($request->getContent(), true);
$this->myCustomValidatorService->validate($content, new Collection([
'foo' => new Choice(MyRealEnum::class),
]));
$obj = new MyClass(MyRealEnum::from($content['foo'])); // MyClass::$foo type is MyRealEnum The real code is obviously way more complicated than that, I summarized it.
I'm not saying the existing code is state-of-the-art or anything but it works and I won't personally re-architecture it just to be able to use Personally, I'm +0 on this PR, I understand arguments from both sides and I'm fine using the workaround. I guess it really depends on how the app is structured. |
What did this end up with? Will we get the ability to use |
@AlexeyKosov if you want to check that the property is one of the instances of the enum, you can already use |
So can we consider reopening such PR for 6.2 🙏 ? Or shall we lock this discussion as "won't fix"? |
This one has been rejected by several members of the core team. So it has no chance to pass for 6.2 IMO (we would have to all change our mind). Please read the discussion above for the arguments |
Adding this method to backed enum classes could be a nice way to go for ppl that really want to deal with backed values directly: public static function values(): array
{
return array_column(self::cases(), 'value');
} We could even suggest making this the default implementation provided by the engine. /cc @Crell |
For the record #47691 (reply in thread). Very not convenient, this PR should really be reconsidered. |
I also think we should re-consider this. While I agree that in most cases it would be cleaner to handle this in the serialization directly, I don't think Symfony should enforce this by not supporting enums in the validator. To me it is unexpected that I can define my choices with an array of constants but not with an enum. Symfony and the individual components should stay flexible and this feature request would just be a few lines of code. So it improves ease of use by just adding a tiny feature (even if not everybody agrees that this is the best approach). |
I'm fine with revisiting the topic, but please let's get this straight: The validator does support enums, via the Unlike other languages and unlike people had been building enums before PHP supported them, PHP makes a big difference between an enum and its backed value and we should do the same unless we want to cause even more confusion. If we want to implement this kind of validation:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Anyway, I'd like to lock this discussion. This PR usually pops up if people google for how to validate enums in Symfony and I'd like to keep it a helpful resource in that regard. New comments on this PR just add more noise and confusion. This PR was about validating if a string value is a valid backing value for an enum. In this comment, I summed up why I felt that the validator is the wrong place for that:
If you still want to validate strings this way, Nicolas has shared a snippet for a function that you can use as a callback for the Choice constraint:
Anyone who still wants to create a dedicated constraint for this kind of validation, please read this comment first. |
A new attempt to solve this issue has been made in #54226. |
This PR allows using PHP 8.1 (backed) enums in
Choice
constraints.Usage: