[Validator] Add ConstraintViolationBuilder methods: fromViolation(), setPath(), getViolation() #60582

rela589n · 2025-05-29T12:09:55Z

Q	A
Branch?	7.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	#58029 (comment)
License	MIT

This PR: (1) adds the ability to create constraint violation builder from an existing violation (static factory method: ConstraintViolationBuilder::fromViolation($violation)) so that it can be adjusted in the builder, in particular (2) setPath() method, and finally retrieve new violation with (3) getViolation() method.

…ult) This PR was merged into the 7.4 branch. Discussion ---------- Replace `get_class()` calls by `::class` | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #...  | License | MIT Replace `get_class()` by `::class` It was already done in past in symfony#47401 Commits ------- e0a602b Replace get_class() calls by ::class

nicolas-grekas

Please add a line to the changelog of the component also

nicolas-grekas · 2025-05-30T15:13:50Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilderInterface.php

     *
     * @return $this
     *
     * @see \Symfony\Contracts\Translation\TranslatorInterface::trans()
     */
-    public function setPlural(int $number): static;
+    public function setPlural(?int $number): static;


this is a BC break
looking at the decoration test case, this might not be needed - instead, don't call setPlural if getPlural returns null

you're absolutely right, thank you for pointing this out
for some reason I didn't think about decoration 🫤

I've changed the interface

What about ConstarintViolationBuilder itself?

nicolas-grekas · 2025-05-30T15:17:01Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

@@ -90,7 +120,7 @@ public function setInvalidValue(mixed $invalidValue): static
        return $this;
    }

-    public function setPlural(int $number): static
+    public function setPlural(?int $number): static


see my concern about this change in the interface

changed the interface

btw, from my point question arises: if at any point we'd like to extend interface, how is it handled?
is this change released on a new major version as bc break? is there any deprecation of "not using new type"?

nicolas-grekas · 2025-05-30T15:20:12Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

@@ -27,24 +28,52 @@
 class ConstraintViolationBuilder implements ConstraintViolationBuilderInterface
 {
    private string $propertyPath;
+    private ?string $message = null;


no need to make this nullable:

Suggested change

private ?string $message = null;

private string $translatedMessage;

is it the point that it will be eventually initialized?

right now this is only initialized as $this->message ??= $this->translateMessage()

any access to this property prior to that place will throw an exception

nicolas-grekas · 2025-05-30T15:20:24Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

        private ?Constraint $constraint,
-        private string|\Stringable $message,
+        private string|\Stringable $messageTemplate,


let's keep the previous name

I was motivated by using the same naming as in ConstraintViolation, as these properties are named there as $message and $messageTemplate.

Naming them as $message and $translatedMessage here would confuse the picture, since $message means different things. Though, if you'd like them to be named this way, I will make a change.

Frankly speaking I myself thought about adding $translatedMessage, and letting the original $message alone, because the rename might introduce a BC break, as changing parameter name from message to messageTemplate would break the invocation code if it passes it as a named parameter.

Though, I've checked BC promise, and it says:

[10] Parameter names are only covered by the compatibility promise for constructors of Attribute classes. Using PHP named arguments might break your code when upgrading to newer Symfony versions.

[11] Only optional argument(s) of a constructor at last position may be added.

So, keeping it unified looks better to me, but in any case, feel free to ask what you will

…nt (crydotsnake) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Dotenv] improve documentation for dotenv component | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Improves the documentation for the Symfony dotenv component :) Commits ------- adfc7e9 [Dotenv] improve documentation for dotenv component

…, `DateType` and `TimeType` (wkania) This PR was merged into the 7.4 branch. Discussion ---------- [Form] Add `input=date_point` to `DateTimeType`, `DateType` and `TimeType` | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | | License | MIT Based on [datetime_immutable](https://symfony.com/blog/new-in-symfony-4-1-added-support-for-immutable-dates-in-forms). After [DatePointType](symfony#59900) and [DatePointDateType](symfony#60237), it would be great to use Forms without needing to transform values into the DatePoint type manually. ``` use Symfony\Component\Form\Extension\Core\Type\DateType; use Symfony\Component\Form\Extension\Core\Type\DateTimeType; use Symfony\Component\Form\Extension\Core\Type\TimeType; use Symfony\Component\Form\Extension\Core\Type\BirthdayType; $builder->add('from', DateType::class, [ 'input' => 'date_point', ]); $builder->add('from', DateTimeType::class, [ 'input' => 'date_point', ]); $builder->add('from', TimeType::class, [ 'input' => 'date_point', ]); $builder->add('from', BirthdayType::class, [ 'input' => 'date_point', ]); ``` Alternative: Make symfony/clock a hard requirement and refactor the existing DateTimeImmutableToDateTimeTransformer to return a DatePoint instead. This should not introduce any breaking changes. Commits ------- f1160d6 [Form] Add input=date_point to DateTimeType, DateType and TimeType

…ration on the schedule name

…rovider service registration on the schedule name (adrianrudnik) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Scheduler] Throw error on duplicate schedule provider service registration on the schedule name | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | See below | License | MIT The way the schedule providers work, either by tagging a service or using the [AsSchedule](https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Scheduler/Attribute/AsSchedule.php) attribute, can lead to a scenario where multiple [ScheduleProviders](https://github.com/symfony/symfony/blob/7.3/src/Symfony/Component/Scheduler/ScheduleProviderInterface.php) register for the same schedule name (e.g. `default`). The problem arises when the [CompilerPass](https://github.com/symfony/symfony/blob/29da4f53d4a5b6a48ae8b43330e764a86ad7a85b/src/Symfony/Component/Scheduler/DependencyInjection/AddScheduleMessengerPass.php#L49) simply overwrites the previously registered one without throwing a warning, a notice or anything else. The problem would be that a full ScheduleProvider would simply not register and therefore not run. I decided to use the InvalidArgumentException from DI, as I could not get a trigger_error on E_USER_WARNING to show anything in a `dev` environment, in case I miss a scenario where this would actually be a desirable use case. Not sure if the test is complete enough for this scenario. I tried to base it on others found in HttpKernel. Also not sure if this falls under "feature" or "bug fix", so I marked it as a feature above. Commits ------- 8548aac [Scheduler] Throw error on duplicate schedule provider service registration on the schedule name

…setPath(), getViolation()

…etPlural()

xabbuh

I don't see that these are valuable changes as they change the ConstraintViolationBuilder class in a way that makes it less obvious to use. Thus I am 👎 here.

xabbuh · 2025-05-31T11:54:27Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

    private ?int $plural = null;
    private ?string $code = null;
    private mixed $cause = null;

    public function __construct(
-        private ConstraintViolationList $violations,
+        private ?ConstraintViolationListInterface $violations,


Making this property nullable doesn't look good to me as you would now have to be aware of the inner state of the ConstraintViolationBuilder class to decide whether or not you can call addViolation().

@xabbuh , what do you propose instead?

to decide whether or not you can call

how often is it necessary to "decide" in the way you say it?

afaik, addViolation is only called in custom validators that create violation builder using execution context, and it itself provides ConstraintViolationList

So what is the point in "deciding"?

Take a look at this arbitrary example taken from the Symfony code base (from the ChoiceValidator):

$this->context->buildViolation($constraint->maxMessage) ->setParameter('{{ limit }}', $constraint->max) ->setPlural((int) $constraint->max) ->setCode(Choice::TOO_MANY_ERROR) ->addViolation();

You wouldn't know here if the ConstraintViolationBuilderInterface implementation returned by buildViolation() would throw or not.

xabbuh · 2025-05-31T11:56:07Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

+            ->setCause($violation->getCause());
+    }
+
+    public function setPath(string $path): static


having this method is probably confusing for users of this class given that we already have an atPath() method and by looking at the names it's not obvious how they differ.

In most programming languages, setter methods (which modify private variables) are conventionally named by starting with "set" followed by the variable name, with the first letter of the variable name capitalized. For example, if a variable is named firstName, the setter would be setFirstName()

xabbuh · 2025-05-31T11:57:37Z

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilderInterface.php

 * objects.
 *
 * Use the various methods on this interface to configure the built violation.
 * Finally, call {@link addViolation()} to add the violation to the current
 * execution context.
 *
 * @author Bernhard Schussek <bschussek@gmail.com>
+ *
+ * @method ConstraintViolationInterface getViolation()


Adding this method adds a complete new meaning to the interface. I don't feel that it's a good idea having the same interface serve two different purposes.

xabbuh · 2025-06-01T20:15:05Z

Re-reading the changes I am 👎 on merging these changes. I also doubt that the described use case is common that we need to provide a solution for it in core. You can easily ship a custom ConstraintViolationBuilderInterface implementation in your application/library if you see fit for it in you specific use case though.

Replace get_class() calls by ::class

e0a602b

carsonbot added Status: Needs Review Feature Validator labels May 29, 2025

carsonbot added this to the 7.4 milestone May 29, 2025

nicolas-grekas reviewed May 30, 2025

View reviewed changes

crydotsnake and others added 8 commits May 30, 2025 17:28

[Dotenv] improve documentation for dotenv component

adfc7e9

[Form] Add input=date_point to DateTimeType, DateType and TimeType

f1160d6

[Scheduler] Throw error on duplicate schedule provider service regist…

8548aac

…ration on the schedule name

[Validator] Add ConstraintViolationBuilder methods: fromViolation(), …

29055ef

…setPath(), getViolation()

[Validator] Fix BC break about ConstraintViolationBuilderInterface::s…

20f264b

…etPlural()

rela589n force-pushed the feat-constraint-violation-builder-from-violation branch from 4cf1543 to 20f264b Compare May 31, 2025 10:44

rela589n changed the title ~~[Validator] Add ConstraintViolationBuilderInterface methods: fromViolation(), setPath(), getViolation()~~ [Validator] Add ConstraintViolationBuilder methods: fromViolation(), setPath(), getViolation() May 31, 2025

xabbuh requested changes May 31, 2025

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 31, 2025

fabpot force-pushed the 7.4 branch from 65b7b8a to 1152edf Compare June 1, 2025 07:08

fabpot requested review from kbond, yceruto and chalasr as code owners June 1, 2025 07:08

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jun 1, 2025

	private ?string $message = null;
	private string $translatedMessage;

Uh oh!

[Validator] Add ConstraintViolationBuilder methods: fromViolation(), setPath(), getViolation() #60582

Are you sure you want to change the base?

[Validator] Add ConstraintViolationBuilder methods: fromViolation(), setPath(), getViolation() #60582

Conversation

rela589n commented May 29, 2025

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rela589n May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jun 1, 2025

Uh oh!

Uh oh!

rela589n May 31, 2025 •

edited

Loading