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 auto fallback feature #35

Merged
merged 15 commits into from
Jul 31, 2019
Merged

Conversation

netdown
Copy link
Contributor

@netdown netdown commented Jul 30, 2019

Currently, only one fallback locale can be used and it has to be predetermined in config. Auto fallback means looping through available locales (in configuration order) and using the first available translation. I believe this is much more realistic than using a specified fallback locale.

This commit generates no conflicts or unwanted operation, as this feature only operates if fallback_locale in configuration is set to null (null value is not used for anything currently). Way of loading translations neither affected.

netdown added 13 commits July 30, 2019 18:11
Add auto fallback feature (use first available locale as fallback locale), can be activated by setting fallback_locale to null.
Avoid too many return statements within this method - return null removed, makes no difference.
@netdown
Copy link
Contributor Author

netdown commented Jul 30, 2019

I gave up regarding cognitive complexity. Please take a look. Note that originally this was a 6 line commit...

@Gummibeer
Copy link
Member

Hey,
thanks for this PR - could you please ignore codeclimate (I think Iwill un-require it) and restore (force push) your original state?
The cognitive complexity is new and never checked the code. So you "fix" things you haven't introduced. I also know/believe that these complexity checks are a good helper but sometimes the code gets more complex by "fixing" the detected complexity just to get a green check mark by a bot.

After this I will take a look in depth. In general I'm not against this - but I have some things to keep in mind:

  • there are two groups
    • Managed content in primary/fallback locale
    • Random content in all locales
  • the fallback logic is already the nearly most complex part of the package
    • Fallback on/off
    • per translation/attribute
    • Country based and configured
    • + looping all until find

Some also changed the logic to respect a primary_locale column in the main model.

So just to warn you: it could be that I reject this particular PR in favor of a whole rework (respecting your changes and author) or simply merge it without an immediate release but reworking the fallback logic.

I really appreciate your work and input but have to check it in a quiet minute.

@netdown
Copy link
Contributor Author

netdown commented Jul 31, 2019

Hi,
Restored. In my opinion, auto fallback is more important than selecting preferred one by country. Of course that is also helpful. But thinking over, on a multilanguage site frontend, I think most people would want to display anything rather than nothing if translation is not available.

For example I'm making a restaurant system, let's say we have english, german, italian languages. For sure, if I haven't got the german and italian translation yet for some meals, I still want to show them. What if I set german as fallback and the user switches to italian language - meals only available in english won't show up.

I think the whole thing could be simplified by only making one fallback config var and removing the rest. This would also enable more specific configuration to country based fallback. However, merging my commit would be the best for keeping compatibility.

  1. False - disabled
  2. String - specifying one locale
  3. Array - specifying the fallback options and order for each locale (note that locales should not be multidimensional this way)
    'en' => ['fr', 'es-MX', 'es-CO'], 'fr' => ['en', 'es-MX', 'es-CO'], 'es-MX' => ['es-CO', 'fr', 'en'], 'es-CO' => ['es-MX', 'fr', 'en'],
  4. Null/true - auto fallback

This seems much cleaner for me, currently for configuring country based locale I would have to adjust 4 parameters.

@Gummibeer Gummibeer changed the base branch from master to ft-auto-fallback July 31, 2019 11:02
@Gummibeer Gummibeer self-assigned this Jul 31, 2019
@Gummibeer
Copy link
Member

Hey @netdown ,
now had some time to check it. Because of the simplicity and not touching any existing logic I'm fine with it.
I've changed the base-branch to add unittest, documentation and so on.

Thanks for your work!


Because at all I'm not fine with the current super complex condition-loop style I think about something like a dedicated class to get the fallback locale FallbackStrategy. Because it doesn't have to do much I would like to use __invoke. This would make it easier for the user to customize the logic to his needs and would also make the trait much cleaner.
What do you think?
PS: I will release your version in a non-BC release before refactoring the fallback logic - if I do it at all.

@Gummibeer Gummibeer merged commit 332e948 into Astrotomic:ft-auto-fallback Jul 31, 2019
@netdown
Copy link
Contributor Author

netdown commented Jul 31, 2019

Good idea to give full control to the users. Maybe the best would be if there was a default strategy with the idea (or simplified, like without point 3) I have written above, and if somebody wants it different, he can overwrite/inject his own strategy.
Thanks for the quick response!

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

Successfully merging this pull request may close these issues.

2 participants