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

[DX] [Form] Use constants for form options #21727

Closed
pimolo opened this Issue Feb 23, 2017 · 9 comments

Comments

Projects
None yet
9 participants
@pimolo

pimolo commented Feb 23, 2017

Q A
Bug report? no
Feature request? no
BC Break report? no
RFC? yes
Symfony version 2.7

Currently

In Symfony Forms, the form type options keys are defined as strings, like 'data_class', 'required', 'choices', etc. (depending of the type of course).

Proposal

Let's defining a constant for each one of these options!

A few examples:

  • 'data_class' -> FormOptions::DATA_CLASS
  • 'required' -> FormOptions::REQUIRED
  • ...

A benefit could be making sure you don't make typos when you write the options in a field/form.
We could also consider defining these constants directly in the form types (not like in my example above which uses a dedicated class), that could give more consistency to options in my opinion.
Example : ChoiceType::CHOICES, FormType::REQUIRED...

Note that it won't create any BC break.

Thoughts? Pros / cons?

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Feb 23, 2017

Member

A benefit could be making sure you don't make typos when you write the options in a field/form.

There is already a validation. Not sure it worth it.

Member

lyrixx commented Feb 23, 2017

A benefit could be making sure you don't make typos when you write the options in a field/form.

There is already a validation. Not sure it worth it.

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Feb 23, 2017

Contributor

This means that I have a few list of constants spread around my application and vendor packages. Either that, or I have a partial string, partial constant list. I don't like this idea.

Contributor

iltar commented Feb 23, 2017

This means that I have a few list of constants spread around my application and vendor packages. Either that, or I have a partial string, partial constant list. I don't like this idea.

@felipsmartins

This comment has been minimized.

Show comment
Hide comment
@felipsmartins

felipsmartins Feb 23, 2017

I like this idea. However, even though you made a typo while writing any form option OptionsResolver validator will throw an exception (AFAIK).
Of course, your proposal is more comprehensive.

felipsmartins commented Feb 23, 2017

I like this idea. However, even though you made a typo while writing any form option OptionsResolver validator will throw an exception (AFAIK).
Of course, your proposal is more comprehensive.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Feb 23, 2017

Contributor

Some IDE are able to autocomplete these options (e.g. PhpStorm) and otherwise the OptionsResolver component validates whether each of them is defined or not, so not worth it (IMHO).

Contributor

yceruto commented Feb 23, 2017

Some IDE are able to autocomplete these options (e.g. PhpStorm) and otherwise the OptionsResolver component validates whether each of them is defined or not, so not worth it (IMHO).

@paulandrieux

This comment has been minimized.

Show comment
Hide comment
@paulandrieux

paulandrieux Feb 24, 2017

Contributor

The mix strings/constants could be a lack of consistency.
However, I think it would be a great benefit to use autocompletion from a ChoiceType to discover all the options available through the inheritances.

Contributor

paulandrieux commented Feb 24, 2017

The mix strings/constants could be a lack of consistency.
However, I think it would be a great benefit to use autocompletion from a ChoiceType to discover all the options available through the inheritances.

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Feb 24, 2017

Member

@paulandrieux You will find only some options not all. First because you can define options from any form type, form extension. Then because as the proposal state:

We could also consider defining these constants directly in the form types

Member

lyrixx commented Feb 24, 2017

@paulandrieux You will find only some options not all. First because you can define options from any form type, form extension. Then because as the proposal state:

We could also consider defining these constants directly in the form types

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Feb 25, 2017

Member

use autocompletion from a ChoiceType to discover all the options available through the inheritances

Impossible, because types inheritance has nothing to do with PHP inheritance.
The FormType itself is inheriting some options and this time through PHP inheritance by extending BaseType. However, as said @lyrixx, options can be defined in any class (i.e. child type calling getParent() or extension calling getExtendedType()) this argument does not suit.

There are too many classes defining options, using only one like FormOptionsas stated in the proposal is not even possible is the core or it wouldn't make any sense, some options are defined in the Doctrine bridge (in both abstract and concrete types).

Thanks for your proposal @pimolo but I'm 👎 on this, also because I agree with arguments given above considering the OptionResolver validation and IDE plug-ins autocompletion.

Member

HeahDude commented Feb 25, 2017

use autocompletion from a ChoiceType to discover all the options available through the inheritances

Impossible, because types inheritance has nothing to do with PHP inheritance.
The FormType itself is inheriting some options and this time through PHP inheritance by extending BaseType. However, as said @lyrixx, options can be defined in any class (i.e. child type calling getParent() or extension calling getExtendedType()) this argument does not suit.

There are too many classes defining options, using only one like FormOptionsas stated in the proposal is not even possible is the core or it wouldn't make any sense, some options are defined in the Doctrine bridge (in both abstract and concrete types).

Thanks for your proposal @pimolo but I'm 👎 on this, also because I agree with arguments given above considering the OptionResolver validation and IDE plug-ins autocompletion.

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Feb 26, 2017

Contributor

👎 The OptionsResolver already errors when unsupported options are provided and gives suggestions which option you meant instead. Considering the flexibility of Form types this will never work as expected.

Also the options are not that hard to remember (read the manual a few time, use it a few times and it becomes natural), using constants for HTTP status codes is usable because not everyone remembers what 401 means, using constants for flags instead of having to remember bit positions (which can be actually change at any time) is is good use-case.

But for this situation it feels like heavy overkill, also FormOptions::DATA_CLASS is much longer to type then 'data_class' and will not encourage anyone to actually use them.

Contributor

sstok commented Feb 26, 2017

👎 The OptionsResolver already errors when unsupported options are provided and gives suggestions which option you meant instead. Considering the flexibility of Form types this will never work as expected.

Also the options are not that hard to remember (read the manual a few time, use it a few times and it becomes natural), using constants for HTTP status codes is usable because not everyone remembers what 401 means, using constants for flags instead of having to remember bit positions (which can be actually change at any time) is is good use-case.

But for this situation it feels like heavy overkill, also FormOptions::DATA_CLASS is much longer to type then 'data_class' and will not encourage anyone to actually use them.

@pimolo

This comment has been minimized.

Show comment
Hide comment
@pimolo

pimolo Feb 27, 2017

Clearely there was a lot of points I didn't have in mind.
As this change would break consistency and DX, I'm going to close this RFC.

Thank you all for your feedback! 😘

pimolo commented Feb 27, 2017

Clearely there was a lot of points I didn't have in mind.
As this change would break consistency and DX, I'm going to close this RFC.

Thank you all for your feedback! 😘

@pimolo pimolo closed this Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment