detect language by apache mod_geoip country code #131
Conversation
| @Vediovis Thanks, this looks interesting. I'm not totally sure yet, if we want to include this, but I'll seriously consider it. |
UrlManager.php Outdated
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Browser preferences should always have highest priority. So this block should be moved further down.
There was a problem hiding this comment.
i was thinking about placing it after enableLanguageDetection block
but detecting by geoip and enableLanguageDetection are excluding each other a bit
and it looks like it does not make sense to detect by geoip if you have enableLanguageDetection set to true and it has higher priority
There was a problem hiding this comment.
Hmm, I don't think that they exclude each other. If someone has configured for example "en" in the browser then the user has made a clear decision. He knows, which language he wants and he tells us via the header fields. So even if this user is in holiday in some foreign country, we should not suddenly show him a different language.
So all there is to do is really to move this down a little bit, then everything should be fine.
There was a problem hiding this comment.
ok, changed everything as you suggested
UrlManager.php Outdated
| /** | ||
| * @var string $_SERVER key set by apache mod_geoip | ||
| */ | ||
| public $geoipServerFieldName = 'HTTP_X_GEO_COUNTRY'; |
There was a problem hiding this comment.
I think a better name is $geoIpServerVar because most entries in $_SERVER are environment variables set by the webserver. We should also mention the default value in the description:
@var string the key in $_SERVER that contains the detected GeoIP country. Default is 'HTTP_X_GEO_COUNTRY' as used by mod_geoip in apache. UrlManager.php Outdated
| public $geoipServerFieldName = 'HTTP_X_GEO_COUNTRY'; | ||
| | ||
| /** | ||
| * @var array list of countries for given language code |
There was a problem hiding this comment.
Better: "list of GeoIP countries indexed by corresponding language code".
It should also have a note about the default value: "The default is an empty list which disables GeoIP detection".
UrlManager.php Outdated
| if ($this->enableLanguagePersistence) { | ||
| $language = $this->loadPersistedLanguage(); | ||
| } | ||
| if ($language===null && isset($_SERVER[$this->geoipServerFieldName]) && !empty($this->geoipLanguageCountries)) { |
There was a problem hiding this comment.
I would say, you could remove !empty($this->geoipLanguageCountries) here because if the array is empty, foreach will be skipped anyway.
UrlManager.php Outdated
| * e.g. 'ru' => ['RUS','AZE','ARM','BLR','KAZ','KGZ','MDA','TJK','TKM','UZB','UKR'] | ||
| * will set app language to ru for countries listed above | ||
| */ | ||
| public $geoipLanguageCountries = []; |
There was a problem hiding this comment.
I think the correct camel case name should be geoIpLanguageCountries (with upper case I)
fix detection order and phpdocs
| Great, thanks for your time. I will add some tests, update the README and also reorganize the whole class a little as some methods have become quite big now. After that there will be a 1.7.0 release which includes this new feature. Thanks again! |
| @Vediovis Just released 1.7.0 |
2 participants
No description provided.