[translation][performances] move loading resources into Translator initialize. #13897
Conversation
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Fixed tickets | #13556 |
| Tests pass? | yes |
| License | MIT |
|
|
| * @param array $options An array of options | ||
| * | ||
| * @throws \InvalidArgumentException | ||
| */ | ||
| public function __construct(ContainerInterface $container, MessageSelector $selector, $loaderIds = array(), array $options = array()) | ||
| public function __construct(ContainerInterface $container, MessageSelector $selector, $loaderIds = array(), $resourceDirs = array(), array $options = array()) |
There was a problem hiding this comment.
Adding the argument in the middle of the list can become an issue for people extending this class.
| @@ -0,0 +1 @@ | |||
| folder: repertoire | |||
There was a problem hiding this comment.
This makes sense ;)
| return 2 === substr_count($file->getBasename(), '.') && preg_match('/\.\w+$/', $file->getBasename()); | ||
| }) | ||
| ->in($this->resourceDirs) | ||
| ; |
There was a problem hiding this comment.
Instead of passing the resource directories, what about passing the list of files and move back this piece of code in the extension? That would make things faster and safe as if the list of files changes, the container is going to be rebuilt anyway.
| @@ -682,23 +683,11 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder | |||
| } | |||
|
|
|||
| // Register translation resources | |||
| $container->setParameter('translator.resource_directories', $dirs); | |||
| if ($dirs) { | |||
|
Thank you @aitboudad. |
…s changed. (aitboudad) This PR was merged into the 2.7 branch. Discussion ---------- [Translation][debug mode] refresh cache when resources is changed. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | #13897 | Tests pass? | yes | License | MIT Commits ------- e36f1a7 [FrameworkBundle][Translation][debug mode] refresh cache when resources is changed.
… to avoid BC. (aitboudad) This PR was merged into the 2.7 branch. Discussion ---------- [Translation] keep old array structure of resourcesFiles to avoid BC. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | #13897 | Tests pass? | yes | License | MIT Commits ------- aa70a94 [Translation] keep old array structure of resourcesFiles to avoid BC.
|
This defers adding the resources until I haven't checked, but I think this re-introduces the bug fixed for the |
|
@aitboudad Could you please check if this PR makes the resource initialization so late that we use wrong or even multiple cache file paths for a given locale and resource set? That would re-introduce a bug already fixed. I don't know if that should be before 2.7 is released? (/cc @fabpot) |
|
Looks like something we might do before 2.7... which will be released in less than 2 weeks from now. |
|
@mpdude ok I'll check it and thanks for reporting this. |
|
@mpdude |
|
Not sure if #14281 is the right thing to revert. The change that (IMO) causes the problem is the optimization made in this PR here. In #14011 (the fix you mentioned above), you basically reverted the optimization for Still, when you're using two different instances of |
|
Well I couldn't find the right solution, I'd prefer using the old name of catalogue without catalogueHash at least in the prod env. private function getCatalogueCachePath($locale)
{
if ($this->debug) {
return $this->cacheDir.'/catalogue.'.$locale.'.php';
}
}right ? |
|
Well you have |
|
@mpdude I'd prefer the safe way as I see some bundles add resources until initializeCatalogue is called see LiipTranslationBundle for example. |
|
fixed by #14725 |