Skip to content
This repository has been archived by the owner on Jun 18, 2019. It is now read-only.

A model can have a default locale for translations #271

Merged
merged 3 commits into from
Jan 29, 2017

Conversation

shadoWalker89
Copy link
Contributor

This will allow to return the translation in the desired locale, by calling the attribute on the model without the need of changing the application locale using App::setLocale();

@dimsav
Copy link
Owner

dimsav commented Sep 2, 2016

Skipping App::setLocale() and setting a protected variable on the fly means we need to assign the locale for each different model which is a smell because it would be easy to forget to set the locale for another model. Thoughts?

@shadoWalker89
Copy link
Contributor Author

You don't need to change anything, and the user doesn't need to define the $defaultLocale for each model.

As you can see in the locale() method i'm taking the $defaultLocale into consideration only if a value exists. If not we check the config values.

To use the default locale the user have two options :

  • Redefine the $defaultLocale on the model if he needs. (If he doesn't define it, everything will work as it used to be before this update)
  • Set the default locale in runtime using setDefaultLocale()

@dimsav
Copy link
Owner

dimsav commented Sep 2, 2016

Can you please explain why you need this feature?

@shadoWalker89
Copy link
Contributor Author

shadoWalker89 commented Sep 2, 2016

So i have a DataSet model with and "fr" and "en" translations.

When showing this model, i will not always use the application locale, but i'll use the locale defined in the url. The url for showing the model is this "data-sets/{id}/{locale}"

What i need it to always use this syntax in my views $dataSet->title

So i have two options :

  • Change the application locale with App::setLocale() which is very bad, because that will mess up other things.
  • use the following notation $dataSet->{'title:fr'} Which is against my goal which is use the following syntax $dataSet->title

Now when it comes to the code of my pull request, it doesn't change a bit about how the package works (edit: of course except for the fact that adds the possibility to set the default locale for each model separately). People already using the package doesn't need to touch their code at all.

The $defaultLocal attribute is already defined in the trait, so the users of the package don't need to define it in their models.

The locale() will take the $defaultLocale into consideration only if :

  • The $defaultLocale is defined on the model.
  • Call setDefaultLocale('myLocalCode')

This addition will make the package more flexible and most important of all it doesn't require any changes to existing application code.

@dimsav
Copy link
Owner

dimsav commented Jan 27, 2017

Sorry for the long delay. Pull requests cannot be merged without tests supporting the changes.

Your point makes sense though and it seems to me we should add this feature.

@dimsav dimsav added the feature label Jan 27, 2017
@shadoWalker89
Copy link
Contributor Author

Ok i'll add tests

shadoWalker89 and others added 2 commits January 28, 2017 20:00
This will allow to return the translation in the desired locale, by calling the attribute on the model without the need of changing the application locale using App::setLocale();
@shadoWalker89
Copy link
Contributor Author

shadoWalker89 commented Jan 28, 2017

@dimsav Rebased and added tests

Can you please confirm that all tests are passing, because i tried running them and i get this error

PHP Fatal error: Class 'PHPUnit\Framework\TestCase' not found in /home/vagrant/trans/laravel-translatable/vendor/orchestra/testbench/src/TestCase.php on line 22

The problem is the same in homestead and on my windows machine

@dimsav
Copy link
Owner

dimsav commented Jan 29, 2017

This looks great @shadoWalker89 ! The tests work great.

@dimsav dimsav merged commit 09fe03b into dimsav:master Jan 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants