Skip to content

[Translation] correctly handle intl domains with TargetOperation #42361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 30, 2021
Merged

[Translation] correctly handle intl domains with TargetOperation #42361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Symfony/Component/Translation/Catalogue/TargetOperation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ protected function processDomain($domain)
foreach ($this->source->all($domain) as $id => $message) {
if ($this->target->has($id, $domain)) {
$this->messages[$domain]['all'][$id] = $message;
$d = $this->target->defines($id, $intlDomain) ? $intlDomain : $domain;
$d = $this->source->defines($id, $intlDomain) ? $intlDomain : $domain;
$this->result->add([$id => $message], $d);
if (null !== $keyMetadata = $this->source->getMetadata($id, $d)) {
$this->result->setMetadata($id, $keyMetadata, $d);
Expand Down
34 changes: 33 additions & 1 deletion src/Symfony/Component/Translation/Tests/Catalogue/TargetOperationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,45 @@ public function testGetResultWithMixedDomains()
{
$this->assertEquals(
new MessageCatalogue('en', [
'messages+intl-icu' => ['a' => 'old_a'],
'messages' => ['a' => 'old_a'],
Copy link
Contributor Author

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

]),
$this->createOperation(
new MessageCatalogue('en', ['messages' => ['a' => 'old_a']]),
new MessageCatalogue('en', ['messages+intl-icu' => ['a' => 'new_a']])
)->getResult()
);

$this->assertEquals(
new MessageCatalogue('en', [
'messages+intl-icu' => ['a' => 'old_a'],
]),
$this->createOperation(
new MessageCatalogue('en', ['messages+intl-icu' => ['a' => 'old_a']]),
new MessageCatalogue('en', ['messages' => ['a' => 'new_a']])
)->getResult()
);

$this->assertEquals(
Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

new MessageCatalogue('en', [
'messages+intl-icu' => ['a' => 'old_a'],
'messages' => ['b' => 'new_b'],
]),
$this->createOperation(
new MessageCatalogue('en', ['messages+intl-icu' => ['a' => 'old_a']]),
new MessageCatalogue('en', ['messages' => ['a' => 'new_a', 'b' => 'new_b']])
)->getResult()
);

$this->assertEquals(
new MessageCatalogue('en', [
'messages' => ['a' => 'old_a'],
'messages+intl-icu' => ['b' => 'new_b'],
]),
$this->createOperation(
new MessageCatalogue('en', ['messages' => ['a' => 'old_a']]),
new MessageCatalogue('en', ['messages+intl-icu' => ['a' => 'new_a', 'b' => 'new_b']])
)->getResult()
);
}

public function testGetResultWithMetadata()
Expand Down