[Validator] Integrated the Translator in the Validator component #6137

webmozart · 2012-11-27T22:33:55Z

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #5844, #6117
Todo: -
License of the code: MIT
Documentation PR: -

This PR allows to replace the default message substitution strategy in the validator (strtr()) by passing an implementation of Symfony\Component\Translation\TranslatorInterface. The motivation for this are both #5844 and the need to replace the translation strategy in Drupal's integration of the Validator.

In the stand-alone usage of the validator, both the translator and the default translation domain can now be passed to ValidatorBuilderInterface:

$validator = Validation::createValidatorBuilder()
    ->setTranslator(new MyTranslator())
    ->setTranslationDomain('validators')
    ->getValidator();

References:

stof · 2012-11-28T00:57:04Z

src/Symfony/Component/Validator/DefaultTranslator.php

+        $id,
+        array $parameters = array(),
+        $domain = null,
+        $locale = null


we are not using multiline signatures in the codebase

Tobion · 2012-11-28T08:53:25Z

no BC break? Looking at ValidatorBuilderInterface there is definitely one.

webmozart · 2012-11-28T08:55:01Z

ValidatorBuilderInterface is not part of the stable API. You are not supposed to implement this interface.

Tobion · 2012-11-28T09:01:07Z

We're not only documenting bc breaks for stable API, otherwise we could remove 90% of the upgrade file since few methods are tagged with API.
An interface that nobody should implement?

webmozart · 2012-11-28T09:30:02Z

The question is what to consider a BC break. Something will always break for someone. Should we consequently mark everything as BC break? I don't think so.

For example, since 2.1, you are supposed to use Validation::createValidator*() for creating a validator. Because of that, I won't consider changing the constructor signature of Validator a BC break anymore from 2.1 on.

The same for the unstable interfaces. These are currently meant to be used only, that is, type hint against them and call their methods. But we don't guarantee that we won't add methods to them.

Tobion · 2012-11-28T09:38:19Z

I agree that almost any change could be considered a BC break. So we probably need to better define what a BC break is and what not. Otherwise Symfony will stop evolving after 2.3 because from then on Fabien wanted to prevent BC breaks which is almost impossible in a strict definition of bc break.

fabpot · 2012-11-28T11:37:22Z

BC breaks should always be documented, and we guarantee BC only for things tagged with @api. I'm going to update the docs to make things clearer.

webmozart · 2012-11-28T13:09:57Z

@fabpot I documented these changes now in the CHANGELOG: af99ebb1206ac92889b7193ba1ecc12bf2617e85

Are we sure we want to document all BC breaks from now on, even in non-@api code? This could rather scare people looking at our changelogs (lots of BC BREAKS there).

fago · 2012-11-28T17:29:58Z

Unfortunately, it turns out the symfony translator interface does not mach the Drupal translation system as well as we initally thought, see http://drupal.org/node/1852106. Given that, this would integrating the validator component into Drupal even harder, because it introduces the dependency on the (unwanted) translation component. :(

stof · 2012-11-28T18:19:36Z

If this does not help Drupal anyway, maybe #5844 is a better way to manage translations for people using the validator outside forms ?
and the Drupal guys would simply follow a similar approach, but based on their own translator instead of the symfony one.

fago · 2012-11-28T18:50:12Z

Yeah. The only problem I see with the approach of #5844 is that after validation only the translated messages are available. We'd need to have access to the untranslated messages also.

fago · 2012-11-29T09:49:47Z

As our translation system handles translating pluralized messages differently, the current ExecutionContextInterface::addViolation() method poses a problem also. We need to pass on - both the single and plural - message, as the message gets chosen during translation, see http://api.drupal.org/api/drupal/core!includes!common.inc/function/format_plural/8
So maybe, we could allow adding an already created ConstraintViolation object also? Then, we could implement a "PluralConstraintViolation" class that takes both message templates.

webmozart · 2012-12-03T15:52:36Z

I updated this PR to support pluralized messages by default in the validator. This should solve the problem of the Drupal guys, because their implementation of TranslatorInterface::transChoice($id, $number, ...) can now simply split the $id by pipes (|) and pass the parts to their own format_plural($count, $singular, $plural, ...) function.

For us, it breaks BC because translation catalog sources had to be adapted.

fabpot · 2012-12-03T16:25:52Z

Most of the XLF files are broken (the end is missing now).

IIUC, we now have a hard dependency on the Translation component, which is something we wanted to avoid.

fabpot · 2012-12-03T16:27:56Z

Oops, clicked on the "comment" button too fast.

So, the dependency is hard (you need to install the dep) but light as we only rely on the translation interface from the component (when using the default translator). It looks acceptable to me, especially because we now use Composer to manage dependencies.

webmozart · 2012-12-03T16:54:10Z

@fabpot Thanks for the hint. Going to fix this.

webmozart · 2012-12-04T11:34:43Z

@fabpot Fixed.

fabpot · 2012-12-05T10:30:25Z

src/Symfony/Component/Validator/DefaultTranslator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function transChoice($id, $number, array $parameters = array(), $domain = null, $locale = null)


This implementation is totally wrong. The string format for a plural message can be more complex than just foo|bar, you can have ranges, several plurals, ...

That's also why we have what we have right now. If you want to use translation, you need the translation component.
.

That's also why we have what we have right now. If you want to use translation, you need the translation component.

This doesn't change. The Default translator does not support message catalogs or any of this. It supports a simple subset of the Translation component that lets the Validator work without that component, unless you need actual translations or more complicated plural messages (which we don't have in the Validator core).

So, if I get you correctly, we are reinventing a way to "fake" plural messages? The source message has a convention where we have 2 messages separated by a pipe? If that's the case, then it should be documented.

And for the record, you cannot say that we don't have complicated plural messages in the core validators, as any message needing plural is complicated in some languages (a message is never complex by itself, but the translation for some languages are complex).

you cannot say that we don't have complicated plural messages in the core validators, as any message needing plural is complicated in some languages

DefaultTranslator does not support translation :) Just interpolation, i.e. parsing of parameters into messages. It does not have to care about what translations look like.

So, if I get you correctly, we are reinventing a way to "fake" plural messages?

To make a long answer short, yes. People can use intervals in their constraint classes, but then these constraints are bound to the Translation component and not compatible for example with Drupal's translation mechanism.

webmozart · 2012-12-07T12:40:24Z

Is there anything missing for this PR to be merged?

fago · 2012-12-31T00:51:07Z

src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php

     */
-    public function __construct($queryBuilder, $manager = null, $class = null)
+    public function __construct(QueryBuilder $queryBuilder)


Looks like an unrelated change??

…ion and used the Translation component for that

…dator

…n the validator

…the validator

This was implemented in order to satisfy Drupal's requirements for a singular and a plural message whenever a message is passed to their implementation of transChoice().

…lidArgumentException

@fabpot

This PR was merged into the master branch. Commits ------- 586a16e [Validator] Changed DefaultTranslator::getLocale() to always return 'en' 58bfd60 [Validator] Improved the inline documentation of DefaultTranslator cd662cc [Validator] Added ExceptionInterface, BadMethodCallException and InvalidArgumentException e00e5ec [Validator] Fixed failing test cc0df0a [Validator] Changed validator to support pluralized messages by default 56d61eb [Form][Validator] Added BC breaks in unstable code to the CHANGELOG 1e34e91 [Form] Added upgrade instructions to the UPGRADE file b94a256 [DoctrineBridge] Adapted DoctrineBridge to translator integration in the validator c96a051 [FrameworkBundle] Adapted FrameworkBundle to translator integration in the validator 92a3b27 [TwigBridge] Adapted TwigBridge to translator integration in the validator e7eb5b0 [Form] Adapted Form component to translator integration in the validator 46f751c [Validator] Extracted message interpolation logic of ConstraintViolation and used the Translation component for that Discussion ---------- [Validator] Integrated the Translator in the Validator component Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: #5844, #6117 Todo: - License of the code: MIT Documentation PR: - This PR allows to replace the default message substitution strategy in the validator (`strtr()`) by passing an implementation of `Symfony\Component\Translation\TranslatorInterface`. The motivation for this are both #5844 and the need to replace the translation strategy in Drupal's integration of the Validator. In the stand-alone usage of the validator, both the translator and the default translation domain can now be passed to `ValidatorBuilderInterface`: ```php $validator = Validation::createValidatorBuilder() ->setTranslator(new MyTranslator()) ->setTranslationDomain('validators') ->getValidator(); ``` References: * #5844 * #6117 * #6129 * [Add a validation framework to Drupal 8](http://drupal.org/node/1845546) * [Add the symfony validator component to core despite Symfony potentially releasing BC-breaking updates after 2.3.](http://drupal.org/node/1849564) --------------------------------------------------------------------------- by Tobion at 2012-11-28T08:53:25Z no BC break? Looking at ValidatorBuilderInterface there is definitely one. --------------------------------------------------------------------------- by bschussek at 2012-11-28T08:55:01Z ValidatorBuilderInterface is not part of the stable API. You are not supposed to implement this interface. --------------------------------------------------------------------------- by Tobion at 2012-11-28T09:01:07Z We're not only documenting bc breaks for stable API, otherwise we could remove 90% of the upgrade file since few methods are tagged with API. An interface that nobody should implement? --------------------------------------------------------------------------- by bschussek at 2012-11-28T09:30:02Z The question is what to consider a BC break. Something will always break for someone. Should we consequently mark everything as BC break? I don't think so. For example, since 2.1, you are supposed to use `Validation::createValidator*()` for creating a validator. Because of that, I won't consider changing the constructor signature of `Validator` a BC break anymore from 2.1 on. The same for the unstable interfaces. These are currently meant to be used only, that is, type hint against them and call their methods. But we don't guarantee that we won't add methods to them. --------------------------------------------------------------------------- by Tobion at 2012-11-28T09:38:19Z I agree that almost any change could be considered a BC break. So we probably need to better define what a BC break is and what not. Otherwise Symfony will stop evolving after 2.3 because from then on Fabien wanted to prevent BC breaks which is almost impossible in a strict definition of bc break. --------------------------------------------------------------------------- by fabpot at 2012-11-28T11:37:22Z BC breaks should always be documented, and we guarantee BC only for things tagged with @api. I'm going to update the docs to make things clearer. --------------------------------------------------------------------------- by bschussek at 2012-11-28T13:09:57Z @fabpot I documented these changes now in the CHANGELOG: af99ebb1206ac92889b7193ba1ecc12bf2617e85 Are we sure we want to document *all* BC breaks from now on, even in non-@api code? This could rather scare people looking at our changelogs (lots of BC BREAKS there). --------------------------------------------------------------------------- by fago at 2012-11-28T17:29:58Z Unfortunately, it turns out the symfony translator interface does not mach the Drupal translation system as well as we initally thought, see http://drupal.org/node/1852106. Given that, this would integrating the validator component into Drupal even harder, because it introduces the dependency on the (unwanted) translation component. :( --------------------------------------------------------------------------- by stof at 2012-11-28T18:19:36Z If this does not help Drupal anyway, maybe #5844 is a better way to manage translations for people using the validator outside forms ? and the Drupal guys would simply follow a similar approach, but based on their own translator instead of the symfony one. --------------------------------------------------------------------------- by fago at 2012-11-28T18:50:12Z Yeah. The only problem I see with the approach of #5844 is that *after* validation only the translated messages are available. We'd need to have access to the untranslated messages also. --------------------------------------------------------------------------- by fago at 2012-11-29T09:49:47Z As our translation system handles translating pluralized messages differently, the current ExecutionContextInterface::addViolation() method poses a problem also. We need to pass on - both the single and plural - message, as the message gets chosen during translation, see http://api.drupal.org/api/drupal/core!includes!common.inc/function/format_plural/8 So maybe, we could allow adding an already created ConstraintViolation object also? Then, we could implement a "PluralConstraintViolation" class that takes both message templates. --------------------------------------------------------------------------- by bschussek at 2012-12-03T15:52:36Z I updated this PR to support pluralized messages by default in the validator. This should solve the problem of the Drupal guys, because their implementation of `TranslatorInterface::transChoice($id, $number, ...)` can now simply split the $id by pipes (`|`) and pass the parts to their own `format_plural($count, $singular, $plural, ...)` function. For us, it breaks BC because translation catalog sources had to be adapted. --------------------------------------------------------------------------- by fabpot at 2012-12-03T16:25:52Z Most of the XLF files are broken (the end is missing now). IIUC, we now have a hard dependency on the Translation component, which is something we wanted to avoid. --------------------------------------------------------------------------- by fabpot at 2012-12-03T16:27:56Z Oops, clicked on the "comment" button too fast. So, the dependency is hard (you need to install the dep) but light as we only rely on the translation interface from the component (when using the default translator). It looks acceptable to me, especially because we now use Composer to manage dependencies. --------------------------------------------------------------------------- by bschussek at 2012-12-03T16:54:10Z @fabpot Thanks for the hint. Going to fix this. --------------------------------------------------------------------------- by bschussek at 2012-12-04T11:34:43Z @fabpot Fixed. --------------------------------------------------------------------------- by bschussek at 2012-12-07T12:40:24Z Is there anything missing for this PR to be merged?

