We want to store configuration translations. To that end, we suggest (note that Option A is a link to the relevant issue, this is Option C):

Characteristic OPTION A OPTION C
Storage Translatable config keys are list of values with a key designating the original key. Language is not specifically the target for this feature.
Set $config->set() method gets generic subkey argument to make it easier to set a specific subkey (language) value.
Get $config->get() method gets generic subkey argument to access subkeys direct. Fallback not defined, and if subkey is not passed, the fallback logic would need to be inside the CMI system (or get() would need to return an array, similar to C and an outside wrapper would be needed). $config->get() will return the list as-is (no additional argument). t() proposed as wrapper to pick from the list (or individual values can be accessed from the array without the wrapper).

(table compiled by Gabor Hojtsy, thanks much)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review
chx’s picture

FileSize
3.44 KB

That sets at the wrong level. What we want is #1613350-3: Multlingual/translatable configuration [OPTION A]:

category:
  _language: en
  ar: '....'
  en: 'Website feedback'
  fr: '....'
  de: '....'
chx’s picture

FileSize
3.38 KB

Slight simplification because we know _default cant be empty.

chx’s picture

FileSize
3.92 KB

And docs. It's funny to see $subkey doxygen sandwiched between two @todos.

Gábor Hojtsy’s picture

Title: Reuse t() for i18n config() » Multilingual/translatable configuration reusing t() for i18n config() [OPTION C]
Issue tags: +Configuration system, +D8MI, +sprint

Marking up better as OPTION C for #1448330: [META] Discuss internationalization of configuration, which has also has #1613350: Multlingual/translatable configuration [OPTION A] and #1616594: META: Implement multilingual CMI.

Per @chx:

i would leave Option A option as a "we are looking to complete this solution inside CMI" and Option C "This complets Option A outside of CMI"

Status: Needs review » Needs work

The last submitted patch, option_c.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
4.75 KB

Seriously? We run reflection against t() in a test? *Sigh*

Anonymous’s picture

Status: Needs review » Needs work

currently, ->set($key, $value) can take an array. i think this patch will only work where $value is a scalar.

it's unclear what the implementation should be if you pass $subkey and pass an array as $value.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Just wanted to note that we discussed this option with @chx and it satisfies the base D8MI requirements (just like option A and B did). We can tell what was the original submission language for something and we can store translations and retrieve them. Pretty basic requirements, right? :)

Due to the reuse of t(), this might or might not become very popular :) The translation template extractor (potx module) used behind localize.drupal.org needs to be updated in this case to not throw warnings for config ->get() calls wrapped in t() for dynamic values, since this would then allow dynamic values. Thinking that data from intermediate sources (not just direct ->get() calls) will be passed in like this, the false positive warnings might increase. The patch also changes how t() and st() pair up in terms of functionality and also Drupal.t() vs. t(). An option is to introduce a ct() (and Drupal.ct(), for config translation) to offer this t()-like functionality. That would indeed mean developers would need to look when they use t() or ct() but there would already be a differentiation in the patch between 'don't use dynamic data with t() except if its a config language array'. Sounds like a separate wrapper implemented for config translation only would need to use minimal of the t() functionality, so could be its own function too.

I don't have strong feelings either way, leaving that to others :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Cross-post.

chx’s picture

Status: Needs work » Needs review

Nope, the CnR is the correct state. And also, you can store whatever you want , this patch perfectly works for scalars, arrays and whatever else your YAML parser/dumper supports. We merely push the value one key down we do not change the value in any way or form.

Edit: if people want ct() just tell me. I can roll that in two seconds, I dont care.

Anonymous’s picture

yep, #8 is wrong.

module devs can take structured config data, and just iterate over it until they hit scalar values, calling $config->set('some.key.blah', $yay_this_is_scalar, 'fr'); on each one. no problem.

sun’s picture

Status: Needs review » Closed (won't fix)

t() (locale()) is not suitable for user input.

That is because all string translations are lost when the original source string changes.

Human-readable strings in configuration are user input.

Gábor Hojtsy’s picture

@sun: looks like you have not looked at the patch?

Gábor Hojtsy’s picture

Status: Closed (won't fix) » Needs review
bojanz’s picture

This feels a bit messy and I see no reason to misuse t() here. We're language aware when it comes to everything else (for example, entities) so I see no reason why we should hide config language behind magic.
So, A > C. This is purely subjective, of course.

sun’s picture

Agreed. This overloads t() with completely different semantics and behavior.

The phpDoc for t() would have to be changed into two entirely different sections, like this:

/**
 * Translates human-readable strings yadayada.
 *
 * If you pass a string as input, then it works like this:
 *
 *   [...]
 *   Important: Do not pass user defined input strings into t(). All
 *   translations of such strings will be lost as soon as the source
 *   string changes.
 *   [...]
 *   [...]
 *
 * But if you pass an array as input, then it works like this:
 *
 *   [...]
 *   Note: Contrary to when passing a string as $input, when passing an
 *   array as $input, it is legit to use t() for user defined strings.
 *   [...]
 *   [...]
 *
 * That is, because we thought it would be a good idea to provide you
 * one function that performs entirely different things based on the
 * input you provide.
 */
Gábor Hojtsy’s picture

@sun: yeah, the real question is whether we want to overload t() "for the sake of simplicity for developers" or want to introduce a separate wrapper, which would be cleaner API but more to learn / understand.

chx’s picture

Guys, A have no patch and I do not even know how it could have one. We can make a ct() wrapper if you like that more but I think that so far we were telling people "if you have multilanguage strings pass them through t() and you dont need to worry about anything else" now we are telling them "if you have multilanguage strings or config pass them through t() and you dont need to worry about anything else" .

webchick’s picture

Part of me really likes using t() for this, because for developers who don't regularly deal with i18n but try to be good citizens, they do indeed think "if you have multilingual strings pass them through t() and you dont need to worry about anything else" and I see t() being used on dynamic strings all the time, despite Gábor and others' pleas not to do so. It doesn't help that core is inconsistent about this; e.g. menu titles/descriptions are dynamic strings passed through t(). Adding another function ct() seems like a really terrible thing to do DX-wise, and is likely to just result in more confusion around the multilingual system. Like right now, I'm pretty sure no one uses st() unless they have a bug report; this would be the same. :\

The other part of me agrees with sun in #17 and the thought of how to write that PHPDoc hurts my brain.

I'm not sure which part wins. :)

chx’s picture

#20 did you realize that the doxygen is already written?

Gábor Hojtsy’s picture

A short comparison between option A and C, since they are so similar and at the same time different :)

Characteristic OPTION A OPTION C
Storage Translatable config keys are list of values with a key designating the original key. Language is not specifically the target for this feature.
Set $config->set() method gets generic subkey argument to make it easier to set a specific subkey (language) value.
Get $config->get() method gets generic subkey argument to access subkeys direct. Fallback not defined, and if subkey is not passed, the fallback logic would need to be inside the CMI system (or get() would need to return an array, similar to C and an outside wrapper would be needed). $config->get() will return the list as-is (no additional argument). t() proposed as wrapper to pick from the list (or individual values can be accessed from the array without the wrapper).
chx’s picture

FileSize
7.57 KB

I took a stab at the long part of the doxygen because Gabor tells me that's the hard part. It's mostly re-indented so here are the new parts: The t() function serves three purposes (was two). Second, also at run-time it picks the correct language version of translateable configuration. * You should never use t() to translate variables except config objects.

Easy.

clemens.tolboom’s picture

The issue summary could use some more balls :/

Just a quote from @Gábor Hojtsy @ #drupal-i18n (and a review reminder)

this is basically how all config translations would be stored / accessed (think contact forms, site name, role names, field config, views, rules, and so on and on)

C could be linked to A then explain the differences

chx’s picture

Issue summary: View changes

copied the comparison table up

chx’s picture

Issue summary: View changes

Linked Option A

chx’s picture

Status: Needs review » Closed (won't fix)

I doubt we need this any more now that Option A has a nice getter.

chx’s picture

Status: Closed (won't fix) » Needs review
FileSize
7.57 KB

In fact the discussion lead to serious problems with Option A so I am reopening this with added tests. (This still does not make either Option A or Option C "pre-alpha prototype material". Both are core-ready patches. Just Option A has more concerns than this one.)

chx’s picture

FileSize
9.8 KB

Except the patch missed the tests. Sigh.

Schnitzel’s picture

I tried to think about this in a developer using way:

When I understand the code correct, the developer which creates the config has to decide if he wants the value translatable or not?

So if I write:

$config->set('testkey', 'some value');

It can never be translated?
So if I want to be able that sitebuilders can translate it I would need to write:

$config->set('testkey', 'some value', 'en');

This is kinda "bad" because the developer sometimes does not know that his config values can be good for translation.
For example the frontpage value, which can be "translated" today with the variables module. With this "translation" I can create different frontpages for different languages. I know this is not correctly translating, but it is used today for this. Or should this be done in the future with some other CMI overwriting possibilities?

If we keep this functionality, we shouldn't developers allow to write other languages then english in the code.
Like we do today in t(), we tell the developers: Write every string in t() and write it english. If we allow him now to define the language the config string is in, some developers could start writing everything in German, which then makes it hard to translate this to other languages if the translator does not understand german.

But I have right now no solution how to to this in code.

Gábor Hojtsy’s picture

@Schnitzel: yeah, the idea is that the code needs to be aware of this. Otherwise its not really possible to go in and replace a string value with an array value transparently for a module. The module needs to know when it displays the value whether to show the original language, or display it in content language (eg. for a field setting) or interface language (eg. for site name), etc. There is really some know-how that we need to transfer to people and will need to explain. I don't think we can get around this much, however clever we are. It will show up in some form or another. All options specify this context in some way, they are just doing it differently :)

effulgentsia’s picture

If choosing between A and C, I'm leaning more towards A, though I want to look at that patch more and will likely have feedback on it.

However, if this option proves most popular, then at the least I agree with bojanz and sun that we need a different function than t().

Re #20:

for developers who don't regularly deal with i18n but try to be good citizens, they do indeed think" if you have multilingual strings pass them through t() and you dont need to worry about anything else" and I see t() being used on dynamic strings all the time, despite Gábor and others' pleas not to do so.

We need to fix this by fixing the places in core (e.g., menu titles) that create this confusion, and then document very clearly that t() should never be called on user input. Part of Drupal 8 is making it so that all user input is either entity content or config. t() is for translating strings that are shipped with code. Entity content and config values need their own systems. Would be great to have each system's i18n handling be easy to use on its own and as consistent as we can make it between code strings, entity content, and config values, but I don't think overloading t() is the best way to achieve that.

It doesn't help that core is inconsistent about this; e.g. menu titles/descriptions are dynamic strings passed through t().

It most certainly does not help. IMO, that we do this is a critical bug on its own. Getting menus into CMI will let us stop this insanity.

Adding another function ct() seems like a really terrible thing to do DX-wise

Agreed. That's why I'm more in favor of option A if we can get the DX of that smoothed out.

Gábor Hojtsy’s picture

Status: Needs review » Closed (duplicate)

As per our discussion in Barcelona with heyrocker (CMI), webchick (core), merlinofchaos (views), reyero (i18n), webflo (i18nviews), Gábor Hojtsy (D8MI), xjm (views) and others, we are going with a base implementation of option B for now. In short we figured out we have a need for almost anything to be translatable/multilingual, we need some context information passed around and we need to make the system extensible due to the definite lack of time to solve all problems until code freeze, and all of those criteria lead us to work with option B. Let's focus our efforts on getting it done best!

Marking duplicate of #1616594: META: Implement multilingual CMI on the grounds of same problem space being solved.

Gábor Hojtsy’s picture

Issue tags: -sprint +language-config

Removing sprint tag to keep focus on where it's at.

Gábor Hojtsy’s picture

Issue summary: View changes

added credit