Executingtranslations: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 in33e6af5

How to reproduce

Setup Test Project

A fresh minimal project to verify this can be setup with

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

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

<?php// in src/translations.phpuseSymfony\Component\Translation\TranslatableMessage;newTranslatableMessage('First Message');newTranslatableMessage('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 againrm -rf translations/*php bin/console translation:update --force --clean en# verify translations/messages+intl-icu.en.xlf was created containing the messages# extract messages againphp 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 againrm -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 againphp 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 thetarget messages instead of thesource. This was probably just a typo since$keyMetadata is take fromsource just below it and theforeach block below which is basically doing the same inverse just switchestarget andsource.
Also the code is mostly identical toMergeOperation which also usessource at the respective line.

The code in both files was introduced in33e6af5

carsonbot added Status: Needs Review Translation labels

Aug 3, 2021

carsonbot added this to the4.4 milestone

Aug 3, 2021

This was referencedAug 3, 2021

[FrameworkBundle] remove dead conditions in Translation Commands#42362

Closed

[FrameworkBundle] remove dead conditions in Translation Commands#42390

Merged

fabpot added a commit that referenced this pull request

Aug 6, 2021

minor#42390[FrameworkBundle] remove dead conditions in Translation …

2f8ac04

…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 inb6eb1f4:* 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

Copy link

ContributorAuthor

acran commentedAug 11, 2021

/cc@yceruto

Copy link

Member

fabpot commentedAug 26, 2021

@acran Could you add a unit test?

[Translation] correctly handle intl domains with TargetOperation

84b48fe

This would cause issues with merging existing intl domains since it wastrying to merge with the target instead of the source.This was originally introduced withc71dfb9The incorrect Test was introduced withb72b7d3

acran force-pushed the4.4 branch from66e2e3c to84b48feCompare

September 2, 2021 12:27

Copy link

ContributorAuthor

acran commentedSep 2, 2021•
edited
Loading

@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 though33e6af5 was the commit that introduced the offendingline it was actually just the result of refactorings and code style fixes and the problem was introduced inc71dfb9. Looking at that commit it definitely smells like a copy&paste mistake.

acran commented

Sep 2, 2021

View reviewed changes

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

		$this->assertEquals(
		newMessageCatalogue('en', [
		'messages+intl-icu' => ['a' =>'old_a'],
		'messages' => ['a' =>'old_a'],

Copy link

ContributorAuthor

acranSep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Since this is fixing the broken test by sayingthe test was wrong I nowreally 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 shouldalways stay in their current domain. Moving new messages to the intl-icu domain is (at least in recent versions) handled byAbstractOperation::moveMessagesToIntlDomainsIfPossible(), see

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

Line 247 in1629b59

$operation->moveMessagesToIntlDomainsIfPossible('new');

and

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

Lines 157 to 183 in1629b59

	publicfunctionmoveMessagesToIntlDomainsIfPossible(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) {
	caseself::OBSOLETE_BATCH:$messages =$this->getObsoleteMessages($domain);break;
	caseself::NEW_BATCH:$messages =$this->getNewMessages($domain);break;
	caseself::ALL_BATCH:$messages =$this->getMessages($domain);break;
	default:thrownew \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 inb72b7d3 which also should only have affectednew messages. So wasin my interpretation already wrong at that point.

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

		)->getResult()
		);

		$this->assertEquals(

Copy link

ContributorAuthor

acranSep 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?