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

Add helper function to get the supported locales excluding mappings #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Erikvv
Copy link

@Erikvv Erikvv commented May 30, 2018

I've been using the mappings feature and found I needed this helper function often enough that it might be worthwhile to add.

Maybe it's also nice to include this option in LocaleMenu

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 82.552% when pulling 40af670 on Erikvv:feature/main-locales into 2a8a0a6 on basz:master.

@svycka
Copy link
Collaborator

svycka commented Jun 13, 2018

not sure about this. Why do you need this?

@Erikvv
Copy link
Author

Erikvv commented Jun 13, 2018

I use this to show a dropdown of locales to select. I will store it in the database and then later use it in jobs to determine which language to send an e-mail or generate a PDF for that particular user.

It makes no sense to show mapped locales in this dropdown, they would be duplicates and using them would break the application as they are not supported.

The mapped locales are only used for Accept-Language headers they aren't visible anywhere else. This was always the intention. See discussion #42

IMHO mapped locale's should not have to be included in Detector::supported in order to work. I misinterpreted the original PR believing that it would work that way (#87)

TLDR; Instead of this feature I should fix it so that you can have mapped-from locales which are not present in the "supported" array. Agreed @svycka?

The mapping feature itself is still necessary I see no way around that. The added complexity is unfortunate but seems necessary.

@svycka
Copy link
Collaborator

svycka commented Jun 13, 2018

for dropdown we have https://github.com/basz/SlmLocale/blob/master/src/SlmLocale/View/Helper/LocaleMenu.php this view helper with use supported locales can't you use this? Or does this return wrong locales? Maybe then we should fix this helper?

*/
protected $supported;

/**
* Optional list of locale mappings
*
* @var array
* @var array & string[string]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be array and maybe add an example or/and description

@Erikvv
Copy link
Author

Erikvv commented Jun 13, 2018

The view helper returns the wrong locales also.

My config is currently like so (shortened example):

return [
    'slm_locale' => [
        'supported' => [
            'de_DE',
            // will be mapped:
            'de_AT',
        ],
        'mappings' => [
            'de_AT' => 'de_DE',
        ]
    ],
];

I want it to be like this but this doesn't work:

return [
    'slm_locale' => [
        'supported' => [
            'de_DE',
        ],
        'mappings' => [
            'de_AT' => 'de_DE',
        ]
    ],
];

If I fix that then I don't need this PR. Understand?

@svycka
Copy link
Collaborator

svycka commented Jun 13, 2018

okay now I understand your pain :) then fixing this would be better or maybe add an option to localeMenu helper to include or skip mapped locales? @basz what do you think?

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

Successfully merging this pull request may close these issues.

None yet

3 participants