[FrameworkBundle] don't load translator services if not required #20928

xabbuh · 2016-12-14T21:08:49Z

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

One step further could be to remove all the loader services (or not register them at all) if only the identity translator is used (i.e. when only forms are enabled, but not translations), but that could be seen as a BC break.

fabpot · 2016-12-15T06:51:13Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -87,7 +87,7 @@ public function load(array $configs, ContainerBuilder $container)
        // A translator must always be registered (as support is included by
        // default in the Form component). If disabled, an identity translator
        // will be used and everything will still work as expected.
-        if (class_exists('Symfony\Component\Translation\Translator') || $this->isConfigEnabled($container, $config['form'])) {
+        if ($this->isConfigEnabled($container, $config['translator']) || $this->isConfigEnabled($container, $config['form'])) {
            if (!class_exists('Symfony\Component\Translation\Translator')) {
                throw new LogicException('Form support cannot be enabled as the Translation component is not installed.');


The error message should probably be different for the first case.

Indeed, updated it.

fabpot · 2016-12-15T07:22:03Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -87,8 +87,12 @@ public function load(array $configs, ContainerBuilder $container)
        // A translator must always be registered (as support is included by
        // default in the Form component). If disabled, an identity translator
        // will be used and everything will still work as expected.
-        if (class_exists('Symfony\Component\Translation\Translator') || $this->isConfigEnabled($container, $config['form'])) {
-            if (!class_exists('Symfony\Component\Translation\Translator')) {
+        if ($this->isConfigEnabled($container, $config['translator']) || $this->isConfigEnabled($container, $config['form'])) {


What about remove this condition then?

We still need it to not always load the translation.xml file (in case both the form and the translator section are not enabled).

ah right, Github did not show the whole if code, so I as confused.

fabpot · 2016-12-15T07:39:41Z

I would indeed go one step further and only register the identity if translator is not enabled explicitly. We should then document this change in the CHANGELOG, but I think we should avoid as much as possible auto-registration like we have now. This will be even more important with Symfony Flex as we don't require symfony/symfony and ask users to explicitly register anything they want to use.

xabbuh · 2016-12-15T13:12:09Z

Fair enough, I included that change too.

BPScott · 2016-12-16T01:08:50Z

Woo, thank you @xabbuh! The identity translation stuff seemed a little fiddly and I'm totally unfamiliar with it so I didn't dare open a PR myself :)

A small review: I spy the class already has formConfigEnabled and translationConfigEnabled properties that are set elsewhere, could the translation activation checks be done a little later in this file after those values are set to avoid repeated calls to isConfigEnabled()? It might also be adding an extra level of nesting to the if structuring to avoid multiple calls to class_exists('Symfony\Component\Translation\Translator') too. Something like:

if ($this->isConfigEnabled($container, $config['translator']) || $this->isConfigEnabled($container, $config['form']) || $this->isConfigEnabled($container, $config['validation'])) {
    if (!class_exists('Symfony\Component\Translation\Translator') {
        if ($this->isConfigEnabled($container, $config['translator'])) {
            throw new LogicException('Translation support cannot be enabled as the Translation component is not installed.');
        }
        if ($this->isConfigEnabled($container, $config['form'])) {
            throw new LogicException('Form support cannot be enabled as the Translation component is not installed.');
        }
        if ($this->isConfigEnabled($container, $config['validation'])) {
            throw new LogicException('Validator support cannot be enabled as the Translation component is not installed.');
        }
    }

    $loader->load('identity_translator.xml');
}

xabbuh · 2016-12-16T06:25:43Z

@BPScott I don't think that's necessary as this code is only executed during compilation.

linaori · 2016-12-16T09:28:15Z

@xabbuh could this cause issues related to: #20949 ?

xabbuh · 2016-12-16T10:29:57Z

@iltar This would solve #20949. Well, at least it will provide a more meaningful error message.

aitboudad

LGTM

fabpot · 2016-12-16T16:26:33Z

Thank you @xabbuh.

…t required (xabbuh) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] don't load translator services if not required | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20791 | License | MIT | Doc PR | One step further could be to remove all the loader services (or not register them at all) if only the identity translator is used (i.e. when only forms are enabled, but not translations), but that could be seen as a BC break. Commits ------- 1e67155 don't load translator services if not required

inelgnu · 2017-01-21T22:48:26Z

@xabbuh "templating.helper.translator" service has a dependency on translator and will not work if templating engine is set to "php" (see templating_php.xml)
cc @fabpot @aitboudad

aitboudad · 2017-01-22T11:29:40Z

@inalgnu can you provide a PR that fixes the issue ?

inelgnu · 2017-01-22T22:02:16Z

@aitboudad done here #21374

xabbuh · 2017-03-15T12:27:04Z

see #22006

…s disabled (xabbuh) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] remove translator helper if Translator is disabled | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20928 (comment), #21374 | License | MIT | Doc PR | Commits ------- 25ea510 remove translator helper if Translator is disabled

…s disabled (xabbuh) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] remove translator helper if Translator is disabled | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#20928 (comment), #21374 | License | MIT | Doc PR | Commits ------- 25ea510ba4 remove translator helper if Translator is disabled

carsonbot added Status: Needs Review FrameworkBundle Feature labels Dec 14, 2016

xabbuh mentioned this pull request Dec 14, 2016

[FrameworkBundle][Translation] Don't add Translation into DI when it is disabled in Config #20791

Closed

fabpot reviewed Dec 15, 2016

View reviewed changes

xabbuh force-pushed the issue-20791 branch from 7e9151a to 551dc32 Compare December 15, 2016 07:17

fabpot reviewed Dec 15, 2016

View reviewed changes

xabbuh force-pushed the issue-20791 branch 2 times, most recently from 8711b67 to 1f86628 Compare December 15, 2016 12:58

xabbuh mentioned this pull request Dec 16, 2016

[3.2] The service "validator.builder" has a dependency on a non-existent service "translator" #20949

Closed

don't load translator services if not required

1e67155

xabbuh force-pushed the issue-20791 branch from 1f86628 to 1e67155 Compare December 16, 2016 13:05

aitboudad approved these changes Dec 16, 2016

View reviewed changes

fabpot merged commit 1e67155 into symfony:master Dec 16, 2016

xabbuh deleted the issue-20791 branch December 16, 2016 16:38

inelgnu mentioned this pull request Jan 22, 2017

Force load translator since templating is enabled with php engine #21374

Closed

xabbuh mentioned this pull request Mar 15, 2017

[FrameworkBundle] remove translator helper if Translator is disabled #22006

Merged

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

ronisaha mentioned this pull request Jun 8, 2017

Hard dependency with Symfony 3.3 Translator Service stwe/DatatablesBundle#627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] don't load translator services if not required #20928

[FrameworkBundle] don't load translator services if not required #20928

xabbuh commented Dec 14, 2016

fabpot Dec 15, 2016

xabbuh Dec 15, 2016

fabpot Dec 15, 2016

xabbuh Dec 15, 2016

fabpot Dec 15, 2016

fabpot commented Dec 15, 2016

xabbuh commented Dec 15, 2016

BPScott commented Dec 16, 2016 •

edited

Loading

xabbuh commented Dec 16, 2016

linaori commented Dec 16, 2016 •

edited

Loading

xabbuh commented Dec 16, 2016

aitboudad left a comment

fabpot commented Dec 16, 2016

inelgnu commented Jan 21, 2017

aitboudad commented Jan 22, 2017

inelgnu commented Jan 22, 2017

xabbuh commented Mar 15, 2017

[FrameworkBundle] don't load translator services if not required #20928

[FrameworkBundle] don't load translator services if not required #20928

Conversation

xabbuh commented Dec 14, 2016

fabpot Dec 15, 2016

Choose a reason for hiding this comment

xabbuh Dec 15, 2016

Choose a reason for hiding this comment

fabpot Dec 15, 2016

Choose a reason for hiding this comment

xabbuh Dec 15, 2016

Choose a reason for hiding this comment

fabpot Dec 15, 2016

Choose a reason for hiding this comment

fabpot commented Dec 15, 2016

xabbuh commented Dec 15, 2016

BPScott commented Dec 16, 2016 • edited Loading

xabbuh commented Dec 16, 2016

linaori commented Dec 16, 2016 • edited Loading

xabbuh commented Dec 16, 2016

aitboudad left a comment

Choose a reason for hiding this comment

fabpot commented Dec 16, 2016

inelgnu commented Jan 21, 2017

aitboudad commented Jan 22, 2017

inelgnu commented Jan 22, 2017

xabbuh commented Mar 15, 2017

BPScott commented Dec 16, 2016 •

edited

Loading

linaori commented Dec 16, 2016 •

edited

Loading