Skip to content
This repository has been archived by the owner. It is now read-only.

Will not work for Indonesian translation #18

Closed
taai opened this issue May 13, 2014 · 8 comments

Comments

@taai
Copy link

commented May 13, 2014

It wont work simply because the Indonesian language code is id.

@dimsav

This comment has been minimized.

Copy link
Owner

commented May 13, 2014

I don't see why this causes a problem. What is the error message you get?

Please provide more info.

@taai

This comment has been minimized.

Copy link
Author

commented May 13, 2014

@dimsav I didn't even test it. Code is very readable, so I red it. 😉
Here is an example of getting Indonesian translation:

<?php
$country = Country::where('iso', '=', 'id')->first();
echo $country->id->name; // Will throw an error (probably)

I believe that the error will be about that I'm trying to get name attribute from non-object. That, or it will return the translation, but wont be able to get the id number of the Country model. I don't know.

@dimsav

This comment has been minimized.

Copy link
Owner

commented May 13, 2014

Ok, I will check this after the end of my holidays in 10 days.

@dimsav

This comment has been minimized.

Copy link
Owner

commented May 13, 2014

In the meantime use "ind" as iso.

@taai

This comment has been minimized.

Copy link
Author

commented May 14, 2014

@dimsav I have never made an app with Indonesian language in use. I'm just saying that this is a potential problem.
Because of this potential problem everybody should better use the getTranslation() method instead of fetching language-like attributes.

$country = Country::where('iso', '=', 'gr')->first();
echo $country->getTranslation('en')->name; // Greece

App::setLocale('en');
echo $country->name;     // Greece

App::setLocale('de');
echo $country->name;     // Griechenland

For Indonesian id locale this won't work anyway, because your code calls getAttribute() which will try to get id... You should use $this->relations[$locale] to access the translation models.

I know that using $mondel->getTranslation('en') is little less handy than using $model->en, but on the other side - it is more understandable.

Probably you it's ok to keep also $model->en option, but I recommend not to promote it as the best way to do this.

@sdebacker

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2014

I agree with taai.

@dimsav

This comment has been minimized.

Copy link
Owner

commented May 21, 2014

Alright, so I will update the documentation in the readme file and promote getTranslation() instead of $country->en->name. I wonder if it would be a good idea to deprecate it in a future version.

Update: The syntax $country->en->name has to be and will be deprecated in the future version. id is the most frequent iso code, nevertheless, the same will happen in the model (country) has another attributed using the same name with a used iso code.

Thanks for the notice!

@dimsav dimsav added the bug label May 21, 2014

@dimsav dimsav added this to the 4.0 milestone May 21, 2014

dimsav added a commit that referenced this issue May 22, 2014

@dimsav

This comment has been minimized.

Copy link
Owner

commented May 22, 2014

The changes were just released with the new version 4.0.0. I also created a method translate() alias of getTranslation() to make things more readable. Read the readme and let me know if you disagree on something.

@taai and @sdebacker thanks for your feedback!

@dimsav dimsav closed this May 22, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.