Problem/Motivation

Steps to reproduce:
1. Call LocaleConfigManager::getStringTranslation with new source string.
2. A new translation object for that source is returned.
3. Save the translation for this.
4. Call it again with the same new source string.
5. Again a new translation object for that source is returned with the same ID.
6. When trying to save this, you get a duplicate key exception.

For example, that happens when translating a view that has the same source string multiple times.

Proposed resolution

Update the static cache with the new object. Then you get the existing, no longer new translation object.

Remaining tasks

Writing tests.

There is also a bigger problem. It is not possible to save multiple translations of the same source string in locale. For those views, the last identical source string will win and only that translation is stored.

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#19 multiple_calls_to-2460259-19.patch2.3 KBsasanikolic
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,465 pass(es). View
#15 multiple_calls_to-2460259-12.patch2.51 KBsasanikolic
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,457 pass(es). View
#15 multiple_calls_to-2460259-11-test-only.patch1.66 KBsasanikolic
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,464 pass(es), 1 fail(s), and 1 exception(s). View
#12 multiple_calls_to-2460259-12.patch2.51 KBsasanikolic
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,462 pass(es). View
#11 multiple_calls_to-2460259-11.patch1.66 KBsasanikolic
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,464 pass(es), 1 fail(s), and 1 exception(s). View
#8 2460259-8-test.patch1.32 KBameenkhan07
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,591 pass(es), 0 fail(s), and 2 exception(s). View
#2 multiple_calls_to-2460259-2.patch870 bytessasanikolic
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,583 pass(es). View

Comments

sasanikolic’s picture

Priority: Normal » Major
Issue tags: +Needs tests
sasanikolic’s picture

Status: Active » Needs review
FileSize
870 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,583 pass(es). View
Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint

The fix totally makes sense to me. That would let our changes from the returned object keep being in the cache. Needs tests indeed.

ameenkhan07’s picture

Assigned: Unassigned » ameenkhan07

I have begun on testing, i'm a novice at testing so need wee bit time to get a hold of reins :)

ameenkhan07’s picture

sasanikolic: Could you perhaps elaborate on this more?
"There is also a bigger problem. It is not possible to save multiple translations of the same source string in locale. For those views, the last identical source string will win and only that translation is stored."

Gábor Hojtsy’s picture

@ameenkhan07: that should not be resolved here. @Berdir asked me to open an issue to document it. See #2460823: Document that locale + config translation integration treats string uniqueness the same way as locale itself to discuss.

Gábor Hojtsy’s picture

ameenkhan07’s picture

FileSize
1.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,591 pass(es), 0 fail(s), and 2 exception(s). View

Need feedback as well

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks for working on this.

  1. +++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
    @@ -40,4 +40,21 @@ public function testHasTranslation() {
    +  public function testGetStringTranslation(){  ¶
    +    $this->installSchema('locale', array('locales_location', 'locales_source', 'locales_target'));
    +    $this->installConfig(array('locale_test'));  ¶
    + ¶
    

    Several lines have whitespace issues. (Not just these).

  2. +++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
    @@ -40,4 +40,21 @@ public function testHasTranslation() {
    +    $result = $locale_config_manager->getStringTranslation('locale_test.no_translation', $language->getId(),'test string', LocaleStringTest::randomMachineName(20));
    ...
    +    $result = $locale_config_manager->getStringTranslation('locale_test.no_translation', $language->getId(),'test string', LocaleStringTest::randomMachineName(20));
    

    Look at the no_translation file, it has "Test" as the only string. It does not have a source string "test string". You should use the correct source string.

    Also the string does not have a context, you should not pass a context, that will not work either.

  3. +++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
    @@ -40,4 +40,21 @@ public function testHasTranslation() {
    +    $result->save();  ¶
    ...
    +    $result->save();  ¶
    

    Would save() do something useful without setting a translation? In LocaleConfigManager::saveCustomizedTranslation() we set a string and then save. That would be better.

  4. +++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
    @@ -40,4 +40,21 @@ public function testHasTranslation() {
    +    $this->assertTrue($result, 'just testing');
    

    What is being tested here? If the above should not throw an exception that is already tested implicitly. The test would fail in case of an exception.

The last submitted patch, 8: 2460259-8-test.patch, failed testing.

sasanikolic’s picture

FileSize
1.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,464 pass(es), 1 fail(s), and 1 exception(s). View

Here is the test, based on the steps written in the issue description.

sasanikolic’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,462 pass(es). View

And a patch with the fix and the test.

Status: Needs review » Needs work

The last submitted patch, 12: multiple_calls_to-2460259-12.patch, failed testing.

Status: Needs work » Needs review
sasanikolic’s picture

FileSize
1.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,464 pass(es), 1 fail(s), and 1 exception(s). View
2.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,457 pass(es). View

Re-upload...

The last submitted patch, 11: multiple_calls_to-2460259-11.patch, failed testing.

The last submitted patch, 15: multiple_calls_to-2460259-11-test-only.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks! Patch looks good minus a few small details:

  1. +++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
    @@ -40,4 +40,26 @@ public function testHasTranslation() {
    +  public function testGetStringTranslation(){
    

    Missing docblock on test method.

    Missing space after ()

  2. +++ b/core/modules/locale/src/Tests/LocaleConfigManagerTest.php
    @@ -40,4 +40,26 @@ public function testHasTranslation() {
    +    $translation_after2 = $locale_config_manager->getStringTranslation('locale_test.no_translation', $language->getId(), 'Test', '');
    +    $this->assertFalse($translation_after2->isNew());
    +    $translation_after2->setString('updated_translation')->save();
    

    The test is already complete without these lines, no? If you are to do this, let's at least set yet another different value.

sasanikolic’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,465 pass(es). View
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Looks good, thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 5ff53d7 and pushed to 8.0.x. Thanks!

  • alexpott committed 5ff53d7 on 8.0.x
    Issue #2460259 by sasanikolic, ameenkhan07: Multiple calls to...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Status: Fixed » Closed (fixed)

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