NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Translator] Intl message converter#28486

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Nyholm wants to merge7 commits intosymfony:masterfromNyholm:icu-converter

Closed

[Translator] Intl message converter#28486

Nyholm wants to merge7 commits intosymfony:masterfromNyholm:icu-converter

Conversation

Copy link

Member

Nyholm commentedSep 17, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

As discussed in#28375, we need a command to convert from "Symfony 3" plural format to Intl message format.

Before

<trans-unitid="xx0"resname="simple_key">  <source>simple_key</source>  <target>Hello %name%</target></trans-unit><trans-unitid="xx1"resname="plural_key">  <source>plural_key</source>  <target>{0} %name%, there are no apples|{1} %name%, there is one apple|]1,Inf] %name%, there are %count% apples</target></trans-unit>

{{'simple_key|trans({"%name%": "foo"}) }} <br>{{'plural_key|transchoice(2, {"%name%":"foo"}) }}

After

<trans-unitid="xx0"resname="simple_key">  <source>simple_key</source>  <target>Hello {name}</target></trans-unit><trans-unitid="xx1"resname="plural_key">  <source>plural_key</source>  <target>{ COUNT, plural,    =0 {{name}, there are no apples}    =1 {{name}, there is one apple}    other {{name}, there are # apples}}</target>

{{'simple_key'|trans({"name":"foo"}) }} <br>{{'plural_key'|trans({"COUNT":2,"name":"foo"}) }}

As you can see, the user needs to modify their twig templates, "%name%" => "name". To use "{%name%}" in the translation message is not valid syntax.

TODO

I need to add more tests of plural messages. Feel free to add a comment with a "Symfony style" plural string and I'll add it to the tests.

We also need a way to modify the the%name% placeholder. Users may use#name# or any other arbitrary string.

carsonbot added Status: Needs Review Translation Feature labels

Sep 17, 2018

Adding IntlMessageConverter

6ac56d3

Nyholm force-pushed theicu-converter branch from47846bb to6ac56d3Compare

September 17, 2018 01:49

nicolas-grekas added this to thenext milestone

Sep 17, 2018

Nyholm mentioned this pull request

Sep 17, 2018

[Translator] Deprecated transChoice and moved it away from contracts#28375

Merged

BenjaminRbt reviewed

Sep 21, 2018

View reviewed changes

src/Symfony/Component/Translation/Command/IntlConvertCommand.php Outdated

		$kernel =$this->getApplication()->getKernel();

		// Define Root Paths
		$transPaths =$kernel->getProjectDir().'/translations';

Copy link

Choose a reason for hiding this comment

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

$transPaths = $kernel->getProjectDir().'/translations';
🔽
$transPaths = $kernel->getProjectDir().DIRECTORY_SEPARATOR.'translations';
WDYT ?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Thank you

src/Symfony/Component/Translation/Util/IntlMessageConverter.php

		EOF;

		$standardRules =array();
		foreach ($partsas$part) {

Copy link

Choose a reason for hiding this comment

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

What if$parts is empty ? I would have add a check not to continue ifempty($parts) is true

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I dont know.. When will parts be empty?

I adding a small fix.

Nyholm added2 commits

September 22, 2018 11:25

minor fix

83bfcb6

Make sure "$parts" are never empty

e83e32e

Nyholm changed the title~~[Translator] WIP: Intl message converter~~[Translator] WIP: Icu message converter

Sep 22, 2018

Nyholm added3 commits

September 22, 2018 12:06

It is called Icu message format. Not Intl

acbf7a0

Allow to specify delimiter

df69607

cs

9071df7

Nyholm changed the title~~[Translator] WIP: Icu message converter~~[Translator] Icu message converter

Sep 22, 2018

Nyholm changed the title~~[Translator] Icu message converter~~[Translator] Intl message converter

Sep 22, 2018

Renamed to Intl

6687d9a

Copy link

Member

nicolas-grekas commentedOct 1, 2018

If#28375 goes well, wemight close this one and save us some work. \o/

Copy link

Member

fabpot commentedOct 10, 2018

Why could we close this one? We still need a way to move old messages to the new format, right?

Copy link

Member

nicolas-grekas commentedOct 10, 2018

We don'tneed one because we don't plan to deprecate the current format. It could be nice to provide the command if ppl want to move to intl, but there is no real reason to do it so I'm not sure it's worth it.

Copy link

Member

fabpot commentedOct 10, 2018

I get that, but I think people will want to migrate to the new format as it is more powerful.

Copy link

Member

stof commentedOct 11, 2018

@nicolas-grekas at least for pluralized message, itis deprecated currently (as APIs consuming it are deprecated)

Copy link

Member

nicolas-grekas commentedOct 11, 2018

@stof not really, see#28375 (comment)

nicolas-grekas reviewed

Jan 27, 2019

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

please rebase and move to short arrays.

javiereguiluz added the ⭐️ EU-FOSSA Hackathon label

Apr 2, 2019

nicolas-grekas removed the ⭐️ EUFOSSA Hackathon label

Apr 10, 2019

Copy link

Member

fabpot commentedDec 3, 2019

@Nyholm Do you have any plans to finish this one? I think it would be a very nice and useful addition.

Copy link

MemberAuthor

Nyholm commentedDec 3, 2019

Yes, I will make sure to complete this PR.

Thank you for the ping

Copy link

Member

welcoMattic commentedJul 1, 2020

This PR could be very useful alongside#37462 to automate intl conversion of messages stored in a translation SaaS.
@Nyholm do you plan to finish it before 5.2 release (which is the release we target for#37462)?

Copy link

MemberAuthor

Nyholm commentedJul 1, 2020

Yes. I will

fabpot requested changes

Aug 19, 2020

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

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

Quick review (mostly about style) as a nice ping/reminder :)

src/Symfony/Bundle/FrameworkBundle/Resources/config/console.xml

		<argumenttype="service"id="translation.writer" />
		<argumenttype="service"id="translation.reader" />
		<tagname="console.command"command="translation:convert-to-intl-messages" />
		</service>

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

Should be moved to the new PHP config file.

src/Symfony/Component/Translation/Command/IntlConvertCommand.php

		protectedfunctionconfigure()
		{
		$this
		->setDescription('Convert from Symfony 3 plural format to ICU message format.')

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

I would remove 3 here.

src/Symfony/Component/Translation/Command/IntlConvertCommand.php

		;
		}

		protectedfunctionexecute(InputInterface$input,OutputInterface$output)

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

: int

src/Symfony/Component/Translation/Command/IntlConvertCommand.php

		}
		}

		$this->writer->write(newMessageCatalogue($locale,$updated),$input->getOption('output-format'),array('path' =>$transPaths));

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

return 0;

src/Symfony/Component/Translation/Util/IntlMessageConverter.php

		}

		/**
		* Get an ICU like array.

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

Gets

src/Symfony/Component/Translation/Util/IntlMessageConverter.php

		namespaceSymfony\Component\Translation\Util;

		/**
		* Convert from Symfony 3's plural syntax to Intl message format.

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

I would remove the reference to 3 here as well.

src/Symfony/Component/Translation/Util/IntlMessageConverter.php

		publicstaticfunctionconvert(string$message,string$variableDelimiter ='%'):string
		{
		$array =self::getMessageArray($message);
		if (empty($array)) {

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

We try to avoid usingempty in Symfony.if (!$array) seems enough here.

src/Symfony/Component/Translation/Util/IntlMessageConverter.php

		returnself::replaceVariables($message,$variableDelimiter);
		}

		$icu =self::buildIcuString($array,$variableDelimiter);

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

temp var could be removed

src/Symfony/Component/Translation/Util/IntlMessageConverter.php


		privatestaticfunctionreplaceVariables(string$message,string$variableDelimiter):string
		{
		$regex =sprintf('\|%s(.*?)%s\|s',$variableDelimiter,$variableDelimiter);

Copy link

Member

fabpotAug 19, 2020

Choose a reason for hiding this comment

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

I would inline the regexp

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Aug 19, 2020

Copy link

Member

nicolas-grekas commentedSep 7, 2020

I'm closing here because this staled and apparently this is not critical, since this gathered very little feedback from the community. Let's reopen when someone actually needs this (I'd invite that person to adopt the patch here to make the new PR).