8000 Revert #34797 "Fixed translations file dumper behavior" and fix #34713 by yceruto · Pull Request #35351 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Revert #34797 "Fixed translations file dumper behavior" and fix #34713 #35351

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
Merged
merged 2 commits into from
Jan 21, 2020
Merged

Revert #34797 "Fixed translations file dumper behavior" and fix #34713 #35351

merged 2 commits into from
Jan 21, 2020

Conversation

yceruto
Copy link
Member
@yceruto yceruto commented Jan 15, 2020 •
edited
Loading
Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #35264
License MIT
Doc PR -

Revert #34797

See also #35328

It's very likely that the new way will be completely different from this one that is being reverted. That's why I'm reverting rather than fixing it.

@yceruto yceruto added this to the 4.4 milestone Jan 15, 2020
This was referenced Jan 15, 2020
Closed
Closed
@ruudk
Copy link
Contributor
ruudk commented Jan 15, 2020

Can we somehow add a test to make sure we don't end up in the same situation once this revert is reverted again? Imho dumping a file should not flip it to +intl-icu. Only when you manually specify so.

@yceruto
Copy link
Member Author
yceruto commented Jan 15, 2020

@ruudk I'm not sure that a test can guarantee that you will end up with a percentage placeholder in a '+intl-icu' translation file.

Imho dumping a file should not flip it to +intl-icu

It's not the case anymore (after reverting it).

@fabpot
Copy link
Member
fabpot commented Jan 15, 2020

Which PR does it revert?

@yceruto yceruto changed the title Revert "Fixed translations file dumper behavior" Revert #34797 "Fixed translations file dumper behavior" Jan 15, 2020
@yceruto
Copy link
Member Author
yceruto commented Jan 15, 2020

Which PR does it revert?

Title and description updated with the PR number that has being reverted.

@fabpot
Copy link
Member
fabpot commented Jan 15, 2020

So, it means that you are re-introducing the bug I've reported in #34713, right?

@yceruto
Copy link
Member Author
yceruto commented Jan 15, 2020

So, it means that you are re-introducing the bug I've reported in #34713, right?

Yes, we need to re-think another way to fix it (the current one is breaking BC).

@yceruto
Copy link
Member Author
yceruto commented Jan 15, 2020

And it's very likely that the new way will be completely different from this one that is being reverted. That's why I'm reverting rather than fixing it.

@yceruto
Copy link
Member Author
yceruto commented Jan 15, 2020 •
edited
Loading

I also can't provide "the another way" in the near future, sorry :(
any help on this is welcome

@yceruto yceruto changed the title Revert #34797 "Fixed translations file dumper behavior" Revert #34797 "Fixed translations file dumper behavior" and fix #34713 Jan 20, 2020
@yceruto
Copy link
Member Author
yceruto commented Jan 20, 2020

@fabpot now #34713 is being fixed here too. Only new translations are taken into account because #35328.

@XWB
Copy link
Contributor
XWB commented Jan 20, 2020 •
edited
Loading

We installed php-intl to use the Twig intl extension because we want to format numbers according to the current locale. This works fine.

As a side effect, I noticed that our translation files are now named messages+intl-icu.nl.xlf instead of messages.nl.xlf. We use https://github.com/php-translation/symfony-bundle to download translations from an external API. File names are created automatically.

That means our translation files are broken because the message formatter does not recognize the %% parameters in the *intl-icu* translation files.

This happens because of $hasMessageFormatter = class_exists(\MessageFormatter::class) in FileDumper. MessageFormatter becomes available as soon as php-intl is installed.

We now have to update hundreds of translation keys, while we just want to use intl for the Twig number formatter. Please consider a fix.

@yceruto
Copy link
Member Author
yceruto commented Jan 20, 2020

@XWB this PR is the fix ;)

@nicolas-grekas
Copy link
Member

Thank you @yceruto.

nicolas-grekas added a commit 8000 that referenced this pull request Jan 21, 2020
@nicolas-grekas
bug #35351 Revert #34797 "Fixed translations file dumper behavior" and …
039feed 
…fix #34713 (yceruto)

This PR was squashed before being merged into the 4.4 branch.

Discussion
----------

Revert #34797 "Fixed translations file dumper behavior" and fix #34713

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35264
| License       | MIT
| Doc PR        | -

Revert #34797

See also #35328

It's very likely that the new way will be completely different from this one that is being reverted. That's why I'm reverting rather than fixing it.

Commits
-------

9ca8720 Fixed #34713 Move new messages to intl domain when possible
56e79fe Revert "Fixed translations file dumper behavior"
@nicolas-grekas nicolas-grekas merged commit 9ca8720 into symfony:4.4 Jan 21, 2020
@yceruto yceruto deleted the revert_34797 branch January 21, 2020 12:08
This was referenced Jan 21, 2020
Merged
Merged
@odolbeau odolbeau mentioned this pull request Jan 28, 2020
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
BC Break Bug Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@yceruto @ruudk @fabpot @XWB @nicolas-grekas @carsonbot
0