Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Translation] trans, add support of domain in $id message#37769

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
wtorsi wants to merge5 commits intosymfony:masterfromwtorsi:ISSUE_37743

Conversation

@wtorsi
Copy link

QA
Branch?master for features
Bug fix?no
New feature?yes
Deprecations?no
Tickets#37743
LicenseMIT
Doc PR

Add support of domain in id message while translating

@wtorsiwtorsi changed the title[ISSUE_37743] trans, add support of domain in $id message[Translation] trans, add support of domain in $id messageAug 8, 2020
@xabbuhxabbuh added this to thenext milestoneAug 8, 2020
Copy link
Contributor

@YaFouYaFou left a comment

Choose a reason for hiding this comment

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

👍 Can you add a test for this feature?

@wtorsi
Copy link
Author

@YaFou sure, done

Copy link
Contributor

@YaFouYaFou left a comment

Choose a reason for hiding this comment

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

Good job@wtorsi! And great feature!

@xabbuh
Copy link
Member

This looks like a BC break to me as it will break most existing translations that contain the@ character. The only viable BC solution that I see would be to create a decorator which contains this logic and that would be an opt-in feature.

jvasseur reacted with thumbs up emoji

@YaFou
Copy link
Contributor

This looks like a BC break to me as it will break most existing translations that contain the@ character. The only viable BC solution that I see would be to create a decorator which contains this logic and that would be an opt-in feature.

Good point. A solution may be to add an argument to theTranslator class which controls this option. Also this feature can be applied in version 6 generally which will be a breaking change.

@wtorsi
Copy link
Author

Just found thatDataCollectorTranslator will be broken after this pull request.
I think It's a good idea, but it needs a little bit more work and considerations

@ro0NL
Copy link
Contributor

Shouldnt we prefer the approach in#37670?

jvasseur reacted with thumbs up emoji

@wtorsi
Copy link
Author

I prefer the approach of translatable objects in#37670 , but my idea is to encode domain in message ID itself, because in my opinion it is easier to store such ids in database

@fabpot
Copy link
Member

I'm against this change. First, it's a BC break, then, it looks a bit hacky, it's also yet another way to do something we can already do, and last, it does not work well when you are not using translation strings:$this->translator->trans('This is a message to translate@messages', $params).

Thanks for proposing.

@fabpotfabpot closed thisAug 11, 2020
@nicolas-grekasnicolas-grekas modified the milestones:next,5.2Oct 5, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@YaFouYaFouYaFou approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

7 participants

@wtorsi@xabbuh@YaFou@ro0NL@fabpot@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp