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

Skip to content

[OptionsResolver] add method getKnownOptions() #9713

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

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

@@ -237,6 +237,14 @@ public function resolve(array $options = array())
}

/**
* {@inheritdoc}
*/
public function getOptions()
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