8000 [OptionsResolver] add method getKnownOptions() by cordoval · Pull Request #9713 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Conversation

cordoval
Copy link
Contributor
@cordoval cordoval commented Dec 6, 2013
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #6458
License MIT
Doc PR no

@stof
Copy link
Member
stof commented Dec 6, 2013

the description of the PR is really weird. This PR has nothing to do with Yaml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name it getKnownOptions

@cordoval
Copy link
Contributor Author
cordoval commented Dec 6, 2013

fixed description @stof , it was a miss copy paste 👶

@cordoval
Copy link
Contributor Author
cordoval commented Dec 6, 2013

@stof i think @bschussek preferred getOptions to getKnownOptions

@fabpot
Copy link
Member
fabpot commented Dec 17, 2013

Looks weird to me to only add this one as a getter. ping @bschussek

@piotrpasich
Copy link

Do you want to return knownOptions or array_keys($this->knownOptions) ? Which should be better?

I think getOptions is better than getKnownOptions because it keeps internal implementation away from interface.

@webmozart
Copy link
Contributor

I don't think we can currently add this functionality, because we'd have to add getKnownOptions() (and probably also getRequiredOptions()) to OptionsResolverInterface, which would break BC.

#9754 is an attempt to fix this issue in a different manner, but it also needs to change the interface (by adding a new parameter to resolve()). Not quite sure what to do. @fabpot?

@fabpot
Copy link
Member
fabpot commented Dec 17, 2013

adding new methods on an interface can be easy by just creating an additional interface that is implemented by the class. That keep BC and we would merge the two interfaces in 3.0. We've done that a lot in the past and it works pretty well.

@cordoval
Copy link
Contributor Author

@fabpot should i proceed then creating a new interface? @bschussek thoughts?

@webmozart
Copy link
Contributor

@fabpot If we add the methods to a new interface, then they are (technically) not accessible in the setDefaultOptions() method of FormTypeInterface, which is coded against OptionsResolverInterface. I don't think this makes sense.

@fabpot
Copy link
Member
fabpot commented Dec 27, 2013

Closing in favor of #9754

@fabpot fabpot closed this Dec 27, 2013
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.

5 participants

0