dimsav / laravel-translatable

[Deprecated] A Laravel package for multilingual models
MIT License
1.95k stars 320 forks source link

Set Locale method not working #344

Closed ConstantMath closed 7 years ago

ConstantMath commented 7 years ago

Hi, First of all, thanks for this amazing package, very useful here in Belgium ;-) ! IO have a little bug when I try to print a translated field like this: echo $article->title;

The app doesn't use the current locale.

I set the locale like that in my controller: App::setLocale('fr');

Inside the view:

This prints 'fr': app()->getLocale()

This works : $article->translate('fr')->title

But not if I use the shortcut directly.

Thanks for your help !

Gummibeer commented 7 years ago

The locale() method on the Translatable trait looks like this

protected function locale()
{
    if ($this->defaultLocale) {
        return $this->defaultLocale;
    }
    return app()->make('config')->get('translatable.locale')
        ?: app()->make('translator')->getLocale();
}

https://github.com/dimsav/laravel-translatable/blob/master/src/Translatable/Translatable.php#L623-L631

So if you have set a default locale or have set a locale in the translatable.locale it won't use the current app locale. I don't know why @dimsav has choosed to make the locale picker this complex!? But if you want to solve it by yourself just create your own Trait and override this method how you need it.

use Dimsav\Translatable\Translatable as BaseTranslatable;

trait Translatable extends BaseTranslatable {
    protected function locale()
    {
        return app()->make('translator')->getLocale();
        // or the shorter and config based one
        return app()->getLocale();
    }
}
dimsav commented 7 years ago

The reason this is complex is to allow customization of the locale. @ConstantMath check what is the output of the locale() method. Have you defined a $this->defaultLocale or the locale in the configuration?

ConstantMath commented 7 years ago

Thanks for your answers !

Mmm, no I don't define a default locale, here is my config file:

'locales' => [
    'en',
    'fr'
],

'locale_separator' => '-',

'locale' => null,

'use_fallback' => false,

'fallback_locale' => 'en',

'translation_suffix' => 'Translation',

'locale_key' => 'locale',

'to_array_always_loads_translations' => true,
dimsav commented 7 years ago

It would be helpful if you could try to debug the output of locale()

ConstantMath commented 7 years ago

it returns "en"

but 'App::getLocale()` still returns "fr"

°_°

Gummibeer commented 7 years ago

@ConstantMath can you dump() the three values alone?

protected function locale()
{
    dump($this->defaultLocale);
    dump(app()->make('config')->get('translatable.locale'));
    dump(app()->make('translator')->getLocale());
    if ($this->defaultLocale) {
        return $this->defaultLocale;
    }
    return app()->make('config')->get('translatable.locale')
        ?: app()->make('translator')->getLocale();
}
ConstantMath commented 7 years ago

@Gummibeer

  "en"

  null

  "fr"

thanks ;-)

Gummibeer commented 7 years ago

@ConstantMath so you have a defaultLocale on this class - find where you define it or override the whole method to make it simpler. ;)

ConstantMath commented 7 years ago

OK ok ok I found that in my model :-/

Thanks again for your help

dimsav commented 7 years ago

@ConstantMath do you have any recommendation of how this could be prevented? Is there something we can change in the package?

ConstantMath commented 7 years ago

@dimsav mmm like @Gummibeer said it's not easy to understand why there is a test concerning the default locale, but I'm sure you have your reasons ;-).

No, maybe write it more clearly in the Readme ?

Anyway, thanks for your communjcation.

Gummibeer commented 7 years ago

@dimsav in my opinion this method is way too complex! A simple return of the application locale should be enough, like here https://github.com/dimsav/laravel-translatable/issues/344#issuecomment-296116274

Why? Cause I think that a package should be simple and basic but extendible. Cause of the fact that everyone, that needs more complex locale finders, can create his own Trait that overrides the basic one I think that a basic solution in package should be enough and better. This is understandable for everyone, covers 90% of all usecases (I think) and shows, in a basic way, how/where it can be done.

Trying to cover every single usecase in a general package, is for me, no good way. It complicates the maintenance, contribution and increases the code complexity in benefit of 2 or 3 users/devs.

dimsav commented 7 years ago

@Gummibeer by default the package uses the app()->make('translator')->getLocale(). Unless the developer overwrites the default, this is what is used. I don't want to remove features just for the sake of simplicity. Forcing the app's locale as default might work for most of the cases, but not for everyone (cms for example).

sineld commented 6 years ago

Setting 'locale' => null, on translatable.php file fixed my problem.

Gummibeer commented 6 years ago

@sineld we've added this to the config, to help others: #470

sineld commented 6 years ago

@Gummibeer I see. Thanks for informing.