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

Allow to change default translation model namespace from config file #508

Merged
merged 5 commits into from
Aug 7, 2018

Conversation

ahmed-aliraqi
Copy link
Contributor

No description provided.

.gitignore Outdated
@@ -1,3 +1,4 @@
.idea/
Copy link
Collaborator

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

*/
public function getTranslationModelNamespace()
{
return config('translatable.translation_model_namespace', 'App');
Copy link
Collaborator

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.

| application, set this to 'App\Translations'.
|
*/
'translation_model_namespace' => 'App',
Copy link
Collaborator

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

|
| 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'.
Copy link
Collaborator

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!?

@@ -23,6 +23,8 @@ public function setUp()

parent::setUp();

App::make('config')->set('translatable.translation_model_namespace', 'Dimsav\Translatable\Test\Model');
Copy link
Collaborator

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.

@@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this->app->make('config')

@ahmed-aliraqi
Copy link
Contributor Author

thanks @Gummibeer 👍

@Gummibeer
Copy link
Collaborator

If @dimsav approves this I will put it into v9.1.0 #509

@dimsav
Copy link
Owner

dimsav commented Aug 6, 2018

@Gummibeer sure
Nice work @ahmed-aliraqi !

@Gummibeer
Copy link
Collaborator

Will merge this tomorrow, add it to changelog and release v9.1.0 after a long time as next Minor release! 😊🎉

@Gummibeer Gummibeer merged commit b1aa31a into dimsav:master Aug 7, 2018
Gummibeer added a commit that referenced this pull request Aug 7, 2018
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.

3 participants