Skip to content

[Translator][fallback catalogues] fixed circular reference. #15527

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

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

aitboudad
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets ~
Tests pass? yes
License MIT

@kimausloos
Copy link

This fixes issue #15522

@aitboudad aitboudad changed the title [Translator][fallback catalogues] fixed circular reference. [WIP][Translator][fallback catalogues] fixed circular reference. Aug 17, 2015
@aitboudad aitboudad force-pushed the translator_circular_reference branch from ea9c63d to 79e29c1 Compare September 2, 2015 10:03
@aitboudad aitboudad changed the title [WIP][Translator][fallback catalogues] fixed circular reference. [Translator][fallback catalogues] fixed circular reference. Sep 2, 2015
@aitboudad
Copy link
Contributor Author

@symfony/deciders can someone review please :)

@stof
Copy link
Member

stof commented Sep 2, 2015

what happens if you defined array('fr', 'en') as fallback locales and you load either fr or en as fallback locales. what would be the fallbacks for each of them ?

@aitboudad
Copy link
Contributor Author

@stof
fr -> en as fallback
en -> fr as fallback

@stof
Copy link
Member

stof commented Sep 2, 2015

well, this looks weird to me though, as the fr fallback of en will be different from the fr catalogue used anywhere else. But it is indeed one of the possible implementations. the other one could be to consider that en does not fallback to anything here, as it is itself the last fallback locale.

Anyway it is a different topic (and a behavior change, so not for 2.3), so it should be discussed separately.

👍 for this
status: reviewed

@fabpot
Copy link
Member

fabpot commented Sep 2, 2015

Thank you @aitboudad.

@fabpot fabpot merged commit 79e29c1 into symfony:2.3 Sep 2, 2015
fabpot added a commit that referenced this pull request Sep 2, 2015
…. (aitboudad)

This PR was merged into the 2.3 branch.

Discussion
----------

[Translator][fallback catalogues] fixed circular reference.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Fixed tickets  | ~
| Tests pass?   | yes
| License       | MIT

Commits
-------

79e29c1 [Translator][fallback catalogues] fixed circular reference.
@aitboudad aitboudad deleted the translator_circular_reference branch September 2, 2015 15:10
@c960657
Copy link
Contributor

c960657 commented Jan 28, 2016

I believe this caused a regression: #17596

fabpot added a commit that referenced this pull request Jan 31, 2016
… catalogue (c960657)

This PR was merged into the 2.3 branch.

Discussion
----------

[Translation] Add resources from fallback locale to parent catalogue

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

The resources representing a locale includes those of the fallback locale. However, `getCatalogue()->getResources()` only returns the resources belonging specifically to the selected locale.

Example: The locale `en_GB` falls back to `en`. I use the locale `en_GB`. During development, when I modify the `en_GB` translation file, the changes appear instantly when reloading the page. If I modify the `en` translation file, I need to manually clear the cache in order for the new translation to appear.

I believe this is a regression that was introduced in #15527.

This patch is for the 2.3 branch. For 2.6 and later, the test can be updated to use the getCatalogue() method instead of using ReflectionProperty.

Commits
-------

f7f82fa [Translation] Add resources from fallback locale
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants