Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for circular reference issue after upgrading to symfony/translation 4.2.0 #156

Merged
merged 8 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@mikoczy
Copy link
Contributor

commented Dec 4, 2018

#155

Show resolved Hide resolved src/DI/TranslationExtension.php Outdated
@@ -141,8 +142,11 @@ public function loadConfiguration()
$builder->addDefinition($this->prefix('selector'))
->setClass(MessageSelector::class);
$builder->addDefinition($this->prefix('intlFormatter'))
->setClass(IntlFormatter::class);

This comment has been minimized.

Copy link
@enumag

enumag Dec 4, 2018

Member

If I now create a service that requires IntlFormatterInterface, will nette autowire IntlFormatter, MessageFormatter or throw an error? I suspect it will throw an error with the current configuration. It should autowire IntlFormatter in my opinion.

This comment has been minimized.

Copy link
@enumag

enumag Dec 4, 2018

Member

Also for compatibility with older symfony, add an interface_exists check.

This comment has been minimized.

Copy link
@mikoczy

mikoczy Dec 4, 2018

Author Contributor

Yes, it will throw an error. It looks (in the symfony/translation sources) like this interface is not directly used, only through the MessageFormatter proxy,

This comment has been minimized.

Copy link
@mikoczy

mikoczy Dec 4, 2018

Author Contributor

To the autowiring: I could only achieve this (autowiring IntlFormatter) if I disable the autowiring of the MessageFormatter service, but that is used in the Translator service, so I cannot do that..

This comment has been minimized.

Copy link
@enumag

enumag Dec 4, 2018

Member

Hmm... I didn't work with nette for years but something like this might work I think:

$builder->addDefinition($this->prefix('intlFormatter'))
    ->setClass(IntlFormatterInterface::class)
    ->setFactory(IntlFormatter::class);

$builder->addDefinition($this->prefix('messageFormatter'))
    ->setClass(MessageFormatterInterface::class)
    ->setFactory(MessageFormatter::class);

But there were some changes regarding this, especially calling setClass with interface might be deprecated for nette/di 3.0 I think? There is certainly some replacement.

With this it might be possible to remove the custom parameters for MessageFormatter. If not, use named parameters instead of numbered parameters. Customizing selector should not be necessary I think...

This comment has been minimized.

Copy link
@mikoczy

mikoczy Dec 4, 2018

Author Contributor

Yes, you're right, this works.. But I had to change the MessageFormatter in Translator to MessageFormatterInterface for this to work, I'm not sure if the tests will pass..

This comment has been minimized.

Copy link
@mikoczy

mikoczy Dec 4, 2018

Author Contributor

And this can be a breaking change if someone changed the formatter service in nette through class: instead of factory:

if (!$this->formatter instanceof ChoiceMessageFormatterInterface) {
$result = $id;
if ($this->panel !== NULL) {
$this->panel->choiceError(new \Symfony\Component\Translation\Exception\LogicException(sprintf('The formatter "%s" does not support plural translations.', get_class($this->formatter))), $domain);

This comment has been minimized.

Copy link
@mikoczy

mikoczy Dec 4, 2018

Author Contributor

No, I did not. My intention with this pull request was to fix an error, so we can use this library after a composer update without any application errors. Fixing the deprecated notices will be a quite big change, since there are new interfaces, some of the old ones are deprecated, some methods in the Translator service are also deprecated, which looks like a breaking change to me. The TranslationExtension uses also deprecated methods (setClass).

This comment has been minimized.

Copy link
@enumag

enumag Dec 4, 2018

Member

Ok, that makes sense.

@enumag

enumag approved these changes Dec 4, 2018

@enumag

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

@namo-R @tvaliasek Can you guys review this and try it in your applications?

@tvaliasek

This comment has been minimized.

Copy link

commented Dec 4, 2018

@enumag No circular reference error in my case. At first sight it seems to be ok. Templates and components are all translated correctly.

php 7.1
symfony/translation 4.2.0
kdyby/translation 2.5.1
symfony/yaml 4.2.0

@namo-R

This comment has been minimized.

Copy link

commented Dec 5, 2018

@enumag Looks good in our case.

@enumag enumag merged commit 6b0721c into Kdyby:master Dec 5, 2018

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 69.631%
Details
Scrutinizer 5 new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@enumag

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

Thank you @mikoczy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.