We cannot use objects in array keys, so if we let t() return an object we should look into either casting those to string, or into refactoring the code so that the translated strings are not in the array key anymore.
Comment | File | Size | Author |
---|---|---|---|
#29 | cast_optgroup_array-2561259-29.patch | 12.8 KB | joelpittet |
Comments
Comment #2
stefan.r CreditAttribution: stefan.r commentedComment #3
joelpittetThe context of why we need to cast these keys is lost without being part of the original issue, I think maybe leave this one in the parent issue, no?
Comment #4
stefan.r CreditAttribution: stefan.r commented@joelpittet my worry is the parent issue is complex enough as it is, while this one is easily reviewable, limited in scope and would have 0 effect if we commit it (as we're just casting strings to string).
The only thing we'd need to prove is that these array keys may hold output from t(), and find a good way to work around that.
But yes, unless the parent is bumped to critical this one likely won't go in without the parent going in as well... but we can always merge this back in there as soon as we're happy with the changes here?
Comment #5
stefan.r CreditAttribution: stefan.r commentedComment #6
stefan.r CreditAttribution: stefan.r commented/wrong issue
Comment #7
joelpittetMaybe this set of options doesn't need to be using translated strings for it's keys?
Kind of like Tasks but using better keys like run it through one of those *Identifier helpers.
Using a translation for a key on options seems problematic for UI related things.
Comment #8
stefan.r CreditAttribution: stefan.r commentedDefinitely yes! The issue title may have been a bit too specific, I'd rather rewrite the code where possible so as to not do the string cast if it doesn't break APIs
Comment #9
lauriiiLets see how hard this fails..
Comment #10
stefan.r CreditAttribution: stefan.r commentedActually none of this will work :( the keys in options arrays are for optgroups, which do have to remain translated.
Sadly I think casting to string is our only option here, if we want to maintain BC.
Interestingly this is not documented in the Form API docs...
Comment #12
stefan.r CreditAttribution: stefan.r commentedScreenshot of what this would regress to:
On the bright side, it makes the patch very reviewable -- it's just just Html::getClass() and all the rest are optgroup keys, where a cast to string is the only choice if we want to keep the label translated and we want to keep the render array.
Likely most of the disruption for contrib with objects as array keys will be around these optgroup keys considering they are 90%+ of the conversions in core as well.
Comment #13
stefan.r CreditAttribution: stefan.r commentedComment #14
stefan.r CreditAttribution: stefan.r commentedI've given this a final review and I think this all looks pretty straightforward now...
Technically this could have been refactored not to use the string casts, ie by using SplObjectStorage, but why bother when a string cast is just as easy?
Same here.
Comment #16
stefan.r CreditAttribution: stefan.r commentedUnrelated failure in BlockContextMappingUpdateFilledTest
Comment #18
lauriiiI think its a little bit sad that we have to do this (cast t functions all over the place) but I don't see there is other way than this if we are going to make t function return translation wrapper because there is no way that array keys would support objects. I tried to grep array keys using t function in during #9 so I'm hoping they are all included.
Comment #19
stefan.r CreditAttribution: stefan.r commentedI have posted a CR draft for the parent issue at https://www.drupal.org/node/2564451 based on the conversion effort in core. I think it's just array keys (treated in this issue), strpos and tests that pose a problem.
Comment #21
stefan.r CreditAttribution: stefan.r commentedSeemingly unrelated testbot fail:
Drupal\system\Tests\Update\RouterIndexOptimizationTest 346 4 0
Message Group Filename Line Function Status
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/update.... returned 0 (0 bytes). Browser UpdatePathTestBase.php 246 Drupal\system\Tests\Update\UpdatePathTestBase->runUpdates()
After all updates ran, entity schema is up to date. Other UpdatePathTestBase.php 263 Drupal\system\Tests\Update\UpdatePathTestBase->runUpdates()
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/update.... returned 0 (0 bytes). Browser UpdatePathTestBase.php 246 Drupal\system\Tests\Update\UpdatePathTestBase->runUpdates()
After all updates ran, entity schema is up to date.
Comment #23
alexpottI think optgroups might be a big problem :(
What happens if they contain HTML? Hmmm... does that even make sense. I just they never should because this is a select list thing - right?
Comment #24
stefan.r CreditAttribution: stefan.r commentedThis will likely be the most frequent "problem" in contrib if t() outputs an object, luckily it won't really be a problem at all as all they'll have to do is a simple string cast, per https://developer.mozilla.org/fr/docs/Web/HTML/Element/Optgroup the string in the array key will end up in the label attribute of the optgroup tag... which can't contain markup :)
Comment #25
stefan.r CreditAttribution: stefan.r commented@catch mentioned #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. may be a nice issue to work on so that in the CR we won't have to recommend casting optgroup array keys to string (which is otherwise perfectly fine as they're plain text so it shouldn't block this patch)
Comment #26
alexpottWhere are the t()'d things that are causing this?
Comment #27
stefan.r CreditAttribution: stefan.r commented#26: we don't need it anymore - I took it out and the TranslationWrapper tests still pass.
Comment #28
stefan.r CreditAttribution: stefan.r commentedConsidering this is 95% about optgroups it may be worth mentioning that in the title
Comment #29
joelpittetWrapping lines at 80 chars.
Still RTBC, IMO. The comment would be nice to avoid having to remove it later in the parent issue(we always forget).
Comment #30
lauriiiRTBC++
Comment #31
dawehnerNote: this conflicts with a critical also fixing it.
Comment #32
stefan.r CreditAttribution: stefan.r commented@dawehner - so let's fix whichever goes in later in a reroll or on commit? What's the issue so I can keep an eye on it?
Comment #35
stefan.r CreditAttribution: stefan.r commentedSeemingly unrelated fail in UserTokenReplaceTest.php
Comment #36
andypostso key ($optgroup_fields) get cast early but label casted at render time
this could lead to inconsistency of render between group and label when default interface langcode is different from entity language
empty() should be done on string, in case of wrapper better to check for interface
Comment #37
stefan.r CreditAttribution: stefan.r commented@andypost do we have any proof 1 could ever happen? IMO they should always both be shown in the interface language - if they dont, that sounds like a separate bug?
Comment #38
stefan.r CreditAttribution: stefan.r commentedComment #39
andypostFiled separate issue for #36.1 #2567559: Language of (optgroup) array keys and interface language at render time should be same
Comment #40
stefan.r CreditAttribution: stefan.r commentedThanks @andypost for raising the issue... if this is actually a problem it seems separate and unrelated to the current patch though, as: t('foo') === (string) t('foo') === (string) new TranslationWrapper('foo')
Comment #41
andypostYep, but in case of cached forms that renderd in other language there separate issue
Comment #42
stefan.r CreditAttribution: stefan.r commentedcool. As to the empty check - I wonder if new TranslationWrapper('') should even be possible. To me that should always return an empty string (such as with SafeString).
So I think we're good there? If the object is set, it'll be non-empty and we go ahead...
Comment #44
stefan.r CreditAttribution: stefan.r commentedThis only left RTBC state because of an unrelated test fail - #36 had further feedback but this shouldnt hold this up. Anyone willing to give this a final review?
Comment #45
lauriiiI think both comments in #36 has been addressed.
Comment #46
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe "once issue #... lands" part of this comment (and all its duplicates) looks really weird. Why do we need to mentioned something that may or may not happen in the future?
Comment #47
stefan.r CreditAttribution: stefan.r commented@amateescu the plan was to take it out once it gets in (or not), but I do agree, can you suggest better wording?
This patch is meaningless without the parent, but merging this in there just adds noise.
Comment #48
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWell, if we're sure the parent issue will land, I suggest to simply take out that part of the comment(s) :)
Comment #49
lauriiiNow that the parent issue is critical I agree with @amateescu that we shouldn't add the documentation and can be pretty sure that it will land sooner or later.
Comment #51
alexpottLet's just remove the comments in #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list. Since atm the string casts are necessary but once that lands they are necessary. As for #36.1 won't #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. fix this?
Comment #52
alexpottCommitted df7ced5 and pushed to 8.0.x. Thanks!