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.

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

Typical elgg_language_key_exists() usage creates bugs #9375

Closed
mrclay opened this Issue Feb 11, 2016 · 9 comments

Comments

Projects
None yet
3 participants
@mrclay
Member

mrclay commented Feb 11, 2016

elgg_language_key_exists() doesn't consider that what the developer usually cares about is whether the key will be translated at all. Consider this example:

A plugin checks a language key "foo:$type":

if (elgg_language_key_exists("foo:$type")) {

Now a Spanish site installs it, defines that key in "es". This will fail because the plugin's check only checks for the English translations. OK, so the plugin is rewritten:

if (elgg_language_key_exists("foo:$type", get_language())) {

Now it's installed on an English site and the key is defined under "en". This will fail for a user who's language is set to "es". So really a dev has to do check both:

if (elgg_language_key_exists("foo:$type", get_language()) || elgg_language_key_exists("foo:$type")) {

I think we'd really want a function that would check the fallbacks as well, maybe elgg_can_echo()?

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 12, 2016

Member

i agree it should check fallbacks, however i think it can be done it the current function... what elgg needs is a new way of storing translations... currently we have seperate arrays for each langauge... is the translation not in Spanish we fall back to English... There is still the bug that fall back is not correct (user->language should fallback to site->language should fall back to 'en'... this is not the case see #4389).

If we change the storage of all loaded languages for the current user into one combined array (array_merge('en', $site_language, $user_language)) then there is just one array to elgg_echo from and to check if language exists. 2 flies one stroke... or something like that :)

I think this approach is a lot easier... and better performing as you can reuse the combined array (cache on disk... serve via javascript)

This approach can be fully backward compatible AFAICS

Member

jdalsem commented Feb 12, 2016

i agree it should check fallbacks, however i think it can be done it the current function... what elgg needs is a new way of storing translations... currently we have seperate arrays for each langauge... is the translation not in Spanish we fall back to English... There is still the bug that fall back is not correct (user->language should fallback to site->language should fall back to 'en'... this is not the case see #4389).

If we change the storage of all loaded languages for the current user into one combined array (array_merge('en', $site_language, $user_language)) then there is just one array to elgg_echo from and to check if language exists. 2 flies one stroke... or something like that :)

I think this approach is a lot easier... and better performing as you can reuse the combined array (cache on disk... serve via javascript)

This approach can be fully backward compatible AFAICS

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 12, 2016

Contributor

That's quite a nice approach I think.

Contributor

hypeJunction commented Feb 12, 2016

That's quite a nice approach I think.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 12, 2016

Member
$languages = [
    $user->language,
    $site->language,
    'en',
];

$languages = array_unique($languages);

$combined_translations = [];
foreach ($languages as $lang) {
     $translation = load_translation($lang); // or something else to get all translations for a certain language
     $combined_translations = $combined_translations + $translation;
}

something like that

Member

jdalsem commented Feb 12, 2016

$languages = [
    $user->language,
    $site->language,
    'en',
];

$languages = array_unique($languages);

$combined_translations = [];
foreach ($languages as $lang) {
     $translation = load_translation($lang); // or something else to get all translations for a certain language
     $combined_translations = $combined_translations + $translation;
}

something like that

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 12, 2016

Member

Right, then you need unique cache keys; this has to be resolved and cached on demand when the first user selects a new language (changing its involvement in system cache quite a bit). We've known this is what needs to be done a long time; it's just a lot of work.

Steve

http://community.elgg.org/profile/steve_clay

On Feb 12, 2016, at 3:22 AM, Jeroen Dalsem notifications@github.com wrote:

$languages = [
$user->language,
$site->language,
'en',
];

$languages = array_unique($languages);

$combined_translations = [];
foreach ($languages as $lang) {
$translation = load_translation($lang); // or something else to get all translations for a certain language
$combined_translations = $combined_translations + $translation;
}
something like that


Reply to this email directly or view it on GitHub.

Member

mrclay commented Feb 12, 2016

Right, then you need unique cache keys; this has to be resolved and cached on demand when the first user selects a new language (changing its involvement in system cache quite a bit). We've known this is what needs to be done a long time; it's just a lot of work.

Steve

http://community.elgg.org/profile/steve_clay

On Feb 12, 2016, at 3:22 AM, Jeroen Dalsem notifications@github.com wrote:

$languages = [
$user->language,
$site->language,
'en',
];

$languages = array_unique($languages);

$combined_translations = [];
foreach ($languages as $lang) {
$translation = load_translation($lang); // or something else to get all translations for a certain language
$combined_translations = $combined_translations + $translation;
}
something like that


Reply to this email directly or view it on GitHub.

@jdalsem

This comment has been minimized.

Show comment
Hide comment
@jdalsem

jdalsem Feb 12, 2016

Member

It think the amount of cache updates is not that frequent.. we can cache based on the unique combination of languages md5('en-nl-fr').. there are not that many unique combinations on an average site...

Member

jdalsem commented Feb 12, 2016

It think the amount of cache updates is not that frequent.. we can cache based on the unique combination of languages md5('en-nl-fr').. there are not that many unique combinations on an average site...

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Feb 12, 2016

Contributor

Can't we automatically merge non-site language keys with site language keys
and English when we cache it. So instead of falling back in code, we would
have the fallback within the array of the user language?

On Friday, February 12, 2016, Jeroen Dalsem notifications@github.com
wrote:

It think the amount of cache updates is not that frequent.. we can cache
based on the unique combination of languages md5('en-nl-fr').. there are
not that many unique combinations on an average site...


Reply to this email directly or view it on GitHub
#9375 (comment).

Contributor

hypeJunction commented Feb 12, 2016

Can't we automatically merge non-site language keys with site language keys
and English when we cache it. So instead of falling back in code, we would
have the fallback within the array of the user language?

On Friday, February 12, 2016, Jeroen Dalsem notifications@github.com
wrote:

It think the amount of cache updates is not that frequent.. we can cache
based on the unique combination of languages md5('en-nl-fr').. there are
not that many unique combinations on an average site...


Reply to this email directly or view it on GitHub
#9375 (comment).

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Feb 12, 2016

Member

These ideas sound great, but should be fleshed out in #4389.

Member

mrclay commented Feb 12, 2016

These ideas sound great, but should be fleshed out in #4389.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Mar 10, 2016

feature(i18n): adds a more useful mode for detecting translation exis…
…tence

By default, `elgg_echo($key)` checks for the translation in the current language
as well as fallback languages (just English for now).

`elgg_language_key_exists($key, '')` (note the empty string) now checks the same
languages, which is probably the behavior the user expects.

`elgg_echo` and `elgg_language_key_exists` can no longer get `false` as the
default language in the case that no language is set anywhere.

In `Translator`, `getLanguage` is now `detectLanguage`, which seems more apt.

Docs cleanup.

Fixes Elgg#9375
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay
Member

mrclay commented Mar 10, 2016

PR #9472

@mrclay mrclay changed the title from correct elgg_language_key_exists() usage is messy to Typical elgg_language_key_exists() usage creates bugs Mar 17, 2016

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 18, 2016

Member

How about we just note in the docs that devs should always make sure an en translation exists when adding a key? That makes elgg_language_key_exists($key) effectively safe usage.

Member

mrclay commented Mar 18, 2016

How about we just note in the docs that devs should always make sure an en translation exists when adding a key? That makes elgg_language_key_exists($key) effectively safe usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment