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

Convert locale to use - instead of _ from symfony #452

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

toooni
Copy link
Contributor

@toooni toooni commented Aug 17, 2023

Subject

Symfony does use country specific locales like de_CH. Js on the other hand uses de-CH. This results in a js issue, since datepicker_controller.js only splits by -.

I am targeting this branch, because the change fixes a bug and keeps BC.

Changelog

### Fixed
- Country specific locale issues (convert de_CH to de-CH)

@VincentLanglet
Copy link
Member

CC @jordisala1991

Hi, @toooni thanks for the contribution.
WDYT would be better ?

  • Doing the replace "_" to "-" in php like you did
  • Adding support for _ in the js

I wonder if the replace couldnt be consider as a BC break if someone use the option value for something else...

@toooni
Copy link
Contributor Author

toooni commented Aug 26, 2023

@VincentLanglet I had considered changing the locale in js but I think it's better if we do this in PHP and JS always gets locales which it supports.
One possibility would be if we replace it in finishView (https://github.com/sonata-project/form-extensions/pull/452/files#diff-ca4ed843dd8a9065ab8cb9735c6ff7d0e63a973223fa9f5a1d082065c1c7f569L222) with something like this:

if (isset($datePickerOptions['localization']['locale'])) {
    $datePickerOptions['localization']['locale'] = str_replace('_', '-', $datePickerOptions['localization']['locale'])
}

This will reduce the possibility of a BC break and JS will always get a valid js locale. Theoretically, a BC break can happen when someone uses this var inside a twig template. But it's very unlikely since this change is pretty new and country specific locales in the current code do not work.

@jordisala1991
Copy link
Member

I would say it is fine doing this change on the php side (it would probably do it on the js side since we already do other checks for the locale on the stimulus controller already). Also do remember that currently there is only one supported locale with country part on the js #allowedLocales (you might want to update that , and the js library if there are more locales now)

@VincentLanglet
Copy link
Member

I would say it is fine doing this change on the php side

If I understand correctly, you approve this PR ? :)

@Kocal
Copy link

Kocal commented Sep 29, 2023

Hi, we are facing the same issue after upgrading sonata-project/admin-bundle from 4.25.0 to 4.27.0, which updated sonata-project/form-extensions 1.19.1 to 2.0.0.

image

Passing en-GB is fine, but en_GB is not:
image

My 2 cents, I find this solution #452 (comment) really fine, as we want to inject a JS valid locale without touching the locale PHP-side.

@VincentLanglet
Copy link
Member

Hi @Kocal does this PR would be ok for you as if too ?

@Kocal
Copy link

Kocal commented Sep 29, 2023

Hey @VincentLanglet, I think yes, we used str_replace('_', '-', $this->locale) in our project through a composer patch.

But it's possible that is does not work if we manually override the option.

@VincentLanglet VincentLanglet merged commit 79fcf63 into sonata-project:2.x Sep 29, 2023
21 checks passed
@Kocal
Copy link

Kocal commented Sep 29, 2023

Thanks for merging :)

@VincentLanglet
Copy link
Member

I'll release it with #455

@jordisala1991
Copy link
Member

I would say it is fine doing this change on the php side

If I understand correctly, you approve this PR ? :)

Yes, Sorry for not giving explicit approval 🙏

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

Successfully merging this pull request may close these issues.

None yet

4 participants