Multiple calls to LocaleConfigManager::getStringTranslation can return multiple new translation objects for the same source

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

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

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

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

Need feedback as well

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.

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.

Looks good, thanks.

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!

Yay, thanks!