8000 [translation][performances] move loading resources into Translator initialize. by aitboudad · Pull Request #13897 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[translation][performances] move loading resources into Translator initialize. #13897

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[translation][performances] move loading resources into Translator initialize. #13897

wants to merge 4 commits into from

Conversation

@aitboudad
Copy link
Contributor
@aitboudad aitboudad commented Mar 11, 2015
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets #13556
Tests pass? yes
License MIT

@aitboudad aitboudad mentioned this pull request Mar 11, 2015
Closed
@aitboudad
Copy link
Contributor Author
aitboudad commented Mar 11, 2015

selection_002

xabbuh
xabbuh reviewed Mar 11, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php Outdated
Copy link
Member
@xabbuh xabbuh Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the argument in the middle of the list can become an issue for people extending this class.

Copy link
Contributor Author
@aitboudad aitboudad Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

fabpot
fabpot reviewed Mar 11, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Resources/translations/messages.fr.yml Outdated
Copy link
Member
@fabpot fabpot Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be répertoire :)

Copy link
Contributor Author
@aitboudad aitboudad Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense ;)

fabpot
fabpot reviewed Mar 11, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php Outdated
Copy link
Member
@fabpot fabpot Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fabpot
fabpot reviewed Mar 11, 2015
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
Copy link
Member
@fabpot fabpot Mar 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if is not needed anymore.

@fabpot
Copy link
Member
fabpot commented Mar 13, 2015

Thank you @aitboudad.

@fabpot fabpot closed this in 027a747 Mar 13, 2015
@aitboudad aitboudad deleted the issue_13556 branch March 13, 2015 13:38
@aitboudad aitboudad mentioned this pull request Mar 21, 2015
Merged
fabpot added a commit that referenced this pull request Mar 23, 2015
@fabpot
minor #14011 [Translation][debug mode] refresh cache when resources i…
e99c09e 
…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.
@aitboudad aitboudad mentioned this pull request Mar 23, 2015
Merged
fabpot added a commit that referenced this pull request Mar 24, 2015
@fabpot
minor #14022 [Translation] keep old array structure of resourcesFiles…
2f39901 
… 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.
@aitboudad aitboudad mentioned this pull request Apr 7, 2015
Closed
@mpdude
Copy link
Contributor
mpdude commented May 13, 2015

This defers adding the resources until initializeCatalogue is called. That means that at the time the cache path is determined the resources will still be uninitialized.

I haven't checked, but I think this re-introduces the bug fixed for the Components/Translation/Translator in #13074 back into the FrameworkBundle/Translator/Translator.

@mpdude
Copy link
Contributor
mpdude commented May 18, 2015

@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)

@fabpot
Copy link
Member
fabpot commented May 19, 2015

Looks like something we might do before 2.7... which will be released in less than 2 weeks from now.

@aitboudad
Copy link
Contributor Author
aitboudad commented May 19, 2015

@mpdude ok I'll check it and thanks for reporting this.

@aitboudad
Copy link
Contributor Author
aitboudad commented May 20, 2015

@mpdude the only way to solve the issue is to revert #14281, is it ok for you ?
sorry I think you missed this fix Translator.php#L68-L70

@mpdude
Copy link
Contributor
mpdude commented May 20, 2015

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 kernel.debug mode.

Still, when you're using two different instances of FrameworkBundle/.../Translator they will mess up each other's cache file because both determine the cache file name before any resources are added.

@aitboudad
Copy link
Contributor Author
aitboudad commented May 20, 2015

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 ?

@mpdude
Copy link
Contributor
mpdude commented May 21, 2015

Well you have $this->resourceFiles, can't you just use that (plus the locale) to derive the cache file name?

@aitboudad
Copy link
Contributor Author
aitboudad commented May 22, 2015

@mpdude I'd prefer the safe way as I see some bundles add resources until initializeCatalogue is called see LiipTranslationBundle for example.

@aitboudad aitboudad mentioned this pull request May 22, 2015
Closed
@aitboudad
Copy link
Contributor Author
aitboudad commented May 22, 2015

fixed by #14725

@mpdude mpdude mentioned this pull request Jun 2, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aitboudad @fabpot @mpdude @xabbuh
0