-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[OptionsResolver] add method getKnownOptions() #9713
Conversation
Q | A |
---|---|
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #6458 |
License | MIT |
Doc PR | no |
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() |
There was a problem hiding this comment.
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
fixed description @stof , it was a miss copy paste 👶 |
@stof i think @bschussek preferred getOptions to getKnownOptions |
Looks weird to me to only add this one as a getter. ping @bschussek |
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. |
I don't think we can currently add this functionality, because we'd have to add #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 |
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. |
@fabpot should i proceed then creating a new interface? @bschussek thoughts? |
@fabpot If we add the methods to a new interface, then they are (technically) not accessible in the |
Closing in favor of #9754 |