Skip to content
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

[FrameworkBundle][Translation] Invalidate cached catalogues when the scanned directories change #34129

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@fancyweb
Copy link
Contributor

fancyweb commented Oct 26, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets #33992
License MIT
Doc PR -

The cache file name needs to depend on the scanned directories list. Otherwise, when a new directory is added, even if the container is rebuilt and the FWB Translator gets the new scanned directories list, the cached catalogue name is still the same and is resolved accordingly.

An alternative would be to make the Translation Translator getCatalogueCachePath() method and fallbackLocales @internal and protected to just override everything in the FWB Translator. The cacheVary argument has the benefit to be reusable by all the Translation component users.

Note that there is a negative minor performance impact that increases when the list of scanned directories grows.

…scanned directories change
@fancyweb fancyweb force-pushed the fancyweb:translation-invalidate-cache branch from 859b50b to 6cbee09 Oct 26, 2019
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Oct 27, 2019
@@ -86,7 +88,7 @@ class Translator implements LegacyTranslatorInterface, TranslatorInterface, Tran
/**
* @throws InvalidArgumentException If a locale contains invalid characters
*/
public function __construct(?string $locale, MessageFormatterInterface $formatter = null, string $cacheDir = null, bool $debug = false)
public function __construct(?string $locale, MessageFormatterInterface $formatter = null, string $cacheDir = null, bool $debug = false, array $cacheVary = [])

This comment has been minimized.

Copy link
@fancyweb

fancyweb Oct 29, 2019

Author Contributor

I was looking at this again an I'm not sure if we can add a new constructor arg on 4.3?

This comment has been minimized.

Copy link
@yceruto

yceruto Oct 29, 2019

Member

As long as it's added with a default value at the end, I think yes.

This comment has been minimized.

Copy link
@yceruto

yceruto Oct 29, 2019

Member

Although it would look like a new feature.

@fabpot
fabpot approved these changes Nov 6, 2019
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Nov 6, 2019

Thank you @fancyweb.

fabpot added a commit that referenced this pull request Nov 6, 2019
…s when the scanned directories change (fancyweb)

This PR was merged into the 4.3 branch.

Discussion
----------

[FrameworkBundle][Translation] Invalidate cached catalogues when the scanned directories change

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #33992
| License       | MIT
| Doc PR        | -

The cache file name needs to depend on the scanned directories list. Otherwise, when a new directory is added, even if the container is rebuilt and the `FWB Translator` gets the new scanned directories list, the cached catalogue name is still the same and is resolved accordingly.

An alternative would be to make the `Translation Translator` `getCatalogueCachePath()` method and `fallbackLocales` `@Internal` and `protected` to just override everything in the `FWB Translator`. The `cacheVary` argument has the benefit to be reusable by all the `Translation` component users.

Note that there is a negative minor performance impact that increases when the list of scanned directories grows.

Commits
-------

6cbee09 [FrameworkBundle][Translation] Invalidate cached catalogues when the scanned directories change
@fabpot fabpot merged commit 6cbee09 into symfony:4.3 Nov 6, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Your code looks good.
Details
@fancyweb fancyweb deleted the fancyweb:translation-invalidate-cache branch Nov 7, 2019
@fabpot fabpot referenced this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.