Problem/Motivation
- Install Interface Translation module and make a non-English language the default language
- Visit
/user/1
and check the Member for output - Visit
/admin/config/regional/translate
and translate the singular and plural date part strings shown. (E.g. if the Member for output is3 weeks 5 days
, translate the1 week
and@count weeks
strings. - Visit
/user/1
. Note that the respective string is not translated! - Visit
/admin/config/regional/translate
. Notice that the singular and plural date part strings are now duplicated! - Repeat this process infinitely to have an infinite number of identical strings in the database, albeit the output never being translated!
This bug is being caused by DateFormatter::formatDiff()
having a (nonsensical) default of NULL
for the langcode, which is passes on to TranslationInterface::formatPlural()
. This eventually ends up in TranslationManager::doTranslate()
which does
// Merge in options defaults.
$options = $options + [
'langcode' => $this->defaultLangcode,
'context' => '',
];
but the NULL
langcode prevents the default language from being merged. Thus, the langcode = NULL
ends up as a database condition in StringStorageInterface::findTranslation()
, so that the existing translations (and source strings) will never be found and new ones will be created.
Proposed resolution
Fix DateFormatter::formatDiff()
to no longer pass along a bogus ['langcode' => NULL]
.
Remaining tasks
User interface changes
The Member for output can be translated!
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff-20-34.txt | 744 bytes | mpdonadio |
#34 | 2600396-34.patch | 5.75 KB | mpdonadio |
#34 | 2600396-test-only.patch | 4.97 KB | mpdonadio |
#20 | 2600396-20.patch | 5.75 KB | mpdonadio |
Comments
Comment #2
tstoecklerHere's a fix. We should write some tests for this. The steps to reproduce are hopefully clear enough for someone to write a web test. I'm really crossing my fingers that we have a unit test that fails with this patch...
Comment #3
tstoecklerPlease also credit https://www.drupal.org/u/matze202 for this. He was uncomfortable opening the bug report himself, because he does not speak English well, but he discovered this very obscure bug and should certainly be credited. Fixing the bug was probably a lot easier than finding it...
I will ping him to comment in the issue, so he can be assigned credit on Drupal.org as well.
Comment #5
xjmI'd be in favor of making this an RC target -- pinged other committers for a second opinion. Yay for infinite database records. :P
Comment #6
xjmCan matze202 post a comment on the issue so we can ensure issue credit? Unfortunately we cannot add someone to the issue credit without a comment yet. =/
Comment #7
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI agree with #5 about this being an appropriate fix for the RC phase, especially if the bug is interfering with getting translations completed on localize.drupal.org.
Comment #8
mpdonadioThis behavior was added in #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known. Need to read through there to see why
'langcode' => NULL,
was added.Comment #9
Gábor HojtsyCan/should formatPlural() itself ignore a NULL for langcode? That would ultimately solve this for any calling code, not just this one.
Comment #10
tstoecklerThe theoretical purist in me says that shouldn't be necessary, because passing NULL for language code doesn't really make sense at all, but - sure - it is just as well possible to (also) adapt the code referenced in the issue summary to account for a (bogus) NULL value. I'm interested to hear what others - specifically @mpdonadio - say about this.
Comment #11
mpdonadio#9, I think that may be the better option.
As far as I can tell, this bug was introduced when the formatDiff() functions where introduced. To parallel what formatInterval() did, the default langcode was set to NULL.
A quick grep shows lots of places where a NULL langcode can creep in. I think this may be best?
Comment #12
skyredwangIt looks like the "+" operator has already added the default langcode, although, I am not sure how PHP handling NULL in this case.
Comment #13
skyredwangI read the NULL handling here: http://stackoverflow.com/questions/2140090/operator-for-array-in-php#com... , so this patch might be just a little bit cleaner.
Comment #14
jhodgdonYeah, patch #10 or #13 (same effect) seems like a good idea. We still need a test for the bug though, and presumably for the related issue as well.
Comment #15
justAChris CreditAttribution: justAChris as a volunteer commentedAdds a web test based on steps to reproduce in issue summary. Tests the duplication of the the interface translation input fields as well as the translation in "Member for" info on the user page.
Attempts to address test for related issue by checking the user page before interface text is updated. The test sets the default language instead of doing an install with a non-english language though, so not exactly as described in #2606046: DateFormatter does not assume default langcode correctly
Uses patch in comment 13 for combined patch.
Comment #17
mpdonadioGábor, are you OK with this approach, or should we do this (or something similar) in TranslationManager::getStringTranslation() instead, which is a public function on the interface? You hinted at this in IRC, but I wasn't able to followup then. That should catch all cases, and not just this DateFormatter one.
Also, the integration test is great, but this should have been caught by TranslationManagerTest, so we need to add coverage there.
Comment #18
Gábor HojtsyI would have personally done:
That also rules out other possibly bugos cases. (Also faster than array_key_exists). Not sure this is the best place of all the indirections that translation is going through now, but it looks very logical given the default merged in just after.
Comment #19
tstoecklerRe #18: That will not work, because isset() yields FALSE on a NULL array key. That's why array_key_exists() is needed (at least in this particular code flow).
Comment #20
mpdonadioHere is a unit test. Assuming this comes back green (unit test does locally), I think it is ready for final review.
Comment #21
Gábor Hojtsy@tstoeckler: right, you are right.
Comment #23
jhodgdonWould it make sense to do:
rather than the explicit === NULL check? Or maybe something like !is_string() ?
Comment #24
jhodgdonAnd does this test cover both this report and the other issue that was marked as duplicate?
Comment #25
mpdonadio#24, lines 165 and 201 of LocalePluralFormatTest should cover the other issue.
#23, I had a few version of that written before settling on the `=== NULL` check. I decided on that, as it is was what core was setting defaults to. If we go down the route of empty() or is_string(), then I feel like we are opening a can of worms and rescoping the issue. Instead of just setting a default, I feel like we would start to move into validating the input, which means adjusting the default logic, exceptions, etc. Perhaps, we just update the docblock on doTranslate() to say that if a NULL langcode is provided, the default will be used.
I also just had a spurious local fail with this when the execution time passed the 60 second mark. I think in that test we need to fudge the user creation time to guarantee a static number.
Comment #26
jhodgdonOK. I've looked carefully over the code change and the tests, and I agree that this patch (a) is what we should be doing and (b) has tests that cover both this reported bug and the related/duplicate issue's reported bug. Gábor also seems to be agreeing with the approach. So, I think we should get this in. Soon. :)
Thanks for the quick work all! (Adding credit to Gábor for providing crucial direction on this.)
Comment #29
Matze202 CreditAttribution: Matze202 commentedThanks @tstoeckler for the post from bug. ;)
Thanks @mpdonadio, @justAChris, @tstoeckler, @skyredwang for the patches from the bug.
Comment #30
catchI'd normally do this as:
if (!isset($options['langcode'] && array_key_exists($options['langcode'])
mostly micro-optimization.
Adding commit credits.
Comment #31
catchCNR for #30 - if you feel strongly it should stay as is, bump back to RTBC, #20 looks fine otherwise.
Comment #34
mpdonadioNo strong opinion either way.
Comment #35
catchComment #38
catchCommitted/pushed to 8.0.x, thanks!
Comment #40
Gábor HojtsyThanks all!