Skip to content

Commit 9e2dda3

Browse files
authored
Bundle does not work properly with new Intl ICU translations (#452)
* Remove both "normal" and ICU domain from messages set #300 * Add new configuration for new message format It will allow us to control if we want to use new ICU format (with suffixed domain with "+intl-icu") or format. Configuration has normalizer (making sure string is in lower case) and validator (making sure it's a string and either "" or "icu"). Setting `new_message_format: ""` we'll end up with new translations in container like: {domain}.{locale}.{ext}. Setting `new_message_format: "icu"` we'll end up with new translations in container like: {domain}+intl-icu.{locale}.{ext}. #300 * Add new field and getter based on recently added new config option #300 * Make sure importer is ICU format aware Add new argument to converter methods to make sure we're able to recognize existing translations (and use correct domain - either standard or ICU format). In case translation is new we're going to use predefined format (using new configuration option new_message_format). #300 * Make sure new_message_format configuration is defined Let the default configuration overlap with Symfony configuration option default value. #300 * Pass to converter new_message_format configuration #300 * Make sure importer is aware of ICU domain format Because of MessageCatalogue interface limitation we're using NSA to access raw messages data. Additionally because defines() method uses isset() were force to check raw messages data directly to check if key exits (translations from source collection are all set to NULL). #300 * Let code "breath" a bit #300 * Make sure we're always aware of ICU domain #300 * Make sure we're aware of 3 different message domains We have: 1. Source message domain. 2. Target message domain. 3. Result message domain (this is tied to 2 of the above). This solution assumes we'd always like to use ICU format (result message domain) if we use it in any of source or target catalogue. Additionally because of default NULL values in source catalogue (internal extractor design to set NULL for "target" when creating messages catalogue from source collection) we have to check if desired catalogue's messages field has a key (within requested domain) or not. Using MessageCatalogue::defines() would return false for catalogue without translation (NULL value). Lastly - make sure internal messages field utilised result message domain for consistency with result field. #300 * Persist PHP CS Fixer changes #300 * Remove both "normal" and ICU domain from messages set #300 * Add new configuration for new message format It will allow us to control if we want to use new ICU format (with suffixed domain with "+intl-icu") or format. Configuration has normalizer (making sure string is in lower case) and validator (making sure it's a string and either "" or "icu"). Setting `new_message_format: ""` we'll end up with new translations in container like: {domain}.{locale}.{ext}. Setting `new_message_format: "icu"` we'll end up with new translations in container like: {domain}+intl-icu.{locale}.{ext}. #300 * Add new field and getter based on recently added new config option #300 * Make sure importer is ICU format aware Add new argument to converter methods to make sure we're able to recognize existing translations (and use correct domain - either standard or ICU format). In case translation is new we're going to use predefined format (using new configuration option new_message_format). #300 * Make sure new_message_format configuration is defined Let the default configuration overlap with Symfony configuration option default value. #300 * Pass to converter new_message_format configuration #300 * Make sure importer is aware of ICU domain format Because of MessageCatalogue interface limitation we're using NSA to access raw messages data. Additionally because defines() method uses isset() were force to check raw messages data directly to check if key exits (translations from source collection are all set to NULL). #300 * Let code "breath" a bit #300 * Make sure we're always aware of ICU domain #300 * Make sure we're aware of 3 different message domains We have: 1. Source message domain. 2. Target message domain. 3. Result message domain (this is tied to 2 of the above). This solution assumes we'd always like to use ICU format (result message domain) if we use it in any of source or target catalogue. Additionally because of default NULL values in source catalogue (internal extractor design to set NULL for "target" when creating messages catalogue from source collection) we have to check if desired catalogue's messages field has a key (within requested domain) or not. Using MessageCatalogue::defines() would return false for catalogue without translation (NULL value). Lastly - make sure internal messages field utilised result message domain for consistency with result field. #300 * Persist PHP CS Fixer changes #300 * Apply PHP CS fixer automated fix * Fix main PHPUnit issue We're now providing getter method name for new field. * Fix catalogue counter * Make assertion more wordy * Update configuration to have passing tests Add new ICU messages domain. Make sure configuration is set to before-the-modification configuration. New messages will not be added to ICU domain. An additional tests must be run with new option set to ICU. This however may require functional tests refactor. * Apply PHP-CS-Fixer suggestion
1 parent 9bd3eca commit 9e2dda3

15 files changed

+154
-42
lines changed

Catalogue/CatalogueCounter.php

+3-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ public function getCatalogueStatistics(MessageCatalogueInterface $catalogue): ar
5757
$result[$domain]['approved'] = 0;
5858

5959
foreach ($catalogue->all($domain) as $key => $text) {
60-
$metadata = new Metadata($catalogue->getMetadata($key, $domain));
60+
$intlDomain = $domain.'+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */;
61+
$rawMetadata = $catalogue->getMetadata($key, $domain) ?: $catalogue->getMetadata($key, $intlDomain);
62+
$metadata = new Metadata($rawMetadata);
6163
$state = $metadata->getState();
6264
if ('new' === $state) {
6365
++$result[$domain]['new'];

Catalogue/CatalogueFetcher.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ public function getCatalogues(Configuration $config, array $locales = []): array
5353

5454
foreach ($currentCatalogue->getDomains() as $domain) {
5555
if (!$this->isValidDomain($config, $domain)) {
56-
$messages = $currentCatalogue->all();
57-
unset($messages[$domain]);
56+
$messages = NSA::getProperty($currentCatalogue, 'messages');
57+
unset($messages[$domain], $messages[$domain.'+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */]);
5858
NSA::setProperty($currentCatalogue, 'messages', $messages);
5959
}
6060
}

Catalogue/Operation/ReplaceOperation.php

+30-20
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Translation\Bundle\Catalogue\Operation;
1313

14+
use Nyholm\NSA;
1415
use Symfony\Component\Translation\Catalogue\AbstractOperation;
1516
use Symfony\Component\Translation\MessageCatalogueInterface;
1617
use Symfony\Component\Translation\MetadataAwareInterface;
@@ -37,26 +38,31 @@ protected function processDomain($domain): void
3738
'new' => [],
3839
'obsolete' => [],
3940
];
40-
if (\defined(sprintf('%s::INTL_DOMAIN_SUFFIX', MessageCatalogueInterface::class))) {
41-
$intlDomain = $domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX;
42-
} else {
43-
$intlDomain = $domain;
44-
}
41+
42+
$intlDomain = $domain.'+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */;
43+
44+
$sourceMessages = NSA::getProperty($this->source, 'messages');
45+
$targetMessages = NSA::getProperty($this->target, 'messages');
4546

4647
foreach ($this->source->all($domain) as $id => $message) {
47-
$messageDomain = $this->source->defines($id, $intlDomain) ? $intlDomain : $domain;
48+
$sourceIdInIntl = \array_key_exists($id, $sourceMessages[$intlDomain] ?? []);
49+
$targetIdInIntl = \array_key_exists($id, $targetMessages[$intlDomain] ?? []);
50+
51+
$sourceMessageDomain = $sourceIdInIntl ? $intlDomain : $domain;
52+
$targetMessageDomain = $targetIdInIntl ? $intlDomain : $domain;
53+
$resultMessageDomain = $sourceIdInIntl || $targetIdInIntl ? $intlDomain : $domain;
4854

4955
if (!$this->target->has($id, $domain)) {
5056
// No merge required
5157
$translation = $message;
52-
$this->messages[$domain]['new'][$id] = $message;
53-
$resultMeta = $this->getMetadata($this->source, $messageDomain, $id);
58+
$this->messages[$resultMessageDomain]['new'][$id] = $message;
59+
$resultMeta = $this->getMetadata($this->source, $sourceMessageDomain, $id);
5460
} else {
5561
// Merge required
56-
$translation = $message ?? $this->target->get($id, $domain);
62+
$translation = $message ?? $this->target->get($id, $targetMessageDomain);
5763
$resultMeta = null;
58-
$sourceMeta = $this->getMetadata($this->source, $messageDomain, $id);
59-
$targetMeta = $this->getMetadata($this->target, $this->target->defines($id, $intlDomain) ? $intlDomain : $domain, $id);
64+
$sourceMeta = $this->getMetadata($this->source, $sourceMessageDomain, $id);
65+
$targetMeta = $this->getMetadata($this->target, $targetMessageDomain, $id);
6066
if (\is_array($sourceMeta) && \is_array($targetMeta)) {
6167
// We can only merge meta if both is an array
6268
$resultMeta = $this->mergeMetadata($sourceMeta, $targetMeta);
@@ -68,11 +74,11 @@ protected function processDomain($domain): void
6874
}
6975
}
7076

71-
$this->messages[$domain]['all'][$id] = $translation;
72-
$this->result->add([$id => $translation], $messageDomain);
77+
$this->messages[$resultMessageDomain]['all'][$id] = $translation;
78+
$this->result->add([$id => $translation], $resultMessageDomain);
7379

7480
if (!empty($resultMeta)) {
75-
$this->result->setMetadata($id, $resultMeta, $messageDomain);
81+
$this->result->setMetadata($id, $resultMeta, $resultMessageDomain);
7682
}
7783
}
7884

@@ -83,14 +89,18 @@ protected function processDomain($domain): void
8389
continue;
8490
}
8591

86-
$messageDomain = $this->target->defines($id, $intlDomain) ? $intlDomain : $domain;
87-
$this->messages[$domain]['all'][$id] = $message;
88-
$this->messages[$domain]['obsolete'][$id] = $message;
89-
$this->result->add([$id => $message], $messageDomain);
92+
$sourceIdInIntl = \array_key_exists($id, $sourceMessages[$intlDomain] ?? []);
93+
$targetIdInIntl = \array_key_exists($id, $targetMessages[$intlDomain] ?? []);
94+
95+
$resultMessageDomain = $sourceIdInIntl || $targetIdInIntl ? $intlDomain : $domain;
96+
97+
$this->messages[$resultMessageDomain]['all'][$id] = $message;
98+
$this->messages[$resultMessageDomain]['obsolete'][$id] = $message;
99+
$this->result->add([$id => $message], $resultMessageDomain);
90100

91-
$resultMeta = $this->getMetadata($this->target, $messageDomain, $id);
101+
$resultMeta = $this->getMetadata($this->target, $resultMessageDomain, $id);
92102
if (!empty($resultMeta)) {
93-
$this->result->setMetadata($id, $resultMeta, $messageDomain);
103+
$this->result->setMetadata($id, $resultMeta, $resultMessageDomain);
94104
}
95105
}
96106
}

Command/CheckMissingCommand.php

+1
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
8080
'blacklist_domains' => $config->getBlacklistDomains(),
8181
'whitelist_domains' => $config->getWhitelistDomains(),
8282
'project_root' => $config->getProjectRoot(),
83+
'new_message_format' => $config->getNewMessageFormat(),
8384
]
8485
);
8586

Command/ExtractCommand.php

+1
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
106106
'blacklist_domains' => $config->getBlacklistDomains(),
107107
'whitelist_domains' => $config->getWhitelistDomains(),
108108
'project_root' => $config->getProjectRoot(),
109+
'new_message_format' => $config->getNewMessageFormat(),
109110
]);
110111
$errors = $result->getErrors();
111112

DependencyInjection/Configuration.php

+22
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,28 @@ private function configsNode(ArrayNodeDefinition $root): void
164164
->scalarNode('output_dir')->cannotBeEmpty()->defaultValue('%kernel.project_dir%/Resources/translations')->end()
165165
->scalarNode('project_root')->info("The root dir of your project. By default this will be kernel_root's parent.")->end()
166166
->scalarNode('xliff_version')->info('The version of XLIFF XML you want to use (if dumping to this format).')->defaultValue('2.0')->end()
167+
->scalarNode('new_message_format')
168+
->info('Use "icu" to place new translations in "{domain}+intl-icu.{locale}.{ext}" container')
169+
->defaultValue('icu')
170+
->beforeNormalization()
171+
->ifTrue(
172+
function ($format) {
173+
return \is_string($format);
174+
}
175+
)
176+
->then(
177+
function (string $format) {
178+
return strtolower($format);
179+
}
180+
)
181+
->end()
182+
->validate()
183+
->ifTrue(function ($value) {
184+
return !\is_string($value) || !\in_array($value, ['', 'icu']);
185+
})
186+
->thenInvalid('The "new_message_format" must be either: "" or "icu"; got "%s"')
187+
->end()
188+
->end()
167189
->variableNode('local_file_storage_options')
168190
->info('Options passed to the local file storage\'s dumper.')
169191
->defaultValue([])

Model/Configuration.php

+15
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,11 @@ final class Configuration
9292
*/
9393
private $xliffVersion;
9494

95+
/**
96+
* @var string
97+
*/
98+
private $newMessageFormat;
99+
95100
public function __construct(array $data)
96101
{
97102
$this->name = $data['name'];
@@ -106,6 +111,7 @@ public function __construct(array $data)
106111
$this->blacklistDomains = $data['blacklist_domains'];
107112
$this->whitelistDomains = $data['whitelist_domains'];
108113
$this->xliffVersion = $data['xliff_version'];
114+
$this->newMessageFormat = $data['new_message_format'];
109115
}
110116

111117
public function getName(): string
@@ -177,6 +183,15 @@ public function getXliffVersion(): string
177183
return $this->xliffVersion;
178184
}
179185

186+
/**
187+
* If set to "icu" it'll place all new translations in "{domain}+intl-icu.{locale}.{ext}" file.
188+
* Otherwise normal "{domain}.{locale}.{ext}" file will be used.
189+
*/
190+
public function getNewMessageFormat(): string
191+
{
192+
return $this->newMessageFormat;
193+
}
194+
180195
/**
181196
* Reconfigures the directories so we can use one configuration for extracting
182197
* the messages only from one bundle.

Service/Importer.php

+43-14
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Translation\Bundle\Service;
1313

14+
use Nyholm\NSA;
1415
use Symfony\Component\Finder\Finder;
1516
use Symfony\Component\Translation\MessageCatalogue;
1617
use Translation\Bundle\Catalogue\Operation\ReplaceOperation;
@@ -74,11 +75,11 @@ public function extractToCatalogues(Finder $finder, array $catalogues, array $co
7475
$results = [];
7576
foreach ($catalogues as $catalogue) {
7677
$target = new MessageCatalogue($catalogue->getLocale());
77-
$this->convertSourceLocationsToMessages($target, $sourceCollection);
78+
$this->convertSourceLocationsToMessages($target, $sourceCollection, $catalogue);
7879

7980
// Remove all SourceLocation and State form catalogue.
80-
foreach ($catalogue->getDomains() as $domain) {
81-
foreach ($catalogue->all($domain) as $key => $translation) {
81+
foreach (NSA::getProperty($catalogue, 'messages') as $domain => $translations) {
82+
foreach ($translations as $key => $translation) {
8283
$meta = $this->getMetadata($catalogue, $key, $domain);
8384
$meta->removeAllInCategory('file-source');
8485
$meta->removeAllInCategory('state');
@@ -90,28 +91,37 @@ public function extractToCatalogues(Finder $finder, array $catalogues, array $co
9091
$result = $merge->getResult();
9192
$domains = $merge->getDomains();
9293

94+
$resultMessages = NSA::getProperty($result, 'messages');
95+
9396
// Mark new messages as new/obsolete
9497
foreach ($domains as $domain) {
98+
$intlDomain = $domain.'+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */;
99+
95100
foreach ($merge->getNewMessages($domain) as $key => $translation) {
96-
$meta = $this->getMetadata($result, $key, $domain);
101+
$messageDomain = \array_key_exists($key, $resultMessages[$intlDomain] ?? []) ? $intlDomain : $domain;
102+
103+
$meta = $this->getMetadata($result, $key, $messageDomain);
97104
$meta->setState('new');
98-
$this->setMetadata($result, $key, $domain, $meta);
105+
$this->setMetadata($result, $key, $messageDomain, $meta);
99106

100107
// Add custom translations that we found in the source
101108
if (null === $translation) {
102109
if (null !== $newTranslation = $meta->getTranslation()) {
103-
$result->set($key, $newTranslation, $domain);
110+
$result->set($key, $newTranslation, $messageDomain);
104111
// We do not want "translation" key stored anywhere.
105112
$meta->removeAllInCategory('translation');
106113
} elseif (null !== ($newTranslation = $meta->getDesc()) && $catalogue->getLocale() === $this->defaultLocale) {
107-
$result->set($key, $newTranslation, $domain);
114+
$result->set($key, $newTranslation, $messageDomain);
108115
}
109116
}
110117
}
118+
111119
foreach ($merge->getObsoleteMessages($domain) as $key => $translation) {
112-
$meta = $this->getMetadata($result, $key, $domain);
120+
$messageDomain = \array_key_exists($key, $resultMessages[$intlDomain] ?? []) ? $intlDomain : $domain;
121+
122+
$meta = $this->getMetadata($result, $key, $messageDomain);
113123
$meta->setState('obsolete');
114-
$this->setMetadata($result, $key, $domain, $meta);
124+
$this->setMetadata($result, $key, $messageDomain, $meta);
115125
}
116126
}
117127
$results[] = $result;
@@ -120,30 +130,48 @@ public function extractToCatalogues(Finder $finder, array $catalogues, array $co
120130
return new ImportResult($results, $sourceCollection->getErrors());
121131
}
122132

