Bundle does not work properly with new Intl ICU translations #452

snpy · 2021-08-24T12:11:15Z

(Please ignore my ignorance if I should have followed PR guideline; PR was created in haste; if I should modify or redefine the PR please let me know and I'll fix it)

This PR is a proposition to handle ICU formats similarly as they are in Symfony internals.

The sanest way to deal with it is to stop using MessageCatalogue interface and look inside it with NSA package and determine manually if we should utilise ICU domain format or "normal" domain format.

Additional this PR introduces new_message_format configuration that will allow us to control if we'd like to use ICU format for new messages or not.
I guess for compatibility reason I should have set it by default to "" instead of "icu", but that can be easily fixed if this PR will be accepted only because of this BC issue.

The code itself was based on 0.12.3 tag instead of master in case master changes require additional work.

This should be fixing first issue described at #300 making bundle operational for translations kept in ICU formats.

php-translation#300

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}. php-translation#300

php-translation#300

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). php-translation#300

Let the default configuration overlap with Symfony configuration option default value. php-translation#300

php-translation#300

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). php-translation#300

php-translation#300

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. php-translation#300

welcoMattic · 2021-08-30T07:00:57Z

Catalogue/CatalogueFetcher.php

-                    $messages = $currentCatalogue->all();
-                    unset($messages[$domain]);
+                    $messages = NSA::getProperty($currentCatalogue, 'messages');
+                    unset($messages[$domain], $messages[$domain . '+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */]);


Why you don't use MessageCatalogueInterface::INTL_DOMAIN_SUFFIX anymore?

I've placed commented out const to make sure everyone knows the source of the string.

Reason: BC.

Personally I'd use imports no problem; however I didn't want to touch any dependencies, so went with literal string.

Similar to this snippet (at line https://github.com/php-translation/symfony-bundle/blob/master/Catalogue/Operation/ReplaceOperation.php#L40):

if (\defined(\sprintf('%s::INTL_DOMAIN_SUFFIX', MessageCatalogueInterface::class))) { $intlDomain = $domain.MessageCatalogueInterface::INTL_DOMAIN_SUFFIX; } else { $intlDomain = $domain; }

(difference is I'd still try to recognise this format - not sure if it'd work though)

welcoMattic · 2021-08-30T07:01:23Z

Catalogue/Operation/ReplaceOperation.php

-            $intlDomain = $domain;
-        }
+
+        $intlDomain = $domain . '+intl-icu' /* MessageCatalogueInterface::INTL_DOMAIN_SUFFIX */;


Here too, why you don't use MessageCatalogueInterface::INTL_DOMAIN_SUFFIX anymore?

Same as "above".

welcoMattic · 2021-08-30T07:04:59Z

DependencyInjection/Configuration.php

+                                })
+                                ->thenInvalid('The "new_message_format" must be either: "" or "icu"; got "%s"')
+                            ->end()
+                        ->end()


I think naming this parameter use_icu_format as a boolean is better here

For now we have 2 formats; later we may have more (though with pace of changes and ICU capability I guess we'd have to wait for at least a decade to have something new).

It's just future proof option; however I do not have any problems with replacing this with a bool configuration.

One remark here - the name does not reflect the its nature (it may suggest all translations will be casted to ICU format; for now it just places new translations in ICU formatted file).

php-translation#300

bocharsky-bw · 2022-01-20T08:42:20Z

Hey @snpy ,

Thank you for working on this and sorry for the delays! Could you pull the latest changes from master to see tests are green here? Also, any pending work left here, or do you think this one is ready?

bocharsky-bw · 2022-01-25T08:39:46Z

@snpy could you at least check "Allow edits by maintainers" here, I'll try to rebase it myself to see if tests are green here

snpy · 2022-01-25T12:02:50Z

@bocharsky-bw Hi, I will try to find some time to do it today or tomorrow.

I don't want to rebase current branch as it was intentionally based on specific tag version.

Will create new branch and rebase it (will also look into option you've mentioned).
Current branch is being used on production, so I'd like to avoid 3rd party edits.

bocharsky-bw · 2022-01-25T12:10:34Z

Hey Marek! Sure, do it when you can, it's not an urgent question. And thank you for the work you did so far on this!

LauLaman · 2022-08-19T20:22:02Z

any word on this? Would love to use this bundle with ICU support

snpy · 2022-08-20T15:05:54Z

Sorry, but I had chronically little time lately.
Will try to sync up changes with master before the end of August.

wokenlex · 2022-11-24T12:11:52Z

is it still alive?

snpy · 2022-11-24T12:38:53Z

Sorry; slipped my mind.
I'm going to try to have conflicts resolved and move things forward.

Update `master` branch

php-translation#300

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}. php-translation#300

php-translation#300

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). php-translation#300

Let the default configuration overlap with Symfony configuration option default value. php-translation#300

php-translation#300

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). php-translation#300

php-translation#300

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. php-translation#300

php-translation#300

…le into handle-icu-domain

We're now providing getter method name for new field.

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.

snpy · 2022-11-24T22:26:30Z

@bocharsky-bw Changes here should be ready for the review/testing. Thanks in advance.

Update local branch

snpy · 2023-01-11T15:02:41Z

@bocharsky-bw One PHP-CS-Fixer suggestions as applied and one was fetched from the master.
Now all checks should be green all the way.
Thanks.

bocharsky-bw

Thank you for finishing this!

Looks good to me for a minor release

bocharsky-bw · 2023-01-23T09:59:57Z

Thank you for finishing it, Marek!

snpy · 2023-01-23T10:04:12Z

Thanks for merging it.
Sorry it took soooo long.

snpy added 10 commits August 24, 2021 13:10

Remove both "normal" and ICU domain from messages set

b020360

php-translation#300

Add new field and getter based on recently added new config option

3952ec8

php-translation#300

Make sure new_message_format configuration is defined

c7284ed

Let the default configuration overlap with Symfony configuration option default value. php-translation#300

Pass to converter new_message_format configuration

ab6631d

php-translation#300

Let code "breath" a bit

6429fb4

php-translation#300

Make sure we're always aware of ICU domain

ef9c691

php-translation#300

welcoMattic reviewed Aug 30, 2021

View reviewed changes

Persist PHP CS Fixer changes

04a1f1e

php-translation#300

Merge pull request #2 from php-translation/master

0b1317a

Update `master` branch

snpy force-pushed the handle-icu-domain branch from e8c547e to 04a1f1e Compare November 24, 2022 15:25

snpy added 8 commits November 24, 2022 16:26

Remove both "normal" and ICU domain from messages set

c040dab

php-translation#300

Add new field and getter based on recently added new config option

a383c24

php-translation#300

Make sure new_message_format configuration is defined

a39113e

Let the default configuration overlap with Symfony configuration option default value. php-translation#300

Pass to converter new_message_format configuration

592b8f2

php-translation#300

Let code "breath" a bit

ee0cc66

php-translation#300

snpy added 9 commits November 24, 2022 16:27

Make sure we're always aware of ICU domain

d3b44d9

php-translation#300

Persist PHP CS Fixer changes

8802b6f

php-translation#300

Merge branch 'handle-icu-domain' of github.com:kram-soft/symfony-bund…

b4b4f5a

…le into handle-icu-domain

Apply PHP CS fixer automated fix

f2e80e0

Fix main PHPUnit issue

bc9bd5f

We're now providing getter method name for new field.

Fix catalogue counter

1e099ec

Make assertion more wordy

4116221

snpy added 3 commits January 11, 2023 15:57

Merge pull request #3 from php-translation/master

0c31b96

Update local branch

Merge branch 'master' into handle-icu-domain

202af61

Apply PHP-CS-Fixer suggestion

4c52eec

bocharsky-bw approved these changes Jan 12, 2023

View reviewed changes

bocharsky-bw requested a review from welcoMattic January 12, 2023 12:40

bocharsky-bw merged commit 9e2dda3 into php-translation:master Jan 23, 2023

This was referenced Jan 23, 2023

Add support for IntlMessageFormatter #269

Closed

Bundle does not work properly with new Intl ICU translations #300

Closed

ICU - Main topic #399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle does not work properly with new Intl ICU translations #452

Bundle does not work properly with new Intl ICU translations #452

snpy commented Aug 24, 2021

welcoMattic Aug 30, 2021

snpy Aug 30, 2021

welcoMattic Aug 30, 2021

snpy Aug 30, 2021

welcoMattic Aug 30, 2021

snpy Aug 30, 2021

bocharsky-bw commented Jan 20, 2022

bocharsky-bw commented Jan 25, 2022

snpy commented Jan 25, 2022

bocharsky-bw commented Jan 25, 2022

LauLaman commented Aug 19, 2022

snpy commented Aug 20, 2022

wokenlex commented Nov 24, 2022

snpy commented Nov 24, 2022

snpy commented Nov 24, 2022 •

edited

Loading

snpy commented Jan 11, 2023

bocharsky-bw left a comment

bocharsky-bw commented Jan 23, 2023

snpy commented Jan 23, 2023

Bundle does not work properly with new Intl ICU translations #452

Bundle does not work properly with new Intl ICU translations #452

Conversation

snpy commented Aug 24, 2021

welcoMattic Aug 30, 2021

Choose a reason for hiding this comment

snpy Aug 30, 2021

Choose a reason for hiding this comment

welcoMattic Aug 30, 2021

Choose a reason for hiding this comment

snpy Aug 30, 2021

Choose a reason for hiding this comment

welcoMattic Aug 30, 2021

Choose a reason for hiding this comment

snpy Aug 30, 2021

Choose a reason for hiding this comment

bocharsky-bw commented Jan 20, 2022

bocharsky-bw commented Jan 25, 2022

snpy commented Jan 25, 2022

bocharsky-bw commented Jan 25, 2022

LauLaman commented Aug 19, 2022

snpy commented Aug 20, 2022

wokenlex commented Nov 24, 2022

snpy commented Nov 24, 2022

snpy commented Nov 24, 2022 • edited Loading

snpy commented Jan 11, 2023

bocharsky-bw left a comment

Choose a reason for hiding this comment

bocharsky-bw commented Jan 23, 2023

snpy commented Jan 23, 2023

snpy commented Nov 24, 2022 •

edited

Loading