Skip to content

[Translator] move loading catalogue out of Translator. #17563

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Translator] move loading catalogue out of Translator. #17563

wants to merge 2 commits into from

Conversation

aitboudad
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Fixed tickets ~
Tests pass? yes
License MIT

Before

$translator = new Translator('fr');
$translator->addLoader('array', new ArrayLoader());
$translator->addResource('array', array('Hello World!' => 'Bonjour'), 'fr');

echo $translator->trans('Hello World!')."\n";

After

$resourceCatalogue = new ResourceMessageCatalogueProvider();
$resourceCatalogue->addLoader('array', new ArrayLoader());
$resourceCatalogue->addResource('array', array('Hello World!' => 'Bonjour'), 'fr');
$translator = new Translator('fr', $resourceCatalogue);

echo $translator->trans('Hello World!')."\n";

@aitboudad aitboudad mentioned this pull request Jan 27, 2016
Closed
xabbuh
xabbuh reviewed Jan 30, 2016
View reviewed changes
src/Symfony/Bridge/Twig/Tests/Extension/TranslationExtensionTest.php Outdated
@@ -87,17 +87,17 @@ public function getTransTests()

// transchoice
array('{% transchoice count from "messages" %}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples{% endtranschoice %}',
'There is no apples', array('count' => 0)),
'There is no apples', array('count' => 0), ),
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, it is an issue in cs-fixer

@aitboudad aitboudad force-pushed the translation_resolve_loader branch from a5808e2 to e63e1e5 Compare January 30, 2016 16:56
@aitboudad aitboudad added this to the 3.1 milestone Feb 3, 2016
@aitboudad aitboudad removed this from the 3.1 milestone Sep 19, 2016
@aitboudad aitboudad mentioned this pull request Nov 7, 2016
Closed
@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 6, 2016
@koemeet
Copy link

koemeet commented Jan 19, 2017

Would love to see this in the translation component! ❤️

@aitboudad Do you still plan on making this ready?

@lunetics
Copy link

lunetics commented Apr 3, 2017

looks good, any updates on that PR

@aitboudad
Copy link
Contributor Author

aitboudad commented Apr 3, 2017 •
edited
Loading

I wish to accomplish this part but I need some feedback's from the core team as I don't want to spend much more time like I did before (#16286, #14671, #13986) and at end it's ignored

@sstok sstok mentioned this pull request Aug 8, 2017
Closed
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 8, 2017 •
edited
Loading

@sstok provided a link to this issue, which then triggers this naive question: what is this supposed to solve?
Is it for loading catalogues at runtime with different strategies?
If yes, then I think this may not be a good idea at all, thanks to PHP7.
On PHP7, arrays are stored in shared memory. There is no more efficient strategy, since even for big catalogues, they cost just zero per request.

@mpdude
Copy link
Contributor

mpdude commented Aug 8, 2017

@nicolas-grekas you mean arrays initialized statically in code are shared, similar to code in the opcache?

@nicolas-grekas
Copy link
Member

yes, that's what I mean, see https://blog.blackfire.io/php-7-performance-improvements-immutable-arrays.html

@sstok
Copy link
Contributor

sstok commented Aug 9, 2017 •
edited
Loading

Moving the MessageCatalogue outside of the Translator allows to make it possible to use a Lazy loading MessageCatalogueProvider (eg. ServiceProvider based or Cache decorated) so you don't have to register all the Loaders every time this service is initialized (once per request, but still).

Minor correction, this makes it possible to use a different MessageCatalogueProvider that can use a more optimized system (rather then then only static arrays, especially for large Catalogues or Catalogues that change often).

@aitboudad
Copy link
Contributor Author

The main reason is delegating the loading message catalogue (caching, loaders, locale fallback) into separate class which allow us a bunch of things:

I'll try to split this PR in small commits for easy review but before let me know WDYT in overall.

@Nyholm
Copy link
Member

Nyholm commented Aug 11, 2017

👍

@nicolas-grekas
Copy link
Member

rebase needed then :)

@nicolas-grekas
Copy link
Member

@aitboudad still wanting to work on this? Could you please rebase before we enter feat. freeze? (end of week)

@koemeet
Copy link

koemeet commented Sep 26, 2017

gogogoo! finally its happening 🍺

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017
@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018
@fabpot
Copy link
Member

fabpot commented Jul 19, 2018

Shall we close? Anyone willing to take over?

@fabpot fabpot closed this Apr 1, 2019
@aitboudad aitboudad deleted the translation_resolve_loader branch April 1, 2019 19:29
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement Performance Status: Needs Review Translation
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.