123-
private function convertSourceLocationsToMessages(MessageCatalogue $catalogue, SourceCollection $collection): void
124-
{
133+
private function convertSourceLocationsToMessages(
134+
MessageCatalogue $catalogue,
135+
SourceCollection $collection,
136+
MessageCatalogue $currentCatalogue
137+
): void {
138+
$currentMessages = NSA::getProperty($currentCatalogue, 'messages');
139+
125140
/** @var SourceLocation $sourceLocation */
126141
foreach ($collection as $sourceLocation) {
127142
$context = $sourceLocation->getContext();
128143
$domain = $context['domain'] ?? 'messages';
144+
129145
// Check with white/black list
130146
if (!$this->isValidDomain($domain)) {
131147
continue;
132148
}
133149

150+
$intlDomain = $domain.'+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */;
151+
134152
$key = $sourceLocation->getMessage();
135-
$catalogue->add([$key => null], $domain);
153+
154+
if (\array_key_exists($key, $currentMessages[$intlDomain] ?? [])) {
155+
$messageDomain = $intlDomain;
156+
} elseif (\array_key_exists($key, $currentMessages[$domain] ?? [])) {
157+
$messageDomain = $domain;
158+
} else {
159+
// New translation
160+
$messageDomain = 'icu' === $this->config['new_message_format'] ? $intlDomain : $domain;
161+
}
162+
163+
$catalogue->add([$key => null], $messageDomain);
136164
$trimLength = 1 + \strlen($this->config['project_root']);
137165

138-
$meta = $this->getMetadata($catalogue, $key, $domain);
166+
$meta = $this->getMetadata($catalogue, $key, $messageDomain);
139167
$meta->addCategory('file-source', sprintf('%s:%s', substr($sourceLocation->getPath(), $trimLength), $sourceLocation->getLine()));
140168
if (isset($sourceLocation->getContext()['desc'])) {
141169
$meta->addCategory('desc', $sourceLocation->getContext()['desc']);
142170
}
143171
if (isset($sourceLocation->getContext()['translation'])) {
144172
$meta->addCategory('translation', $sourceLocation->getContext()['translation']);
145173
}
146-
$this->setMetadata($catalogue, $key, $domain, $meta);
174+
$this->setMetadata($catalogue, $key, $messageDomain, $meta);
147175
}
148176
}
149177

@@ -178,6 +206,7 @@ private function processConfig(array $config): void
178206
'project_root' => '',
179207
'blacklist_domains' => [],
180208
'whitelist_domains' => [],
209+
'new_message_format' => 'icu',
181210
];
182211

