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

Locale errors #10

Closed
gustavs-gutmanis opened this issue Apr 1, 2016 · 4 comments
Closed

Locale errors #10

gustavs-gutmanis opened this issue Apr 1, 2016 · 4 comments
Labels

Comments

@gustavs-gutmanis
Copy link
Contributor

Hi @rlanvin,

Seems like it's impossible to use the RRule::humanReadable() method if the \Locale doesn't exist, or if the locale is wrong. There are some cases where the default Locale is en_US_POSIX which in turn triggers an exception in RRule::i18nLoad() line 2022. Would it be possible to fallback to en if the locale isn't set or isn't supported?

I could make a PR for it if you'd like, just wondering if that's something you see as fitting in this case?

@rlanvin
Copy link
Owner

rlanvin commented Apr 1, 2016

Hi,

Sure, a PR is always appreciated, so if you feel like it feel free. Here are my comments:

  • Locale is part of the intl extension. I had intended this extension to be optional, so it definitely should not fail if intl is not loaded... I suggest you get the locale from the old fashioned setlocale() method when intl is not loaded.
  • I didn't know that en_US_POSIX was a valid locale, so yes, the regexp in line 2022 should probably be updated. I think it some cases the old setlocale() can also return strange locales like C I think, so keep that in mind.
  • Having a fallback locale if the locale is not set for some reason a good idea, though I think Locale or setlocale will always return something.
  • Having a fallback locale if the locale doesn't exist is also a good idea, but I'm wondering if hardcoding en is a good idea. Maybe this could be an option of humanReadable (with en as default, so I can select another fallback language if necessary, and null or false meaning "no fallback, throw an exception if you can't find the locale")

Also, please remember to add (or update) a test that demonstrate the bugs in the PR. You should also update the CHANGELOG file.

@gustavs-gutmanis
Copy link
Contributor Author

Ok, thanks for the input - very good points.
I'll update the tests, but I wonder how could I test Locale class not existing.

@gustavs-gutmanis
Copy link
Contributor Author

I have a question - what should happen if there is no translation file for a given locale?
Right now it throws an exception saying that it couldn't load translations for said locale.
Maybe it should try to use the $fallbackLocale, if any were to be given?

@rlanvin
Copy link
Owner

rlanvin commented Apr 4, 2016

Fixed by PR #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants