Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Intl] fix Locale::getFallback() throwing exception on long $locale#40162
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedFeb 12, 2021
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
8f96c7a toa41a1c7CompareOskarStark commentedFeb 12, 2021
Thanks for your contribution, could you please add a testcase to avails a regression in the future? Also it looks like your commit/email is not associated with your github account 🧐 |
OskarStark commentedFeb 12, 2021
Friendly ping@ro0NL |
Uh oh!
There was an error while loading.Please reload this page.
AmirHo3ein13 commentedFeb 13, 2021
@OskarStark thanks for your review. At first, I added a test case for my change but seems like in CI (appveyor) the Also thank you for informing me about the email. I'll fix it and commit with the right email. |
AmirHo3ein13 commentedFeb 14, 2021
@OskarStark@ro0NL seems like there is a problem with Doctrine, you can see ithere. Please check it, thanks in advance. |
xabbuh commentedFeb 14, 2021
The Doctrine failure isn't related. We can ignore it here. |
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas left a comment
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.
Can you please add a test case?
AmirHo3ein13 commentedFeb 16, 2021
@nicolas-grekas as I wrote here, I have a problem testing my change and I need some help. Can you help me?
|
nicolas-grekas commentedFeb 16, 2021
Can you add the test to the PR? |
AmirHo3ein13 commentedFeb 16, 2021
@nicolas-grekas thank you very much. I added the test successfully. |
| 'LC_MONETARY=fr_FR.UTF-8;LC_MESSAGES=fr_FR.UTF-8;LC_PAPER=fr_FR.UTF-8;LC_NAME=fr_FR.UTF-8;'. | ||
| 'LC_ADDRESS=fr_FR.UTF-8;LC_TELEPHONE=fr_FR.UTF-8;LC_MEASUREMENT=fr_FR.UTF-8;LC_IDENTIFICATION=fr_FR.UTF-8'; | ||
| $this->assertNull(Locale::getFallback($locale)); |
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.
do we agreeroot is the fallback then?
compared to above test$this->assertSame('root', Locale::getFallback('nl'));
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.
actually $defaultFallback /en is expected
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.
@ro0NL the function is returningnull on long $locale by this change:$localeSubTags = locale_parse($locale) ?? ['language' => $locale];
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 thinknull is correct also.
ro0NL left a comment
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.
no strong opinion behavior-wise,null is reasonable
05070df toa89ced8Comparenicolas-grekas commentedFeb 17, 2021
Thank you@AmirHo3ein13. |
Uh oh!
There was an error while loading.Please reload this page.
Locale::getFallback()throws an exception when the$localelength is greater thanINTL_MAX_LOCALE_LENso I added a condition to check if locale_parse return null, theLocale::getFallback()don't call\countfunction and just return null instead.