8000 [Translator] Resource loading order depends on kernel.debug setting · Issue #23034 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Translator] Resource loading order depends on kernel.debug setting #23034

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
mpdude opened this issue Jun 2, 2017 · 3 comments
Closed

[Translator] Resource loading order depends on kernel.debug setting #23034

mpdude opened this issue Jun 2, 2017 · 3 comments
Labels
Bug Status: Needs Review Translation

Comments

@mpdude
Copy link
Contributor
mpdude commented Jun 2, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 2.8 (more precisely, problem exists since 2.7)

In 027a747, #13897 was merged. This was a performance improvement: Instead of calling the (expensive?) Translator::addResource method when creating the Translator instance in the DIC, only an array of resources would be passed to Translator::__construct. Translator::loadResources would then iterate over this array and make the actual addResource call, but not unless Translator::initializeCatalogue is called.

Then, e36f1a7 tries to fix a bug with not reloading the messages. It does so by adding another Translator::loadResources call in Translator::loadCatalogue(), but only does this in kernel.debug mode (assuming messages don't change in prod).

Then, in aa70a94, this loadCatalogue overriden method was removed again and the loadResources-if-in-debug code slipped into the constructor.

I admin that I cannot really follow the ideas behind all these changes. However, the bottom line is that in kernel.debug mode, the resources passed to the constructor are turned into addResource() calls immediately, whereas in production mode this is deferred until initialize()/ initializeCatalogue() are called.

This is a problem for other bundles/extensions that add additional resources to the Translator via addResource() calls (for example added to the DIC): Depending on the kernel.debug setting, these resources will be loaded first or last, which makes the resource override (or "lose" against) other message sources.

Assuming that message source priority should not change between debug and prod, I see the following options:

a) Always add resources passed in the resource_files option in the constructor. Probably defeats the initial performance improvement intention?

b) Always defer adding resource_files until initialize() is called. That way, resources added via addResource() by 3rd party bundles always have a lower priority than resource_files.

c) Change the private loadResources() method in a way that "shifts" the resources before those added by addResource() calls (where they would end up if they were added in the constructor).

Additionally, if in production it is no problem to defer this initialization until initialize() is called, why is it necessary to perform this step in the constructor under kernel.debug?

Update: Suggested fix in #23057
@mpdude
Copy link
Contributor Author
mpdude commented Jun 2, 2017

ping @aitboudad – you've been involved in much of this code, maybe you can help

@mpdude mpdude mentioned this issue Jun 3, 2017
Closed
@sstok
Copy link
Contributor
sstok commented Jun 4, 2017

Basically the problem is that injecting the Translator will trigger the loading of resources (which may not be needed for this service). Eg. the translator resources loading must be made lazy (at least for the framework) or we need a wrapper service that will lazy initialize the translator?

@mpdude
Copy link
Contributor Author
mpdude commented Jun 4, 2017

I was suspecting something like that. In #23057, resource loading should still be lazy but with a consistent order in debug/prod environments.

@fabpot fabpot closed this as completed Jun 14, 2017
fabpot pushed a commit that referenced this issue Jun 14, 2017
@mpdude @fabpot
[Translation][FrameworkBundle] Fix resource loading order inconsisten…
2a9e65d 
…cy reported in #23034
fabpot added a commit that referenced this issue Jun 14, 2017
@fabpot
bug #23057 [Translation][FrameworkBundle] Fix resource loading order …
baf988d 
…inconsistency reported in #23034 (mpdude)

This PR was squashed before being merged into the 2.7 branch (closes #23057).

Discussion
----------

[Translation][FrameworkBundle] Fix resource loading order inconsistency reported in #23034

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23034
| License       | MIT
| Doc PR        |

Fixes the bug reported in #23034:

When mixing `addResource()` calls and providing the `resource_files` option, the order in which resources are loaded depends on the `kernel.debug` setting and whether a cache is used.

In particular, when several loaders provide translations for the same message, the one that "wins" may change between development and production mode.

Commits
-------

2a9e65d [Translation][FrameworkBundle] Fix resource loading order inconsistency reported in #23034
fabpot added a commit that referenced this issue Jun 20, 2017
@fabpot
Merge branch '2.7' into 2.8
be6af2e 
* 2.7:
  [Routing] Fix XmlFileLoader exception message
  Sessions: configurable "use_strict_mode" option for NativeSessionStorage
  [FrameworkBundle] [Command] Clean bundle directory, fixes #23177
  Reset redirectCount when throwing exception
  [TwigBundle] Remove template.xml services when templating is disabled
  add content-type header on exception response
  Embedding a response that combines expiration and validation, that should not defeat expiration on the combined response
  Fix two edge cases in ResponseCacheStrategy
  [Routing] Expose request in route conditions, if needed and possible
  [Routing] Expose request in route conditions, if needed and possible
  [Translation][FrameworkBundle] Fix resource loading order inconsistency reported in #23034
  [Filesystem] added workaround in Filesystem::rename for PHP bug
  Add tests for ResponseCacheStrategy to document some more edge cases
  [HttpFoundation] added missing docs
  fixes #21606
  [VarDumper] fixes
  [Security] fix switch user _exit without having current token
xabbuh added a commit that referenced this issue Jun 23, 2017
@xabbuh
Merge branch '2.8' into 3.2
80b114e 
* 2.8: (40 commits)
  Show exception is checked twice in ExceptionController of twig
  allow SSI fragments configuration in XML files
  Display a better error message when the toolbar cannot be displayed
  render hidden _method field in form_rest()
  return fallback locales whenever possible
  [Console] Fix catching exception type in QuestionHelper
  [WebProfilerBundle] Eliminate line wrap on count columnt (routing)
  [Routing] Fix XmlFileLoader exception message
  [Translation] Fix FileLoader::loadResource() php doc
  Sessions: configurable "use_strict_mode" option for NativeSessionStorage
  [FrameworkBundle] [Command] Clean bundle directory, fixes #23177
  Reset redirectCount when throwing exception
  [TwigBundle] Remove template.xml services when templating is disabled
  add content-type header on exception response
  Embedding a response that combines expiration and validation, that should not defeat expiration on the combined response
  Fix two edge cases in ResponseCacheStrategy
  [Routing] Expose request in route conditions, if needed and possible
  [Routing] Expose request in route conditions, if needed and possible
  [Translation][FrameworkBundle] Fix resource loading order inconsistency reported in #23034
  [Filesystem] added workaround in Filesystem::rename for PHP bug
  ...
This was referenced Jul 3, 2017
Merged
Merged
Merged
Merged
ostrolucky pushed a commit to ostrolucky/symfony that referenced this issue Mar 25, 2018
@xabbuh
Merge branch '2.8' into 3.2
84adcf2 
* 2.8: (40 commits)
  Show exception is checked twice in ExceptionController of twig
  allow SSI fragments configuration in XML files
  Display a better error message when the toolbar cannot be displayed
  render hidden _method field in form_rest()
  return fallback locales whenever possible
  [Console] Fix catching exception type in QuestionHelper
  [WebProfilerBundle] Eliminate line wrap on count columnt (routing)
  [Routing] Fix XmlFileLoader exception message
  [Translation] Fix FileLoader::loadResource() php doc
  Sessions: configurable "use_strict_mode" option for NativeSessionStorage
  [FrameworkBundle] [Command] Clean bundle directory, fixes symfony#23177
  Reset redirectCount when throwing exception
  [TwigBundle] Remove template.xml services when templating is disabled
  add content-type header on exception response
  Embedding a response that combines expiration and validation, that should not defeat expiration on the combined response
  Fix two edge cases in ResponseCacheStrategy
  [Routing] Expose request in route conditions, if needed and possible
  [Routing] Expose request in route conditions, if needed and possible
  [Translation][FrameworkBundle] Fix resource loading order inconsistency reported in symfony#23034
  [Filesystem] added workaround in Filesystem::rename for PHP bug
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Translation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @sstok @mpdude @xabbuh @carsonbot
0