fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

[BUG] \Lang::get() does not use the fallback language. #1860

Closed rickmacgillis closed 9 years ago

rickmacgillis commented 9 years ago

The Problem When a line or file does not exist for the specified language, \Lang::get() should use the fallback language, and it doesn't. That causes empty strings or default text to show.

The Solution Change the method to read as follows. (I've tested it.)

    public static function get($line, array $params = array(), $default = null, $language = null)
    {
        ($language === null) and $language = static::get_lang();

        if (
            (isset(static::$lines[$language]) and
            ($entry = \Arr::get(static::$lines[$language], $line, false)) !== false) ||
            (isset(static::$lines[static::$fallback[0]]) and
            ($entry = \Arr::get(static::$lines[static::$fallback[0]], $line, false)) !== false)
        ) {
            return \Str::tr(\Fuel::value($entry), $params);
        }

        return $default;
    }
emlynwest commented 9 years ago

The fallback is only used if the language file does not exist. From the docs:

You can also define a fallback language in your configuration, which is either a language code, or an array of language codes. The fallback is used whenever you load a language file, and the file does not exist for the specified language.

http://fuelphp.com/dev-docs/classes/lang.html

rickmacgillis commented 9 years ago

Alright, so how do I ensure that no matter what my script always pulls a value from my chosen language instead of having to maintain default values all over my script? The only way I can think of is to sloppily create a "lock" file for each language and check if a string in it exists, then manually switch the language to the one I want. However, that doesn't account for certain lines that may be translated, whereas other lines and files might not exist for the user selected language.

emlynwest commented 9 years ago

If there are lines or files missing for a language then your translators are not doing a good enough job :wink: Think of it like deploying your app with only half the configs or only three of ten migrations.

WanWizard commented 9 years ago

I might have to dive into this a bit more before a final conclusion.

@CozyLife might have a point here, in the current release, all language files were read into a single array, and merged. So if you had a half-translated file (or even a missing file), you were ok if you had a fallback language that was complete.

In 1.8/develop, there are separate array's per language, allowing you to keep different languages in memory at the same time, and allowing you to switch. This means merging might have changed, and might have caused get() not to find the fallback language(s) any more.

WanWizard commented 9 years ago

Looked into the code, and I agree with @stevewest here.

The Lang class allows you to have multiple languages active at the same time, but this is NOT the role of the fallback.

The fallback is designed primarily as an aid to translation. The assumption here is that the language files in your app's primary language (usually your own language) are 100% complete. This language is your fallback language (and it could also be your active language). If you set another language as your active language, Lang will load both the active and the fallback file, will merge the two, and store it as strings for the active language. This allows fallback strings to "bleed through" in case your translations aren't 100%.

This mechanism also means that you never have to use a default value for get() if you make sure you keep your primary files at 100% completion. If you need the default because of missing strings, you've missed adding the keys to your language file during development.

So there is never a need for get() to do a second lookup, any fallback language strings are already there.

rickmacgillis commented 9 years ago

@wanwizard, the purpose of my fixing the issue is that it isn't finding the backup language strings that verifiably do exist in the fallback language. Please test it to see what I mean.

Steps to Reproduce

  1. Write a language file for English and add a string for 'test'. Name it testing.php.
  2. In the main code, check the output of:
\Config:set('language', 'es');
\Lang:load('testing');
\Debug:dump (\Lang:get ('test'));
  1. You won't see the string you entered in the English file since one doesn't exist for Spanish. It'll use the default of 'null'.

EDIT I just thought of something. I'm not at my computer right now, but I think the language gets set after loading the file, so perhaps the script doesn't catch it because it had already loaded everything by the time the change takes place. In that case, I'll look into it on my end as I might need to refactor my code to get it compatible with the internal workings of FuelPHP. I'll report back later once I've checked into it.

WanWizard commented 9 years ago

Just tried it here. Configured language 'xx' in an existing app, and added one single language file in the 'xx' folder, with one single key translated.

Application starts up fine, in English ('en' is the fallback language'), and that single key translated, as expected. So the code works as advertised, and you must have a logic error in your app, like you already suspected.

rickmacgillis commented 9 years ago

You're right, it was a flaw on my end. I apologize for the inconvenience.

For anyone who reads this thread, the error is in the order of operations. FuelPHP needs to have the selected language set before it loads the language file, as it merges the files in \Lang::load(), as described above. \Lang::get() pulls from the already loaded data, but it doesn't merge anything. The way FuelPHP finds the string is a more efficient way to load the data than the one I supplied.