[Translation] correctly handle intl domains with TargetOperation #42361

acran · 2021-08-03T23:24:26Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

Executing translations:update with --clean was producing erratic result: creating duplicate non-/intl files for domains, removing used translations...

TL;DR: judging from context this was most probably just a typo in 33e6af5

How to reproduce

Setup Test Project

A fresh minimal project to verify this can be setup with

composer create-project symfony/skeleton translations_test
cd translations_test/
composer require translation

and adding extractable translations into a file in src/, e.g.

<?php
// in src/translations.php
use Symfony\Component\Translation\TranslatableMessage;

new TranslatableMessage('First Message');
new TranslatableMessage('Second Message');

Otherwise any existing project should work as well.

Steps to reproduce

Case 1: duplicate translation files

# remove all existing translations and extract them again
rm -rf translations/*
php bin/console translation:update --force --clean en

# verify translations/messages+intl-icu.en.xlf was created containing the messages

# extract messages again
php bin/console translation:update --force --clean en

# messages+intl-icu.en.xlf is unchanged but messages.en.xlf with all the same messages was added

Case 2: lost translations

# remove all existing translations and extract them again
rm -rf translations/*
php bin/console translation:update --force --clean en

# verify translations/messages+intl-icu.en.xlf was created containing the messages
# remove one or more messages from messages+intl-icu.en.xlf, keeping at least one

# extract messages again
php bin/console translation:update --force --clean en

# messages+intl-icu.en.xlf will *only* contain the missing/"new" messages and all other messages got removed

The Fix

The previous code was trying to merge with the target messages instead of the source. This was probably just a typo since $keyMetadata is take from source just below it and the foreach block below which is basically doing the same inverse just switches target and source.
Also the code is mostly identical to MergeOperation which also uses source at the respective line.

The code in both files was introduced in 33e6af5

…Commands (acran) This PR was merged into the 5.4 branch. Discussion ---------- [FrameworkBundle] remove dead conditions in Translation Commands | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - This is just a trivial removal of unused code I stumbled upon while debugging #42361. In the [original code](https://github.com/symfony/symfony/blob/e617a9b/src/Symfony/Bundle/FrameworkBundle/Command/TranslationDebugCommand.php#L165-L170): ~~~php $transPaths = [$path.'/translations']; $codePaths = [$path.'/templates']; if (!is_dir($transPaths[0]) && !isset($transPaths[1])) { throw new InvalidArgumentException(sprintf('"%s" is neither an enabled bundle nor a directory.', $transPaths[0])); } ~~~ The second part of the condition `isset($transPaths[1])` will **always** evaluate to true, since `$targetPath` is just set 3 lines above but only has a single element. This check was originally to support legacy paths which was removed in b6eb1f4: * in [`TranslationDebugCommand.php`](b6eb1f4#diff-67afa5b8860d0df4e44f1e1fc89f444b7ac77de515b698a6824dd5403a0acdbcL187-L194) * in [`TranslationUpdateCommand.php `](b6eb1f4#diff-a01c7858e84f1868a427634740511da7c8c73e56772baa78bdcd98200d7125c0L180-L187) Rebased from 5.3 to 5.4, see #42362 /cc `@fabpot` Commits ------- 22db5ad [FrameworkBundle] remove dead conditions in Translation Commands

acran · 2021-08-11T12:06:47Z

/cc @yceruto

fabpot · 2021-08-26T06:16:02Z

@acran Could you add a unit test?

This would cause issues with merging existing intl domains since it was trying to merge with the target instead of the source. This was originally introduced with c71dfb9 The incorrect Test was introduced with b72b7d3

acran · 2021-09-02T12:31:06Z

@acran Could you add a unit test?

I can try 🤓
But I'll need probably some feedback on this.

Also digging a bit further in this I found though 33e6af5 was the commit that introduced the offending line it was actually just the result of refactorings and code style fixes and the problem was introduced in c71dfb9. Looking at that commit it definitely smells like a copy&paste mistake.

acran · 2021-09-02T12:39:27Z

src/Symfony/Component/Translation/Tests/Catalogue/TargetOperationTest.php

@@ -71,13 +71,45 @@ public function testGetResultWithMixedDomains()
    {
        $this->assertEquals(
            new MessageCatalogue('en', [
-                'messages+intl-icu' => ['a' => 'old_a'],
+                'messages' => ['a' => 'old_a'],


Since this is fixing the broken test by saying the test was wrong I now really need someone with a deep understanding of the code to look into this 🙈

But my reasoning for why this is actually the correct and intended behavior was:
Existing messages should always stay in their current domain. Moving new messages to the intl-icu domain is (at least in recent versions) handled by AbstractOperation::moveMessagesToIntlDomainsIfPossible(), see

symfony/src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php

Line 247 in 1629b59

$operation->moveMessagesToIntlDomainsIfPossible('new');

and

symfony/src/Symfony/Component/Translation/Catalogue/AbstractOperation.php

Lines 157 to 183 in 1629b59

public function moveMessagesToIntlDomainsIfPossible(string $batch = self::ALL_BATCH): void

{

// If MessageFormatter class does not exists, intl domains are not supported.

if (!class_exists(\MessageFormatter::class)) {

return;

}

foreach ($this->getDomains() as $domain) {

$intlDomain = $domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX;

switch ($batch) {

case self::OBSOLETE_BATCH: $messages = $this->getObsoleteMessages($domain); break;

case self::NEW_BATCH: $messages = $this->getNewMessages($domain); break;

case self::ALL_BATCH: $messages = $this->getMessages($domain); break;

default: throw new \InvalidArgumentException(sprintf('$batch argument must be one of ["%s", "%s", "%s"].', self::ALL_BATCH, self::NEW_BATCH, self::OBSOLETE_BATCH));

}

if (!$messages || (!$this->source->all($intlDomain) && $this->source->all($domain))) {

continue;

}

$result = $this->getResult();

$allIntlMessages = $result->all($intlDomain);

$currentMessages = array_diff_key($messages, $result->all($domain));

$result->replace($currentMessages, $domain);

$result->replace($allIntlMessages + $messages, $intlDomain);

}

}

This Test was introduced in b72b7d3 which also should only have affected new messages. So was in my interpretation already wrong at that point.

acran · 2021-09-02T12:40:43Z

src/Symfony/Component/Translation/Tests/Catalogue/TargetOperationTest.php

+            )->getResult()
+        );
+
+        $this->assertEquals(


This is testing the behavior when merging with new messages and mixed domains. Should this be moved into a separate function/test case? Or is it OK to have it all in the same function?

I would prefer a data provider with some explanations for each data, but this is only a recommendation.

acran · 2021-10-28T12:06:02Z

Any progress on this? Who could best review this PR??

fabpot · 2021-10-30T08:55:56Z

Thank you @acran.

fabpot · 2021-10-30T08:56:16Z

@acran Could you do a follow-up PR to take into account @OskarStark's suggestion about documenting the tests?

carsonbot added Status: Needs Review Translation labels Aug 3, 2021

carsonbot added this to the 4.4 milestone Aug 3, 2021

This was referenced Aug 3, 2021

[FrameworkBundle] remove dead conditions in Translation Commands #42362

Closed

[FrameworkBundle] remove dead conditions in Translation Commands #42390

Merged

[Translation] correctly handle intl domains with TargetOperation

84b48fe

This would cause issues with merging existing intl domains since it was trying to merge with the target instead of the source. This was originally introduced with c71dfb9 The incorrect Test was introduced with b72b7d3

acran force-pushed the 4.4 branch from 66e2e3c to 84b48fe Compare September 2, 2021 12:27

acran commented Sep 2, 2021

View reviewed changes

yceruto approved these changes Oct 29, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 29, 2021

fabpot merged commit d146704 into symfony:4.4 Oct 30, 2021

This was referenced Nov 22, 2021

Release v4.4.34 #44193

Merged

Release v5.3.11 #44194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Translation] correctly handle intl domains with TargetOperation #42361

[Translation] correctly handle intl domains with TargetOperation #42361

acran commented Aug 3, 2021 •

edited by Nyholm

Loading

acran commented Aug 11, 2021

fabpot commented Aug 26, 2021

acran commented Sep 2, 2021 •

edited

Loading

acran Sep 2, 2021

acran Sep 2, 2021

OskarStark Oct 29, 2021

acran commented Oct 28, 2021

fabpot commented Oct 30, 2021

fabpot commented Oct 30, 2021

	public function moveMessagesToIntlDomainsIfPossible(string $batch = self::ALL_BATCH): void
	{
	// If MessageFormatter class does not exists, intl domains are not supported.
	if (!class_exists(\MessageFormatter::class)) {
	return;
	}

	foreach ($this->getDomains() as $domain) {
	$intlDomain = $domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX;
	switch ($batch) {
	case self::OBSOLETE_BATCH: $messages = $this->getObsoleteMessages($domain); break;
	case self::NEW_BATCH: $messages = $this->getNewMessages($domain); break;
	case self::ALL_BATCH: $messages = $this->getMessages($domain); break;
	default: throw new \InvalidArgumentException(sprintf('$batch argument must be one of ["%s", "%s", "%s"].', self::ALL_BATCH, self::NEW_BATCH, self::OBSOLETE_BATCH));
	}

	if (!$messages \|\| (!$this->source->all($intlDomain) && $this->source->all($domain))) {
	continue;
	}

	$result = $this->getResult();
	$allIntlMessages = $result->all($intlDomain);
	$currentMessages = array_diff_key($messages, $result->all($domain));
	$result->replace($currentMessages, $domain);
	$result->replace($allIntlMessages + $messages, $intlDomain);
	}
	}

[Translation] correctly handle intl domains with TargetOperation #42361

[Translation] correctly handle intl domains with TargetOperation #42361

Conversation

acran commented Aug 3, 2021 • edited by Nyholm Loading

How to reproduce

Setup Test Project

Steps to reproduce

Case 1: duplicate translation files

Case 2: lost translations

The Fix

acran commented Aug 11, 2021

fabpot commented Aug 26, 2021

acran commented Sep 2, 2021 • edited Loading

acran Sep 2, 2021

Choose a reason for hiding this comment

acran Sep 2, 2021

Choose a reason for hiding this comment

OskarStark Oct 29, 2021

Choose a reason for hiding this comment

acran commented Oct 28, 2021

fabpot commented Oct 30, 2021

fabpot commented Oct 30, 2021

acran commented Aug 3, 2021 •

edited by Nyholm

Loading

acran commented Sep 2, 2021 •

edited

Loading