Problem/Motivation

  1. Install Interface Translation module and make a non-English language the default language
  2. Visit /user/1 and check the Member for output
  3. Visit /admin/config/regional/translate and translate the singular and plural date part strings shown. (E.g. if the Member for output is 3 weeks 5 days, translate the 1 week and @count weeks strings.
  4. Visit /user/1. Note that the respective string is not translated!
  5. Visit /admin/config/regional/translate. Notice that the singular and plural date part strings are now duplicated!
  6. Repeat this process infinitely to have an infinite number of identical strings in the database, albeit the output never being translated!

A screenshot of the Interface Translation form, with multiple instances of Singular form and Plural form form elements for the 1 year and @count years strings. All elements but the last to are translated to 1 Jahr and @count Jahre, respectively.

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3.78 KB

Here'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...

tstoeckler’s picture

Please 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.

Status: Needs review » Needs work

The last submitted patch, 2: 2600396-2.patch, failed testing.

xjm’s picture

I'd be in favor of making this an RC target -- pinged other committers for a second opinion. Yay for infinite database records. :P

xjm’s picture

Can 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. =/

effulgentsia’s picture

Issue tags: -rc target triage +rc target

I 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.

mpdonadio’s picture

Gábor Hojtsy’s picture

Can/should formatPlural() itself ignore a NULL for langcode? That would ultimately solve this for any calling code, not just this one.

tstoeckler’s picture

The 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.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
833 bytes

#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?

skyredwang’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -134,6 +134,11 @@ public function translateString(TranslatableMarkup $translated_string) {
     // Merge in options defaults.
     $options = $options + [
       'langcode' => $this->defaultLangcode,
 

It looks like the "+" operator has already added the default langcode, although, I am not sure how PHP handling NULL in this case.

skyredwang’s picture

I 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.

jhodgdon’s picture

Yeah, 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.

justAChris’s picture

Adds 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.

The last submitted patch, 15: 2600396-15-test-only.patch, failed testing.

mpdonadio’s picture

Gá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.

Gábor Hojtsy’s picture

I would have personally done:

// If the language code was set but empty, remove it.
if (isset($options['langcode']) && empty($options['langcode'])) {
  unset($options['langcode']);
}

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.

tstoeckler’s picture

Re #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).

mpdonadio’s picture

Here is a unit test. Assuming this comes back green (unit test does locally), I think it is ready for final review.

Gábor Hojtsy’s picture

@tstoeckler: right, you are right.

The last submitted patch, 20: 2600396-test-only.patch, failed testing.

jhodgdon’s picture

Would it make sense to do:

if (array_key_exists('langcode', $options) && empty($options['langcode'])) {

rather than the explicit === NULL check? Or maybe something like !is_string() ?

jhodgdon’s picture

And does this test cover both this report and the other issue that was marked as duplicate?

mpdonadio’s picture

#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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

OK. 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.)

The last submitted patch, 15: 2600396-15-test-only.patch, failed testing.

The last submitted patch, 20: 2600396-test-only.patch, failed testing.

Matze202’s picture

Thanks @tstoeckler for the post from bug. ;)

Thanks @mpdonadio, @justAChris, @tstoeckler, @skyredwang for the patches from the bug.

catch’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -134,6 +134,11 @@ public function translateString(TranslatableMarkup $translated_string) {
+    if (array_key_exists('langcode', $options) && $options['langcode'] === NULL) {

I'd normally do this as:

if (!isset($options['langcode'] && array_key_exists($options['langcode'])

mostly micro-optimization.

Adding commit credits.

catch’s picture

Status: Reviewed & tested by the community » Needs review

CNR for #30 - if you feel strongly it should stay as is, bump back to RTBC, #20 looks fine otherwise.

The last submitted patch, 15: 2600396-15-test-only.patch, failed testing.

The last submitted patch, 20: 2600396-test-only.patch, failed testing.

mpdonadio’s picture

catch’s picture

Status: Needs review » Reviewed & tested by the community

The last submitted patch, 34: 2600396-test-only.patch, failed testing.

The last submitted patch, 34: 2600396-test-only.patch, failed testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 33a1baa on 8.0.x
    Issue #2600396 by mpdonadio, justAChris, tstoeckler, skyredwang,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all!

  • catch committed 33a1baa on 8.1.x
    Issue #2600396 by mpdonadio, justAChris, tstoeckler, skyredwang,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.