Skip to content
This repository was archived by the owner on Sep 23, 2020. It is now read-only.

Conversation

danrot
Copy link
Contributor

@danrot danrot commented Aug 12, 2015

This PR removes the locale as mandatory parameter, because sometimes you only want to persist changes that are not locale specific.

tasks:

  • test coverage
  • gather feedback for my changes

informations:

q a
Fixed tickets fixes #16
BC breaks none
Documentation PR none

@danrot danrot force-pushed the bugfix/persist-without-locale branch from f8f332e to a6ce01e Compare August 12, 2015 08:50
@danrot danrot mentioned this pull request Aug 12, 2015
2 tasks
@danrot danrot force-pushed the bugfix/persist-without-locale branch from a6ce01e to 0140b15 Compare August 12, 2015 15:31
@@ -19,14 +21,14 @@
/**
* This array is used as key value storage for the options.
*
* @var array
* @var OptionsResolver
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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...

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

@danrot danrot force-pushed the bugfix/persist-without-locale branch from 0140b15 to 442b0db Compare August 14, 2015 05:34
wachterjohannes added a commit that referenced this pull request Aug 17, 2015
removed the locale as mandatory parameter from persist
@wachterjohannes wachterjohannes merged commit b6df514 into master Aug 17, 2015
@wachterjohannes wachterjohannes deleted the bugfix/persist-without-locale branch August 17, 2015 08:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make locale non-mandatory for persist.
3 participants