-
Notifications
You must be signed in to change notification settings - Fork 7
removed the locale as mandatory parameter from persist #30
Conversation
f8f332e
to
a6ce01e
Compare
a6ce01e
to
0140b15
Compare
@@ -19,14 +21,14 @@ | |||
/** | |||
* This array is used as key value storage for the options. | |||
* | |||
* @var array | |||
* @var OptionsResolver |
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.
This is not an options resolver, its an array iirc.
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.
The options resolver only "resolves" an array of options to according to a configuration. The options themselves are always an array.
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.
It's not, on these options methods like setDefault
are called.
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.
Yeah, your right. In this case I would suggest to change the variable to "resolver" and remove the default value. "options" is just a misleading variable name.
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've removed the default value, but I am not sure if it is a good idea to change the name of the variable and method, since it feels strange in some places. E.g. if you call getResolver
just to get the options in a subscriber, and the fact that there is also already a getOption
method, which should not be renamed. Having two different method names here for more or less the same thing (only the count differs) feels quite strange...
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.
If I understand your point: you cannot determine the options from the OptionsResolver, it is there to be configured by the subscribers, it doesn't contain any option data, so you can't "get the options" from the resolver.
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.
You can, exactly the same way it is done in this class' getOption
method. The OptionsResolver
implements \ArrayAccess
.
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.
ok I see. It kinda sucks, but OK lets keep the name.
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.
Now I think I remember having this dilema when I created the class
0140b15
to
442b0db
Compare
removed the locale as mandatory parameter from persist
This PR removes the locale as mandatory parameter, because sometimes you only want to persist changes that are not locale specific.
tasks:
informations: