bug #38477 [Form] fix ViolationMapper was always generating a localized label for each FormType (romaricdrigon)
This PR was merged into the 5.x branch. Discussion ---------- [Form] fix ViolationMapper was always generating a localized label for each FormType | Q | A | ------------- | --- | Branch? | 5.x (fix new behavior from 5.2) | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | / | License | MIT | Doc PR | / Follow-up of #38435, with branch changed **Explanation of the issue** In Symfony 5.2, the `{{ label }}` placeholder can be used in constraint messages (-> introduced in commit0d9f44235c
) However, the way it was coded is introducing a small side effect: now, every time there is validation error, `ViolationMapper` will ask the Form Type its `label`, and if not `false`, it will try to translate it. **Why it is important/why it causes a BC break** Since by default `AbstractType` does not have any `label`, it also introduces a minor BC break. I will explain it with an example: in a project I work on, we check we don't have any missing translation. Sometimes we have violation errors bound to form ; then current code will get Form `label`, which in `null` in form type classes (which is quite usual I believe), so it will generate one, and pass that one to translator. And we see a lot on erroneous missing translations. **Proposed fix** This fix moves all this logic into a `if`, so `ViolationMapper` call the translator component only if `{{ label }}` placeholder is used in constraint error message. On top of fixing BC, it has the benefit of lowering the performance cost for every violation when the feature is not used. I added a test, as I believe the behavior should be guaranteed from now on. Commits -------aee5571a71
[Form] fix ViolationMapper was always generating a localized label for each FormType
This commit is contained in:
commit
88b158d989
@ -131,6 +131,10 @@ class ViolationMapper implements ViolationMapperInterface
|
|||||||
|
|
||||||
// Only add the error if the form is synchronized
|
// Only add the error if the form is synchronized
|
||||||
if ($this->acceptsErrors($scope)) {
|
if ($this->acceptsErrors($scope)) {
|
||||||
|
$message = $violation->getMessage();
|
||||||
|
$messageTemplate = $violation->getMessageTemplate();
|
||||||
|
|
||||||
|
if (false !== strpos($message, '{{ label }}') || false !== strpos($messageTemplate, '{{ label }}')) {
|
||||||
$labelFormat = $scope->getConfig()->getOption('label_format');
|
$labelFormat = $scope->getConfig()->getOption('label_format');
|
||||||
|
|
||||||
if (null !== $labelFormat) {
|
if (null !== $labelFormat) {
|
||||||
@ -149,13 +153,14 @@ class ViolationMapper implements ViolationMapperInterface
|
|||||||
$label = $scope->getConfig()->getOption('label');
|
$label = $scope->getConfig()->getOption('label');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (false !== $label) {
|
||||||
if (null === $label && null !== $this->formRenderer) {
|
if (null === $label && null !== $this->formRenderer) {
|
||||||
$label = $this->formRenderer->humanize($scope->getName());
|
$label = $this->formRenderer->humanize($scope->getName());
|
||||||
} elseif (null === $label) {
|
} elseif (null === $label) {
|
||||||
$label = $scope->getName();
|
$label = $scope->getName();
|
||||||
}
|
}
|
||||||
|
|
||||||
if (false !== $label && null !== $this->translator) {
|
if (null !== $this->translator) {
|
||||||
$label = $this->translator->trans(
|
$label = $this->translator->trans(
|
||||||
$label,
|
$label,
|
||||||
$scope->getConfig()->getOption('label_translation_parameters', []),
|
$scope->getConfig()->getOption('label_translation_parameters', []),
|
||||||
@ -163,13 +168,10 @@ class ViolationMapper implements ViolationMapperInterface
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
$message = $violation->getMessage();
|
|
||||||
$messageTemplate = $violation->getMessageTemplate();
|
|
||||||
|
|
||||||
if (false !== $label) {
|
|
||||||
$message = str_replace('{{ label }}', $label, $message);
|
$message = str_replace('{{ label }}', $label, $message);
|
||||||
$messageTemplate = str_replace('{{ label }}', $label, $messageTemplate);
|
$messageTemplate = str_replace('{{ label }}', $label, $messageTemplate);
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
$scope->addError(new FormError(
|
$scope->addError(new FormError(
|
||||||
$message,
|
$message,
|
||||||
|
@ -1738,4 +1738,25 @@ class ViolationMapperTest extends TestCase
|
|||||||
$this->assertSame('Message Translated 2nd Custom Label', $error->getMessage());
|
$this->assertSame('Message Translated 2nd Custom Label', $error->getMessage());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
public function testTranslatorNotCalledWithoutLabel()
|
||||||
|
{
|
||||||
|
$renderer = $this->getMockBuilder(FormRenderer::class)
|
||||||
|
->setMethods(null)
|
||||||
|
->disableOriginalConstructor()
|
||||||
|
->getMock()
|
||||||
|
;
|
||||||
|
$translator = $this->getMockBuilder(TranslatorInterface::class)->getMock();
|
||||||
|
$translator->expects($this->never())->method('trans');
|
||||||
|
$this->mapper = new ViolationMapper($renderer, $translator);
|
||||||
|
|
||||||
|
$parent = $this->getForm('parent');
|
||||||
|
$child = $this->getForm('name', 'name');
|
||||||
|
$parent->add($child);
|
||||||
|
|
||||||
|
$parent->submit([]);
|
||||||
|
|
||||||
|
$violation = new ConstraintViolation('Message without label', null, [], null, 'data.name', null);
|
||||||
|
$this->mapper->mapViolation($violation, $parent);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user