Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] don't load translator services if not required#20928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| 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')) { | ||
| thrownewLogicException('Form support cannot be enabled as the Translation component is not installed.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The error message should probably be different for the first case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, updated it.
| // 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'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about remove this condition then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We still need it to not always load thetranslation.xml file (in case both theform and thetranslator section are not enabled).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah right, Github did not show the whole if code, so I as confused.
fabpot commentedDec 15, 2016
I would indeed go one step further and only register the identity if |
8711b67 to1f86628Comparexabbuh commentedDec 15, 2016
Fair enough, I included that change too. |
BPScott commentedDec 16, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 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'])) {thrownewLogicException('Translation support cannot be enabled as the Translation component is not installed.'); }if ($this->isConfigEnabled($container,$config['form'])) {thrownewLogicException('Form support cannot be enabled as the Translation component is not installed.'); }if ($this->isConfigEnabled($container,$config['validation'])) {thrownewLogicException('Validator support cannot be enabled as the Translation component is not installed.'); } }$loader->load('identity_translator.xml');} |
xabbuh commentedDec 16, 2016
@BPScott I don't think that's necessary as this code is only executed during compilation. |
linaori commentedDec 16, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
xabbuh commentedDec 16, 2016
aitboudad left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
fabpot commentedDec 16, 2016
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 commentedJan 21, 2017
@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) |
aitboudad commentedJan 22, 2017
@inalgnu can you provide a PR that fixes the issue ? |
inelgnu commentedJan 22, 2017
@aitboudad done here#21374 |
xabbuh commentedMar 15, 2017
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
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.