Skip to content

[Validator] New feature: Choice validator / enum struct for choices #21356

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

Closed
wants to merge 4 commits into from

Conversation

TheCelavi
Copy link
Contributor

@TheCelavi TheCelavi commented Jan 20, 2017

Proposal for new feature for Choice validator: ability to specify class where class constants will be used as source of choices.

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR symfony/symfony-docs#7394

We (developers) tend to emulate a struct with PHP classes and constants, example:

 final class Gender {
     const MALE = 'male';
     const FEMALE = 'female';
     
     private function __construct() { /* noop */ }
 }

It is natural that we can state in choice validator which class is source for enum values for validation.

Although first go-to solution would be to add a static class Gender::getConstants(), it adds behaviour to enum struct, which should just enumerate possible values. Furthermore, it forces us to maintain that method with every addition/removal of enumerating value.

It seams trivial, however, proposal is based on previous experience, where maintenance of (in our example) Gender::getConstants() does get forgotten which breaks the apps that we maintain.

Lately, we started to use reflections to introspect class constants to get enumerating values and our developer experience has increased for better.

Therefore, the benefit of this feature is solely focused on increased developer experience.

@ogizanagi
Copy link
Contributor

ogizanagi commented Jan 20, 2017

This implementation looks very opinionated to me, thus I don't think it belongs to the Symfony Core asis (but that's only my opinion) :/

For instance, in https://github.com/Elao/PhpEnums, we chose to not use reflection to automatically guess enum values, because some constants can be used to combine values (in case of the FlaggedEnum for instance). (<-- but we do provide more opinionated features based on traits now, so everyone can use it the way they like, including autodiscovering enum values from public constants)
The validator shipped with the library uses the same validator as Symfony Choice's one, but with a different constraint class allowing to choose the method to use to select allowed values (using the callback option, or a list of allowed values).

@TheCelavi
Copy link
Contributor Author

@ogizanagi It is opinionated - no argue there. Our common practice is to use dedicated enum class for enumerations and we have noticed that it is common practice for others as well -> considering that there is a lack of native enum support.

In order to have enumerations in project, I don't like idea to pull down whole library, especially having in mind that flagged enums are edge case (i want banana, not gorilla holding a banana, in the forest). Especially I don't like idea of inheritance and implementation of enum interface with several methods -> when I only need an enumeration of several possible values. Just enumeration.

Now, having in mind that I have simple final class with enumerations only, naturally, I will benefit of this option in validation.

Wether will this be part of Symfony core is a question about common practice widespread between users: do they use enum classes at all, if they use them, do they use vendor libraries and inheritance or they create a simple class with enums only?

Answers on those question can provide you/parties in charge if this is a good idea or not.

@ogizanagi
Copy link
Contributor

ogizanagi commented Jan 20, 2017

We only created this package because we deal with enums the same way across projects a lot and having clear interfaces eases integrations. We don't always use all the bananas indeed 🍌

But to me, the simplest way to deal with enums is a class per needs, with static methods to get allowed values. Then, use the callback option from the Choice constraint.

Here are my 2 cents 😄

@linaori
Copy link
Contributor

linaori commented Jan 21, 2017

This feature would be nice to have in a custom package. While the name is incorrect, this package allows you to translate enum values to human friendly texts via enums that are built the same way: https://github.com/hostnet/entity-translation-bundle

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Jan 24, 2017
@greg0ire
Copy link
Contributor

greg0ire commented Mar 3, 2017

Shameless plug : https://github.com/greg0ire/enum

@javiereguiluz
Copy link
Member

My fear with this proposal is that we're trying to implement something that PHP doesn't support natively, so we may be overstepping here.

By the way, isn't an enum a choice with a predefined set of values? Can't we use the existing choice instead?

@greg0ire
Copy link
Contributor

greg0ire commented Jul 6, 2017

@javiereguiluz the issue is referencing the values from a non-form piece of code. People want to be able to define the set of values from outside the form, in their domain, and then reuse that set of values in the form.

@TheCelavi
Copy link
Contributor Author

@javiereguiluz this proposal is very opinionated and base on our usual coding practice.

We have noticed that, in majority of libraries that we use, similar practice is used as well -> final classes with private constructors and constants are used as enum data structures.

Also, we have noticed that we, and others as well, do a lot of copy/paste coding which can be prevented with this kind of implementation.

The question here is not wether this proposal is opinionated or not, or if it is supported natively by PHP or not, we know answers on those questions.

Question is here if this is going to add value to library and improve developer experience, and possible answers are:

  • Yes, this is common practice among developers, this will increase productivity and developer experience, it will be useful and used
  • No, this is not common practice among developers and this would be used by minority of developers, so it is not worth of maintaining it and supporting

Thats it. If you are going to dismiss it solely because it is opinionated or not supported natively by PHP - there is no need to argue there, this is opinionated, enums are not natively supported, you can do that right away!

However, if developer experience and usability are in game - than, it should be reconsidered from that point of view only.

@greg0ire Yes, we use that "enum" for entity(es), form(s), validator(s), translation sentences (twig) -> same single enum is one single "source of truth", that is our common practice.

@nicolas-grekas
Copy link
Member

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017
@TheCelavi
Copy link
Contributor Author

@nicolas-grekas In general this idea of this PR is solid and it will be merged when PR is updated?

@fabpot
Copy link
Member

fabpot commented Mar 31, 2018

I agree with the other commenters that this is very opinionated and I don't feel like this should be part of Symfony core as is. Thanks for proposing.

@fabpot fabpot closed this Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants