Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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)
Comment | File | Size | Author |
---|---|---|---|
#27 | 1617334_26.patch | 9.8 KB | chx |
#26 | 1617334_26.patch | 7.57 KB | chx |
#23 | 1617334_23.patch | 7.57 KB | chx |
#7 | option_c.patch | 4.75 KB | chx |
#4 | option_c.patch | 3.92 KB | chx |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
chx CreditAttribution: chx commentedThat sets at the wrong level. What we want is #1613350-3: Multlingual/translatable configuration [OPTION A]:
Comment #3
chx CreditAttribution: chx commentedSlight simplification because we know _default cant be empty.
Comment #4
chx CreditAttribution: chx commentedAnd docs. It's funny to see $subkey doxygen sandwiched between two @todos.
Comment #5
Gábor HojtsyMarking 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:
Comment #7
chx CreditAttribution: chx commentedSeriously? We run reflection against t() in a test? *Sigh*
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedcurrently, ->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.
Comment #9
Gábor HojtsyJust 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 :)
Comment #10
Gábor HojtsyCross-post.
Comment #11
chx CreditAttribution: chx commentedNope, 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.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedyep, #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.
Comment #13
sunt() (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.
Comment #14
Gábor Hojtsy@sun: looks like you have not looked at the patch?
Comment #15
Gábor HojtsyComment #16
bojanz CreditAttribution: bojanz commentedThis 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.
Comment #17
sunAgreed. 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:
Comment #18
Gábor Hojtsy@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.
Comment #19
chx CreditAttribution: chx commentedGuys, 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" .
Comment #20
webchickPart 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. :)
Comment #21
chx CreditAttribution: chx commented#20 did you realize that the doxygen is already written?
Comment #22
Gábor HojtsyA short comparison between option A and C, since they are so similar and at the same time different :)
Comment #23
chx CreditAttribution: chx commentedI 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.
Comment #24
clemens.tolboomThe issue summary could use some more balls :/
Just a quote from @Gábor Hojtsy @ #drupal-i18n (and a review reminder)
Comment #24.0
chx CreditAttribution: chx commentedcopied the comparison table up
Comment #24.1
chx CreditAttribution: chx commentedLinked Option A
Comment #25
chx CreditAttribution: chx commentedI doubt we need this any more now that Option A has a nice getter.
Comment #26
chx CreditAttribution: chx commentedIn 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.)
Comment #27
chx CreditAttribution: chx commentedExcept the patch missed the tests. Sigh.
Comment #28
Schnitzel CreditAttribution: Schnitzel commentedI 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:
It can never be translated?
So if I want to be able that sitebuilders can translate it I would need to write:
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.
Comment #29
Gábor Hojtsy@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 :)
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedIf 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:
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 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.
Agreed. That's why I'm more in favor of option A if we can get the DX of that smoothed out.
Comment #31
Gábor HojtsyAs 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.
Comment #32
Gábor HojtsyRemoving sprint tag to keep focus on where it's at.
Comment #32.0
Gábor Hojtsyadded credit