183212
$config = array_merge($default, $config);

Tests/Functional/Catalogue/CatalogueFetcherTest.php

+1
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ public static function getDefaultData(): array
8181
'blacklist_domains' => ['getBlacklistDomains'],
8282
'whitelist_domains' => ['getWhitelistDomains'],
8383
'xliff_version' => ['getXliffVersion'],
84+
'new_message_format' => ['getNewMessageFormat'],
8485
];
8586
}
8687

Tests/Functional/Command/ExtractCommandTest.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,9 @@ public function testExecute(): void
111111
}
112112

113113
$meta = new Metadata($catalogue->getMetadata('not.in.source'));
114-
$this->assertTrue('obsolete' === $meta->getState());
114+
self::assertSame('obsolete', $meta->getState(), 'Expect meta state to be correct');
115115

116116
$meta = new Metadata($catalogue->getMetadata('translated.title'));
117-
$this->assertTrue('new' === $meta->getState());
117+
self::assertSame('new', $meta->getState(), 'Expect meta state to be correct');
118118
}
119119
}

Tests/Functional/Command/StatusCommandTest.php

+3-3
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,9 @@ public function testExecute(): void
5555
$this->assertArrayHasKey('new', $total);
5656
$this->assertArrayHasKey('obsolete', $total);
5757
$this->assertArrayHasKey('approved', $total);
58-
$this->assertEquals(2, $total['defined']);
59-
$this->assertEquals(1, $total['new']);
58+
$this->assertEquals(4, $total['defined']);
59+
$this->assertEquals(2, $total['new']);
6060
$this->assertEquals(0, $total['obsolete']);
61-
$this->assertEquals(1, $total['approved']);
61+
$this->assertEquals(2, $total['approved']);
6262
}
6363
}
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
1+
messages+intl-icu.sv.xlf
12
messages.sv.xlf
23
*~
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<xliff xmlns="urn:oasis:names:tc:xliff:document:2.0" version="2.0" srcLang="fr-FR" trgLang="en-US">
3+
<file id="messages.en_US">
4+
<unit id="IcuLCa0a2j">
5+
<notes>
6+
<note category="state">new</note>
7+
<note category="approved">true</note>
8+
<note category="section" priority="1">user login</note>
9+
</notes>
10+
<segment>
11+
<source>key2</source>
12+
<target>trans2</target>
13+
</segment>
14+
</unit>
15+
<unit id="IcuLCa0a2b">
16+
<notes>
17+
<note category="file-source" priority="1">Resources/views/translated.html.twig:12</note>
18+
<note>file-source</note>
19+
<note>status:new</note>
20+
</notes>
21+
<segment>
22+
<source>key3</source>
23+
<target>trans3</target>
24+
</segment>
25+
</unit>
26+
</file>
27+
</xliff>

Tests/Functional/app/config/normal_config.yaml

+2
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ translation:
99
dirs: ["%test.project_dir%/Resources/views"]
1010
output_dir: "%test.project_dir%/Resources/translations"
1111
project_root: "%test.project_dir%"
12+
new_message_format: ''
1213
app_with_transchoice:
1314
dirs: ["%test.project_dir%/Resources/views_with_transchoice"]
1415
output_dir: "%test.project_dir%/Resources/translations"
1516
project_root: "%test.project_dir%"
17+
new_message_format: ''

0 commit comments

Comments
 (0)