Skip to content

Added support for using the "{{ label }}" placeholder in constraint messages #35338

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 1 commit into from
Sep 1, 2020
Merged

Added support for using the "{{ label }}" placeholder in constraint messages #35338

merged 1 commit into from
Sep 1, 2020

Conversation

a-menshchikov
Copy link
Contributor

@a-menshchikov a-menshchikov commented Jan 14, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets #12238
License MIT
Doc PR
  • Add docs PR

@a-menshchikov a-menshchikov requested a review from xabbuh as a code owner January 14, 2020 17:39
@a-menshchikov a-menshchikov changed the title Added support for using the "{{ label }}" placeholder in constraint messages [WIP] Added support for using the "{{ label }}" placeholder in constraint messages Jan 15, 2020
@a-menshchikov
Copy link
Contributor Author

There is no support for label_format yet. I have no idea about how get %id% substitution value in ViolationMapper.

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 17, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 17, 2020 •
edited
Loading

I think this would need some examples - via tests maybe. Reviewing I'm wondering e.g. why doesn't the message go through translation? Why do we use str_replace instead of "label" as a translation variable? Should translating the label be done before or after humanize() by form? etc. All these may become more obvious with examples.

@a-menshchikov
Copy link
Contributor Author

a-menshchikov commented Jan 17, 2020 •
edited
Loading

Why doesn't the message go through translation?

Message into violation object already translated.

Why do we use str_replace instead of "label" as a translation variable?

Since the message has already been translated, it makes no sense to translate it one more time with {{ label }} parameter. IMO str_replace is quite enough. (But maybe I'm not taking something into account.)

Should translating the label be done before or after humanize() by form?

There I've repeated translation logic from form_div_layout.html.twig because we want to get the same label as it rendered on form.

@a-menshchikov
Copy link
Contributor Author

Maybe placeholder should be %label% for consistence with substitutions format in label_format widget option?

@a-menshchikov
Copy link
Contributor Author

@nicolas-grekas what can I do to bring the review closer?

@YaFou
Copy link
Contributor

YaFou commented Aug 21, 2020

Any news on this pull request?

@a-menshchikov a-menshchikov force-pushed the feature/12238 branch 2 times, most recently from 01b14fd to 47b92d4 Compare August 22, 2020 08:30
@a-menshchikov
Copy link
Contributor Author

@nicolas-grekas @xabbuh PR rebased on master and ready for review.

@a-menshchikov a-menshchikov force-pushed the feature/12238 branch 2 times, most recently from 32267a1 to ff9bc3f Compare August 26, 2020 13:57
@fabpot
Copy link
Member

fabpot commented Aug 27, 2020

@xabbuh Could you review this PR?

xabbuh
xabbuh reviewed Sep 1, 2020
View reviewed changes
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

Just one minor comment. Apart from that the PR looks very good. 👍

@a-menshchikov a-menshchikov requested a review from xabbuh September 1, 2020 07:37
@xabbuh xabbuh added the Form label Sep 1, 2020
@xabbuh xabbuh changed the title [WIP] Added support for using the "{{ label }}" placeholder in constraint messages Added support for using the "{{ label }}" placeholder in constraint messages Sep 1, 2020
@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

Thank you @a-menshchikov.

@fabpot fabpot mentioned this pull request Sep 1, 2020
Closed
@fabpot
Copy link
Member

fabpot commented Sep 1, 2020

@a-menshchikov Can you submit a PR for the docs?

@fabpot fabpot merged commit ccfc4ba into symfony:master Sep 1, 2020
@a-menshchikov a-menshchikov mentioned this pull request Sep 2, 2020
Merged
@a-menshchikov a-menshchikov deleted the feature/12238 branch September 2, 2020 22:25
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 3, 2020
@javiereguiluz
minor #14161 Added support for using the "{{ label }}" placeholder in…
68cd260 
… constraint messages (a-menshchikov)

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

Discussion
----------

Added support for using the "{{ label }}" placeholder in constraint messages

Docs for symfony/symfony#35338

Commits
-------

e298929 Added support for using the "{{ label }}" placeholder in constraint messages
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@mmarton mmarton mentioned this pull request Jan 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kocal Kocal Kocal left review comments

@fabpot fabpot fabpot approved these changes

@xabbuh xabbuh xabbuh approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Feature Form Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
7 participants
@a-menshchikov @nicolas-grekas @YaFou @fabpot @xabbuh @Kocal @carsonbot