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

[RFC][Validator] Allow to set the translation domain or disable translations on a per-constraint level#59770

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

Open
mpdude wants to merge4 commits intosymfony:7.4
base:7.4
Choose a base branch
Loading
frommpdude:set-translation-domain-in-constraint

Conversation

mpdude
Copy link
Contributor

@mpdudempdude commentedFeb 13, 2025
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
Issues
LicenseMIT

This PR builds upon the work in#48852 and makes it possible to specify the translation domain for validation constraints on a per-constraint level. This can happen either when using the constraint as an attribute, or when creating constraint instances e. g. when building a form.

$formBuilder            ->add('some_field', TextType::class, ['label' =>'Something','constraints' => [newNotBlank(['message' =>'standard.notblank.message','translation_domain' =>'my_special_domain'])],            ])            ->add('other_field', TextType::class, ['label' =>'Elsething','constraints' => [newNotBlank(['message' =>'Use this text as-is, do not attempt to translate','translation_domain' =>false])],            ]);

The setting works similar to thetranslation_domain option for form types. Setting it tofalse disables translations (for that constraint) altogether.

TODO

  •  Add documentation PR
  • Apply enhancement to all constraints once we agree on the basics; for now, it's justNotBlank
  • Add tests
  • Update src/Symfony/Component/Translation/Extractor/Visitor/ConstraintVisitor.php

@carsonbotcarsonbot added this to the7.3 milestoneFeb 13, 2025
@mpdudempdude changed the titleAllow to set the translation domain or disable translations on a per-constraint level[RFC][Validator] Allow to set the translation domain or disable translations on a per-constraint levelFeb 13, 2025
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Is there really a use case for that?

@mpdude
Copy link
ContributorAuthor

We have monitoring in place that reports certain errors from production systems, and the translator warnings are among those.

Sometimes, you don't want to go through the hassle of maintaining translation catalogues and put the plain text into the code directly (single-lang apps).

In that case, it would be nice if translations could be disabled.

@nicolas-grekas
Copy link
Member

How do you deal with that at the moment?
I feel like the proposed strategy has too much impact for achieving your use case.
Any other idea that'd be less invasive?

@mpdude
Copy link
ContributorAuthor

How do you deal with that at the moment?
I feel like the proposed strategy has too much impact for achieving your use case.
Any other idea that'd be less invasive?

Not really, yet. Right now, in such cases, my logs are full with warnings fromLoggingTranslator. When I feel that these log messages are getting too much (like dozens of megabytes of log volume per day), I'd change the code to use "real" message-IDs instead, and move the text to avalidators message catalogue.

I care mostly about validation constraint messages added when form types are being built. But since the form types come from different bundles and some of them provide translations whereas others do not, I cannot use the solution from#50797 to disable all translations altogether.

#48852 put the logic in place to change or disable the translation domain for individual (project-specific) validators, but does not make this feature available for the built-in constraints.

To me, it felt natural if the sametranslation_domain parameter that can be used for the form types could be used on for the constraints as well (consider my initial example).

@mpdude
Copy link
ContributorAuthor

Any other idea that'd be less invasive?

... or do you mean "achieves the same without a repeating code pattern in all constraints and validators"?

Full ack, I do not fancy putting the same change into lots of classes over and over again.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedFeb 24, 2025
edited
Loading

Thanks for the hints. I've no better idea so I feel like this might be good to me.
I'm just wondering: would it make sense to allow defining the translation domain by contraint's FQCN in config/packages/validation.yaml? This way one could say that all#[SomeConstraint] messages should use some domain?

@nicolas-grekas
Copy link
Member

... or do you mean "achieves the same without a repeating code pattern in all constraints and validators"?

That's what I meant yes.
Maybe my config idea could be enough?

validation:translation_domains:SomeConstraint:some-domain

@mpdude
Copy link
ContributorAuthor

Not sure – assume you have two bundles that build aFormType each. And in thoseFormTypes, they both use theNotBlank constraint with message-IDs.

To me, it would make sense if both bundles could keep their respective validation/constraint message in their "own" translation domain, as suggested by thebest practice.

That would not be possible with a config setting as suggested, or am I missing something?

Would you be fine with a solution that involves something with traits to plug-in thetranslation_domain into all the constraints?

Still, that does not allow to pass it as a constructor argument. 🤔

Also, since the validator class calls$this->context->buildViolation() to create theConstraintViolationBuilder, but never passes it somewhere else, and ends with callingaddViolation() without ever passing theConstraint instance, I am afraid there is nothing we can do with the current code design to centralizetranslation_domain handling.

Maybe GitHub Copilot can pull it off for us 😹?

@mpdude
Copy link
ContributorAuthor

@ro0NL You made a comment aboutTranslatableMessage support; did you withdraw that for good reasons? Probably makes the change even bigger.

@welcoMattic
Copy link
Member

Adding this feature will break the PhpAstExtractor as it is. Actually, to extract translation keys from Constraints, it leverageshttps://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Translation/Extractor/Visitor/ConstraintVisitor.php, which is looking keys and extract them invalidators domain by default.

Introducingtranslation_domain option here means to update the ConstraintVisitor to handle thetranslation_domain option value to extract keys in the correct domain (orvalidators by default as it is now).

IIUC, the main reason of this feature is:

#48852 put the logic in place to change or disable the translation domain for individual (project-specific) validators, but does not make this feature available for the built-in constraints.

But I'm wondering why do you need to change the translation domain of built-in constraints? Did I miss something?

@mpdude
Copy link
ContributorAuthor

@welcoMattic thank you for the heads up! If I get you right, I'll need to update thesrc/Symfony/Component/Translation/Extractor/Visitor/ConstraintVisitor.php as well.

It's not about changing the translation domain for built-in messages; it's great that Symfony ships with translations for those!

My use case is about using the constraints likeNotBlank in custom form types, potentially shipped in bundles, with custom validation error messages. For those, I'd like to be able to control the message catalogue. See my initial comment for the example.

Does that make sense?

Still, I have no better idea yet how to approach this apart from making the same kind of change in all constraints. Anyone else?

@mpdudempdudeforce-pushed theset-translation-domain-in-constraint branch from5986ddf toc8f94d4CompareMarch 11, 2025 17:59
@mpdudempdudeforce-pushed theset-translation-domain-in-constraint branch fromc8f94d4 to4a4905dCompareMarch 11, 2025 18:08
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 11, 2025
edited
Loading

So, instead of spreading this everywhere, maybe we could just update the NotBlank one?
And maybe a few more constraints where changing the default message makes sense?

@mpdude
Copy link
ContributorAuthor

mpdude commentedMar 11, 2025
edited
Loading

I've fixed tests and added an example of what a test case might look like. 

The complete change ain't gonna be fun – so maybe@nicolas-grekas can give me a 👍 that you would indeed merge it if I go the whole way of adding it? 😉

Sorry, we both posted at the same time.

@mpdude
Copy link
ContributorAuthor

So you'd prefer changing only a few constraints over consistency of usage?

@nicolas-grekas
Copy link
Member

I do yes. Consistency is a superficial thing compared to code complexity.

@nicolas-grekas
Copy link
Member

Up to finish this PR@mpdude ?

@mpdude
Copy link
ContributorAuthor

I checked out the codebase where I'd benefit from this change and was daunted due to the number of validators actually used 🙈.

Let me think through again whether I really want to go on this journey.

@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

5 participants
@mpdude@nicolas-grekas@welcoMattic@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp