Skip to content
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

Allow default locale to be configured #123

Closed
wants to merge 3 commits into from

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Jan 17, 2019

In preparation for the removal of $this->holiday from Holiday and Provider #122, this adds the suggested default language setting in backwards compatible way.

Two new class methods are added, Yasumi::setDefaultLocale($locale) and Yasumi::getDefaultLocale(), and a new optional argument is added to $holiday->getName($locale = null) that allows overriding the default.

This approach is used e.g. by the PHP intl extension, the Symfony Translator, Laravel, Drupal's t() function and the Punic library.

Passing a locale to Yasumi::create() or Yasumi:: createByISO3166_2() is now deprecated (but still works as before). People can already now migrate to the new behaviour suggested for Yasumi 3.0 in #122. A deprecation warning is triggered following the tradition in Symfony.

This paves the way for a second PR (#124) that adds support for fallback locales. I made two PR's in order to make code review easier. I recommend merging both PR's before doing a new release to ensure that the dust has settled on the API.

This was referenced Jan 17, 2019
c960657 added a commit to c960657/yasumi that referenced this pull request Aug 17, 2019
@c960657
Copy link
Contributor Author

c960657 commented Aug 17, 2019

@stelgenhof What do you think of this?

For the record: This does not break backwards compatibility but only deprecates some things. Such deprecations should preferably land early in the 2.x development cycle to allow users of the library to adjust their code already now, so their eventual upgrade to 3.x will be seamless.

@c960657
Copy link
Contributor Author

c960657 commented Aug 17, 2019

@Milamber33 The visibility changes are not part of this PR.

They were committed to the develop branch back in January 2019.
326e8ec
22609ce
d7b5e92
13dd203
f1a4892

@Milamber33
Copy link
Contributor

@c960657 Okay, noted, I missed that. The question remains, though I grant that this is probably the wrong place for it now.

@stelgenhof
Copy link
Member

@Milamber33 As for the visibility of class members, you are right. When I started the project I didn't pay attention to the visibility much (I know: bad me!), however this should be changed of course.

@stelgenhof
Copy link
Member

@c960657 I had a look at the code of this PR and think it looks good. Just want to do a few more test rounds.

My plan is do to a 2.2 release with this PR included. 2.2 will also contain all changes since v2.1 plus I am working on the Chile holiday provider. With this, also the documentation needs to be updated.

I don't have a particular target date yet in mind, but hopefully soon.

BTW: Big thanks for the coffee!! I feel I should return the favor as you have contributed much to Yasumi :)

Cheers! Sacha

@stelgenhof stelgenhof added this to the v2.2.0 milestone Aug 18, 2019
@stelgenhof stelgenhof removed this from the v2.2.0 milestone Sep 14, 2019
@c960657 c960657 mentioned this pull request Sep 17, 2019
@c960657
Copy link
Contributor Author

c960657 commented Sep 18, 2019

There is something about this that does not feel right. I seem to mix up default locale and fallback locale. I'll prepare an update. Stay tuned. I suggest this is merged after #176 (if that is accepted).

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.

3 participants