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

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

Closed
mpdude opened this Issue Jun 2, 2017 · 3 comments

Comments

Projects
None yet
5 participants
@mpdude
Contributor

mpdude commented Jun 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Jun 2, 2017

Contributor

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

Contributor

mpdude commented Jun 2, 2017

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

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jun 4, 2017

Contributor

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?

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

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Jun 4, 2017

Contributor

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

Contributor

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 Jun 14, 2017

fabpot added a commit that referenced this issue Jun 14, 2017

bug #23057 [Translation][FrameworkBundle] Fix resource loading order …
…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

Merge branch '2.7' into 2.8
* 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

Merge branch '2.8' into 3.2
* 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
  ...

ostrolucky pushed a commit to ostrolucky/symfony that referenced this issue Mar 25, 2018

Merge branch '2.8' into 3.2
* 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
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment