[Validator] Add ConstraintViolationBuilder methods: fromViolation(), setPath(), getViolation() #60582
Conversation
| * | ||
| * @return $this | ||
| * | ||
| * @see \Symfony\Contracts\Translation\TranslatorInterface::trans() | ||
| */ | ||
| public function setPlural(int $number): static; | ||
| public function setPlural(?int $number): static; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
you're absolutely right, thank you for pointing this out
for some reason I didn't think about decoration 🫤
| @@ -90,7 +120,7 @@ public function setInvalidValue(mixed $invalidValue): static | |||
| return $this; | |||
| } | |||
|
|
|||
| public function setPlural(int $number): static | |||
| public function setPlural(?int $number): static | |||
There was a problem hiding this comment.
see my concern about this change in the interface
There was a problem hiding this comment.
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"?
| @@ -27,24 +28,52 @@ | |||
| class ConstraintViolationBuilder implements ConstraintViolationBuilderInterface | |||
| { | |||
| private string $propertyPath; | |||
| private ?string $message = null; | |||
There was a problem hiding this comment.
no need to make this nullable:
| private ?string $message = null; | |
| private string $translatedMessage; |
There was a problem hiding this comment.
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
| private ?Constraint $constraint, | ||
| private string|\Stringable $message, | ||
| private string|\Stringable $messageTemplate, |
There was a problem hiding this comment.
let's keep the previous name
There was a problem hiding this comment.
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
…setPath(), getViolation()
4cf1543 to
20f264b
Compare
| private ?int $plural = null; | ||
| private ?string $code = null; | ||
| private mixed $cause = null; | ||
|
|
||
| public function __construct( | ||
| private ConstraintViolationList $violations, | ||
| private ?ConstraintViolationListInterface $violations, |
There was a problem hiding this comment.
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().
| ->setCause($violation->getCause()); | ||
| } | ||
|
|
||
| public function setPath(string $path): static |
There was a problem hiding this comment.
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.
| * 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() |
There was a problem hiding this comment.
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.
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.