stof reviewed Nov 28, 2012
View reviewed changes

webmozart mentioned this pull request Nov 28, 2012

[Validator] Simplify adding of constraint violations #6138

Closed

webmozart mentioned this pull request Dec 5, 2012

Extracted the API of the Translation component into symfony/api #6189

Closed

fabpot reviewed Dec 5, 2012
View reviewed changes

fago reviewed Dec 31, 2012
View reviewed changes

webmozart added 6 commits January 8, 2013 14:43

[Validator] Extracted message interpolation logic of ConstraintViolat…

46f751c

…ion and used the Translation component for that

[Form] Adapted Form component to translator integration in the validator

e7eb5b0

[TwigBridge] Adapted TwigBridge to translator integration in the vali…

92a3b27

…dator

[FrameworkBundle] Adapted FrameworkBundle to translator integration i…

c96a051

…n the validator

[DoctrineBridge] Adapted DoctrineBridge to translator integration in …

b94a256

…the validator

[Form] Added upgrade instructions to the UPGRADE file

1e34e91

webmozart added 6 commits January 8, 2013 14:45

[Form][Validator] Added BC breaks in unstable code to the CHANGELOG

56d61eb

[Validator] Changed validator to support pluralized messages by default

cc0df0a

This was implemented in order to satisfy Drupal's requirements for a singular and a plural message whenever a message is passed to their implementation of transChoice().

[Validator] Fixed failing test

e00e5ec

[Validator] Added ExceptionInterface, BadMethodCallException and Inva…

cd662cc

…lidArgumentException

[Validator] Improved the inline documentation of DefaultTranslator

58bfd60

[Validator] Changed DefaultTranslator::getLocale() to always return 'en'

586a16e

fabpot merged commit 586a16e into symfony:master Jan 9, 2013

fabpot mentioned this pull request Jan 9, 2013

[Form] Add Translator Validator #5844

Closed

fabpot mentioned this pull request Sep 7, 2015

Decouple Validator from Translation component #15714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Integrated the Translator in the Validator component #6137

[Validator] Integrated the Translator in the Validator component #6137

webmozart commented Nov 27, 2012

stof Nov 28, 2012

webmozart Nov 28, 2012

Tobion commented Nov 28, 2012

webmozart commented Nov 28, 2012

Tobion commented Nov 28, 2012

webmozart commented Nov 28, 2012

Tobion commented Nov 28, 2012

fabpot commented Nov 28, 2012

webmozart commented Nov 28, 2012

fago commented Nov 28, 2012

stof commented Nov 28, 2012

fago commented Nov 28, 2012

fago commented Nov 29, 2012

webmozart commented Dec 3, 2012

fabpot commented Dec 3, 2012

fabpot commented Dec 3, 2012

webmozart commented Dec 3, 2012

webmozart commented Dec 4, 2012

fabpot Dec 5, 2012

webmozart Dec 5, 2012

fabpot Dec 5, 2012

fabpot Dec 5, 2012

webmozart Dec 5, 2012

webmozart commented Dec 7, 2012

fago Dec 31, 2012

[Validator] Integrated the Translator in the Validator component #6137

[Validator] Integrated the Translator in the Validator component #6137

Conversation

webmozart commented Nov 27, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Nov 28, 2012

webmozart commented Nov 28, 2012

Tobion commented Nov 28, 2012

webmozart commented Nov 28, 2012

Tobion commented Nov 28, 2012

fabpot commented Nov 28, 2012

webmozart commented Nov 28, 2012

fago commented Nov 28, 2012

stof commented Nov 28, 2012

fago commented Nov 28, 2012

fago commented Nov 29, 2012

webmozart commented Dec 3, 2012

fabpot commented Dec 3, 2012

fabpot commented Dec 3, 2012

webmozart commented Dec 3, 2012

webmozart commented Dec 4, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmozart commented Dec 7, 2012

Choose a reason for hiding this comment