-
Notifications
You must be signed in to change notification settings - Fork 321
Allow to change default translation model namespace from config file #508
Conversation
.gitignore
Outdated
@@ -1,3 +1,4 @@ | |||
.idea/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't project related and should be done in your local global .gitignore
file.
https://stackoverflow.com/a/7335487/4907524
src/Translatable/Translatable.php
Outdated
*/ | ||
public function getTranslationModelNamespace() | ||
{ | ||
return config('translatable.translation_model_namespace', 'App'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the default logic and is BC.
I won't merge this feature if it's BC because it's too specific.
To solve this the default config value should be null
als fallback it should use the namespace of get_class()
.
Atm it will also work, by default, if model namespace is moved into App\Models
or anywhere else. After this change I have to adjust the config to keep this running. That's why I want to keep the default behavior and just add the option to adjust the namespace.
src/config/translatable.php
Outdated
| application, set this to 'App\Translations'. | ||
| | ||
*/ | ||
'translation_model_namespace' => 'App', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be null
by default
src/config/translatable.php
Outdated
| | ||
| Defines the default 'Translation' class namespace. For example, if | ||
| you want to use App\Translations\CountryTranslation instead of App\CountryTranslation | ||
| application, set this to 'App\Translations'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application
doesn't makes sense here!?
tests/TestsBase.php
Outdated
@@ -23,6 +23,8 @@ public function setUp() | |||
|
|||
parent::setUp(); | |||
|
|||
App::make('config')->set('translatable.translation_model_namespace', 'Dimsav\Translatable\Test\Model'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it's in a lot of places in unittests but can you please switch to $this->app->make('config')
instead of the Facade.
tests/TranslatableTest.php
Outdated
@@ -16,6 +16,15 @@ public function test_it_finds_the_default_translation_class() | |||
$country->getTranslationModelNameDefault()); | |||
} | |||
|
|||
public function test_it_finds_the_translation_class_with_namespace_set() | |||
{ | |||
App::make('config')->set('translatable.translation_model_namespace', 'App\Models\Translations'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->app->make('config')
thanks @Gummibeer 👍 |
@Gummibeer sure |
Will merge this tomorrow, add it to changelog and release |
No description provided.