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

Replace fallback locale configuration and logic by dedicated Strategy class #41

Open
Gummibeer opened this issue Aug 7, 2019 · 4 comments
Assignees

Comments

@Gummibeer
Copy link
Member

Gummibeer commented Aug 7, 2019

The fallback logic should get moved out of the trait into dedicated class(es) which could be easily replaced by custom user logic without adjusting the whole trait.
It should be possible to chain multiple strategies to handle the current state but in multiple classes.

'fallback_strategies' => [
    LocaleStrategy::class => [
        'locale' => 'en',
    ],
    CountryLocaleStrategy::class,
    FirstDefinedLocaleStrategy::class,
],

A strategy instance should be created via DIC - this way an instance could also be bound, possible required injections will be done and custom arguments could be passed via an array like for LocaleStrategy.
The signature of called method should retrieve a lot of arguments to also handle model aware logic, attribute specific fallback and so on.

$strategy->fallback(
    Model $model,
    string $currentLocale,
    Collection $alreadyCheckedLocales,
    ?string $attribute = null,
): ?Model;

The method should return an instance of the translatable model or throw an exception FallbackNotFound which contains all checked locales null.
Every checked locale should be pushed to the collection. This way we can pass all already checked locales to the following strategies and prevent duplicated checks.

@Gummibeer
Copy link
Member Author

@netdown we've already wrote about this. Could you read through it and tell me if something missing or you would something different?

@netdown
Copy link
Contributor

netdown commented Aug 9, 2019

@Gummibeer It seems alright for me. I'm not sure how you thought, but I believe a default strategy should be embedded.

@Gummibeer
Copy link
Member Author

@netdown I thought to use the posted configuration as default. This will represent the current state. The user can remove, replace or reorder them but by default we will keep the current state.
So it could be that we could also release it in a minor one instead of a new major.

@netdown
Copy link
Contributor

netdown commented Aug 9, 2019

@Gummibeer I see now. Yes, great idea.

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

Successfully merging a pull request may close this issue.

2 participants