Skip to content

detect language by apache mod_geoip country code #131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 13, 2018
Merged

detect language by apache mod_geoip country code #131

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a26f445
detect language by apache mod_geoip country code
Vediovis Feb 10, 2018
2b92787
detect language by apache mod_geoip country code
Vediovis Feb 12, 2018
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions UrlManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,18 @@ class UrlManager extends BaseUrlManager
*/
public $languageParam = 'language';

/**
* @var string $_SERVER key set by apache mod_geoip
*/
public $geoipServerFieldName = 'HTTP_X_GEO_COUNTRY';
Copy link
Collaborator

@mikehaertl mikehaertl Feb 12, 2018

Choose a reason for hiding this comment

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

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.

/**
* @var array list of countries for given language code
Copy link
Collaborator

@mikehaertl mikehaertl Feb 12, 2018

Choose a reason for hiding this comment

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

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".
* 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 = [];
Copy link
Collaborator

@mikehaertl mikehaertl Feb 12, 2018

Choose a reason for hiding this comment

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

I think the correct camel case name should be geoIpLanguageCountries (with upper case I)

/**
* @var \yii\web\Request
*/
Expand Down Expand Up @@ -370,6 +382,14 @@ protected function processLocaleUrl($normalized)
if ($this->enableLanguagePersistence) {
$language = $this->loadPersistedLanguage();
}
if ($language===null && isset($_SERVER[$this->geoipServerFieldName]) && !empty($this->geoipLanguageCountries)) {
Copy link
Collaborator

@mikehaertl mikehaertl Feb 12, 2018

Choose a reason for hiding this comment

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

I would say, you could remove !empty($this->geoipLanguageCountries) here because if the array is empty, foreach will be skipped anyway.
foreach ($this->geoipLanguageCountries as $key => $codes) {
if (in_array($_SERVER[$this->geoipServerFieldName], $codes)) {
$language = $key;
break;
}
}
}
Copy link
Collaborator

@mikehaertl mikehaertl Feb 11, 2018

Choose a reason for hiding this comment

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

Browser preferences should always have highest priority. So this block should be moved further down.
Copy link
Contributor Author

@Vediovis Vediovis Feb 11, 2018

Choose a reason for hiding this comment

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

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

@mikehaertl mikehaertl Feb 12, 2018

Choose a reason for hiding this comment

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

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.
Copy link
Contributor Author

@Vediovis Vediovis Feb 12, 2018

Choose a reason for hiding this comment

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

ok, changed everything as you suggested
if ($language===null && $this->enableLanguageDetection) {
foreach ($this->_request->getAcceptableLanguages() as $acceptable) {
list($language,$country) = $this->matchCode($acceptable);
Expand Down