Skip to content

Commit 88b158d

Browse files
fabpotfabpot
committed
bug symfony#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 symfony#38435, with branch changed **Explanation of the issue** In Symfony 5.2, the `{{ label }}` placeholder can be used in constraint messages (-> introduced in commit 0d9f442) 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 ------- aee5571 [Form] fix ViolationMapper was always generating a localized label for each FormType
2 parents 19c550d + aee5571 commit 88b158d

File tree

2 files changed

+58
-35
lines changed

2 files changed

+58
-35
lines changed

‎src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php

Lines changed: 37 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -131,44 +131,46 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
131131

132132
// Only add the error if the form is synchronized
133133
if ($this->acceptsErrors($scope)) {
134-
$labelFormat = $scope->getConfig()->getOption('label_format');
135-
136-
if (null !== $labelFormat) {
137-
$label = str_replace(
138-
[
139-
'%name%',
140-
'%id%',
141-
],
142-
[
143-
$scope->getName(),
144-
(string) $scope->getPropertyPath(),
145-
],
146-
$labelFormat
147-
);
148-
} else {
149-
$label = $scope->getConfig()->getOption('label');
150-
}
151-
152-
if (null === $label && null !== $this->formRenderer) {
153-
$label = $this->formRenderer->humanize($scope->getName());
154-
} elseif (null === $label) {
155-
$label = $scope->getName();
156-
}
157-
158-
if (false !== $label && null !== $this->translator) {
159-
$label = $this->translator->trans(
160-
$label,
161-
$scope->getConfig()->getOption('label_translation_parameters', []),
162-
$scope->getConfig()->getOption('translation_domain')
163-
);
164-
}
165-
166134
$message = $violation->getMessage();
167135
$messageTemplate = $violation->getMessageTemplate();
168136

169-
if (false !== $label) {
170-
$message = str_replace('{{ label }}', $label, $message);
171-
$messageTemplate = str_replace('{{ label }}', $label, $messageTemplate);
137+
if (false !== strpos($message, '{{ label }}') || false !== strpos($messageTemplate, '{{ label }}')) {
138+
$labelFormat = $scope->getConfig()->getOption('label_format');
139+
140+
if (null !== $labelFormat) {
141+
$label = str_replace(
142+
[
143+
'%name%',
144+
'%id%',
145+
],
146+
[
147+
$scope->getName(),
148+
(string) $scope->getPropertyPath(),
149+
],
150+
$labelFormat
151+
);
152+
} else {
153+
$label = $scope->getConfig()->getOption('label');
154+
}
155+
156+
if (false !== $label) {
157+
if (null === $label && null !== $this->formRenderer) {
158+
$label = $this->formRenderer->humanize($scope->getName());
159+
} elseif (null === $label) {
160+
$label = $scope->getName();
161+
}
162+
163+
if (null !== $this->translator) {
164+
$label = $this->translator->trans(
165+
$label,
166+
$scope->getConfig()->getOption('label_translation_parameters', []),
167+
$scope->getConfig()->getOption('translation_domain')
168+
);
169+
}
170+
171+
$message = str_replace('{{ label }}', $label, $message);
172+
$messageTemplate = str_replace('{{ label }}', $label, $messageTemplate);
173+
}
172174
}
173175

174176
$scope->addError(new FormError(

‎src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1738,4 +1738,25 @@ public function testMessageWithLabelFormat2()
17381738
$this->assertSame('Message Translated 2nd Custom Label', $error->getMessage());
17391739
}
17401740
}
1741+
1742+
public function testTranslatorNotCalledWithoutLabel()
1743+
{
1744+
$renderer = $this->getMockBuilder(FormRenderer::class)
1745+
->setMethods(null)
1746+
->disableOriginalConstructor()
1747+
->getMock()
1748+
;
1749+
$translator = $this->getMockBuilder(TranslatorInterface::class)->getMock();
1750+
$translator->expects($this->never())->method('trans');
1751+
$this->mapper = new ViolationMapper($renderer, $translator);
1752+
1753+
$parent = $this->getForm('parent');
1754+
$child = $this->getForm('name', 'name');
1755+
$parent->add($child);
1756+
1757+
$parent->submit([]);
1758+
1759+
$violation = new ConstraintViolation('Message without label', null, [], null, 'data.name', null);
1760+
$this->mapper->mapViolation($violation, $parent);
1761+
}
17411762
}

0 commit comments

Comments
 (0)