Skip to content

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

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 3 commits into from
Closed

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

wants to merge 3 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 Oct 19, 2015
Closed
@aitboudad
Copy link
Contributor Author

@symfony/deciders
I really want to see this in 2.8 which will allows us to simplify a bunch of things in the translator component.

@fabpot
Copy link
Member

fabpot commented Oct 19, 2015

I'm ok with the decoupling but hadn't had time to review the PR.

fabpot
fabpot reviewed Oct 19, 2015
View reviewed changes
src/Symfony/Component/Translation/MessageCatalogueProvider/ResourceMessageCatalogueProvider.php
*
* @author Abdellatif Ait boudad <a.aitboudad@gmail.com>
*/
class ResourceMessageCatalogueProvider implements MessageCatalogueProviderInterface
Copy link
Member

Choose a reason for hiding this comment

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

What about just MessageCatalogueProvider as this is really the default implementation that almost everyone is going to use, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'll rename ContainerAwareResourceMessageCatalogueProvider too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@aitboudad aitboudad force-pushed the translation_loader_resolver branch from 452c426 to 5d66f84 Compare October 19, 2015 15:09
@linaori
Copy link
Contributor

linaori commented Oct 20, 2015

Even though this PR is rather big and my understanding of the changes minimal, I like what I see here. If possible I would indeed like to see this in 2.8 so it prevents a big BC layer scattered around for a while. I'm really happy to see the framework translator go, it was a pretty confusing setup and might inspire the router to be decoupled like that in the future as well.

@mpdude
Copy link
Contributor

mpdude commented Oct 20, 2015

Are there major differences to #14671? We've already had a lot of discussion and suggestions there.

@aitboudad aitboudad force-pushed the translation_loader_resolver branch from 5d66f84 to 7be546f Compare October 20, 2015 10:52
@fabpot
Copy link
Member

fabpot commented Jan 27, 2016

@aitboudad Now that 3.0 is out, what about this one?

@aitboudad
Copy link
Contributor Author

see #17563, it only needs to be reviewed

@aitboudad aitboudad closed this Jan 27, 2016
@aitboudad aitboudad mentioned this pull request Apr 3, 2017
Closed
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
Status: Needs Review Translation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aitboudad @fabpot @linaori @mpdude @carsonbot