[Translator] Further postpone adding resource files #27063
Conversation
nicolas-grekas
commented
Apr 26, 2018
| Q | A |
|---|---|
| Branch? | 4.1 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #27010 |
| License | MIT |
| Doc PR | - |
| @@ -75,7 +77,7 @@ public function __construct(ContainerInterface $container, MessageFormatterInter | |||
|
|
|||
| $this->options = array_merge($this->options, $options); | |||
| $this->resourceLocales = array_keys($this->options['resource_files']); | |||
| $this->addResourceFiles($this->options['resource_files']); | |||
| $this->resourceFiles = $this->options['resource_files']; | |||
There was a problem hiding this comment.
This will create a copy of $this->options['resource_files'] array, right? Wouldn't it be better for memory to have a $resourceFilesLoaded boolean attribute instead, and use $this->options['resource_files'] later to iterate at addResourceFiles?
There was a problem hiding this comment.
This will only copy the array of there is a modification to the array, if we don't touch to the array the copy-on-write mechanism will just keep a second reference to the same array.
There was a problem hiding this comment.
actually, thanks to copy-on-write, no duplication will ever happen
| @@ -46,6 +46,8 @@ class Translator extends BaseTranslator implements WarmableInterface | |||
| */ | |||
| private $resources = array(); | |||
|
|
|||
| private $resourceFiles; | |||
There was a problem hiding this comment.
I think this could be a "private $resourceFilesLoaded" boolean (see next comment).
| @@ -103,6 +105,9 @@ public function warmUp($cacheDir) | |||
|
|
|||
| public function addResource($format, $resource, $locale, $domain = null) | |||
| { | |||
| if ($this->resourceFiles) { | |||
There was a problem hiding this comment.
This could be "if (!$this->resourceFilesLoaded) {"
| @@ -117,6 +122,9 @@ protected function initializeCatalogue($locale) | |||
|
|
|||
| protected function initialize() | |||
| { | |||
| if ($this->resourceFiles) { | |||
There was a problem hiding this comment.
This could also be "if (!$this->resourceFilesLoaded) { "
| { | ||
| $filesByLocale = $this->resourceFiles; |
There was a problem hiding this comment.
With this assignment, the resources are cloned again, and now we have 3 times the same content for a moment. We already have $this->options['resource_files'] to iterate in the next line
| @@ -103,6 +105,9 @@ public function warmUp($cacheDir) | |||
|
|
|||
| public function addResource($format, $resource, $locale, $domain = null) | |||
| { | |||
| if ($this->resourceFiles) { | |||
| $this->addResourceFiles(); | |||
There was a problem hiding this comment.
do we need to do it here ?
We could ensure we do it only on initialize by calling parent::addResource for the resource files instead of $this->addResource (avoiding to copy them first from resourceFiles to resources, to add them in the parent later.
As initialize will process resourceFiles before resources, the other will be preserved.
There was a problem hiding this comment.
one could call the constructor, then addResource on the object, isn't it?
There was a problem hiding this comment.
(I confirm, and this is already covered by tests :) )
…olas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [Translator] Further postpone adding resource files | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27010 | License | MIT | Doc PR | - Commits ------- 2aa62df [Translator] Further postpone adding resource files