Refactor validation of PersonIdentifier

src/Bundle/ChillPersonBundle/PersonIdentifier/Identifier/StringIdentifier.php

@@ -13,10 +13,10 @@ namespace Chill\PersonBundle\PersonIdentifier\Identifier;
 
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifierDefinition;
+use Chill\PersonBundle\PersonIdentifier\IdentifierViolationDTO;
 use Chill\PersonBundle\PersonIdentifier\PersonIdentifierEngineInterface;
 use Symfony\Component\Form\Extension\Core\Type\TextType;
 use Symfony\Component\Form\FormBuilderInterface;
-use Symfony\Component\Validator\Context\ExecutionContextInterface;
 
 final readonly class StringIdentifier implements PersonIdentifierEngineInterface
 {
@@ -50,20 +50,24 @@ final readonly class StringIdentifier implements PersonIdentifierEngineInterface
         return '' === trim($identifier->getValue()['content'] ?? '');
     }
 
-    public function validate(ExecutionContextInterface $context, PersonIdentifier $identifier, PersonIdentifierDefinition $definition): void
+    public function validate(PersonIdentifier $identifier, PersonIdentifierDefinition $definition): array
     {
         $config = $definition->getData();
         $content = (string) ($identifier->getValue()['content'] ?? '');
+        $violations = [];
 
         if (($config[self::ONLY_NUMBERS] ?? false) && !preg_match('/^[0-9]+$/', $content)) {
-            $context->buildViolation('person_identifier.only_number')
-                ->addViolation();
+            $violations[] = new IdentifierViolationDTO('person_identifier.only_number', '2a3352c0-a2b9-11f0-a767-b7a3f80e52f1');
         }
 
         if (null !== ($config[self::FIXED_LENGTH] ?? null) && strlen($content) !== $config[self::FIXED_LENGTH]) {
-            $context->buildViolation('person_identifier.fixed_length')
-                ->setParameter('limit', (string) $config[self::FIXED_LENGTH])
-                ->addViolation();
+            $violations[] = new IdentifierViolationDTO(
+                'person_identifier.fixed_length',
+                '2b02a8fe-a2b9-11f0-bfe5-033300972783',
+                ['limit' => (string) $config[self::FIXED_LENGTH]]
+            );
         }
+
+        return $violations;
     }
 }

src/Bundle/ChillPersonBundle/PersonIdentifier/IdentifierViolationDTO.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * Chill is a software for social workers
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+namespace Chill\PersonBundle\PersonIdentifier;
+
+/**
+ * Data Transfer Object to create a ConstraintViolationListInterface.
+ */
+class IdentifierViolationDTO
+{
+    public function __construct(
+        public string $message,
+        /**
+         * @var string an UUID
+         */
+        public string $code,
+        /**
+         * @var array<string, string>
+         */
+        public array $parameters = [],
+        public string $messageDomain = 'validators',
+    ) {}
+}

src/Bundle/ChillPersonBundle/PersonIdentifier/PersonIdentifierEngineInterface.php

@@ -14,7 +14,6 @@ namespace Chill\PersonBundle\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifierDefinition;
 use Symfony\Component\Form\FormBuilderInterface;
-use Symfony\Component\Validator\Context\ExecutionContextInterface;
 
 interface PersonIdentifierEngineInterface
 {
@@ -34,5 +33,10 @@ interface PersonIdentifierEngineInterface
      */
     public function isEmpty(PersonIdentifier $identifier): bool;
 
-    public function validate(ExecutionContextInterface $context, PersonIdentifier $identifier, PersonIdentifierDefinition $definition): void;
+    /**
+     * Return a list of @link{IdentifierViolationDTO} to generatie violation errors.
+     *
+     * @return list<IdentifierViolationDTO>
+     */
+    public function validate(PersonIdentifier $identifier, PersonIdentifierDefinition $definition): array;
 }

src/Bundle/ChillPersonBundle/PersonIdentifier/PersonIdentifierWorker.php

@@ -14,7 +14,6 @@ namespace Chill\PersonBundle\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifierDefinition;
 use Symfony\Component\Form\FormBuilderInterface;
-use Symfony\Component\Validator\Context\ExecutionContextInterface;
 
 readonly class PersonIdentifierWorker
 {
@@ -56,8 +55,11 @@ readonly class PersonIdentifierWorker
         return $this->identifierEngine->isEmpty($identifier);
     }
 
-    public function validate(ExecutionContextInterface $context, PersonIdentifier $identifier, PersonIdentifierDefinition $definition): void
+    /**
+     * @return list<IdentifierViolationDTO>
+     */
+    public function validate(PersonIdentifier $identifier, PersonIdentifierDefinition $definition): array
     {
-        $this->identifierEngine->validate($context, $identifier, $definition);
+        return $this->identifierEngine->validate($identifier, $definition);
     }
 }

src/Bundle/ChillPersonBundle/PersonIdentifier/Validator/ValidIdentifierConstraintValidator.php

@@ -34,6 +34,14 @@ final class ValidIdentifierConstraintValidator extends ConstraintValidator
 
         $worker = $this->personIdentifierManager->buildWorkerByPersonIdentifierDefinition($value->getDefinition());
 
-        $worker->validate($this->context, $value, $value->getDefinition());
+        $violations = $worker->validate($value, $value->getDefinition());
+
+        foreach ($violations as $violation) {
+            $this->context->buildViolation($violation->message)
+                ->setParameters($violation->parameters)
+                ->setParameter('{{ code }}', $violation->code)
+                ->setParameter('definition_id', (string) $value->getDefinition()->getId())
+                ->addViolation();
+        }
     }
 }

src/Bundle/ChillPersonBundle/Tests/PersonIdentifier/Identifier/StringIdentifierValidationTest.php

@@ -14,10 +14,7 @@ namespace Chill\PersonBundle\Tests\PersonIdentifier\Identifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifierDefinition;
 use Chill\PersonBundle\PersonIdentifier\Identifier\StringIdentifier;
-use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
-use Symfony\Component\Validator\Context\ExecutionContextInterface;
-use Symfony\Component\Validator\Violation\ConstraintViolationBuilderInterface;
 
 /**
  * @internal
@@ -28,10 +25,15 @@ class StringIdentifierValidationTest extends TestCase
 {
     private function makeDefinition(array $data = []): PersonIdentifierDefinition
     {
-        return new PersonIdentifierDefinition(label: ['en' => 'String'], engine: StringIdentifier::getName(), data: $data);
+        $definition = new PersonIdentifierDefinition(label: ['en' => 'Test'], engine: StringIdentifier::NAME);
+        if ([] !== $data) {
+            $definition->setData($data);
+        }
+
+        return $definition;
     }
 
-    private function makeIdentifier(PersonIdentifierDefinition $definition, string $content): PersonIdentifier
+    private function makeIdentifier(PersonIdentifierDefinition $definition, ?string $content): PersonIdentifier
     {
         $identifier = new PersonIdentifier($definition);
         $identifier->setValue(['content' => $content]);
@@ -39,90 +41,71 @@ class StringIdentifierValidationTest extends TestCase
         return $identifier;
     }
 
-    /**
-     * Helper creating a context mock expecting zero violations.
-     */
-    private function expectNoViolation(): ExecutionContextInterface
+    public function testValidateWithoutOptionsHasNoViolations(): void
     {
-        $context = $this->createMock(ExecutionContextInterface::class);
-        $context->expects(self::never())->method('buildViolation');
+        $definition = $this->makeDefinition();
+        $identifier = $this->makeIdentifier($definition, 'AB-123');
 
-        return $context;
-    }
-
-    /**
-     * Helper creating a context mock expecting a single violation with a given message and optional parameters chain.
-     */
-    private function expectSingleViolation(string $message, array $parameters = []): ExecutionContextInterface
-    {
-        /** @var ConstraintViolationBuilderInterface&MockObject $builder */
-        $builder = $this->createMock(ConstraintViolationBuilderInterface::class);
-
-        // If parameters are provided, ensure setParameter is called with each, in any order, and returns builder for chaining
-        if ($parameters) {
-            foreach ($parameters as $name => $value) {
-                $builder->expects(self::atLeastOnce())
-                    ->method('setParameter')
-                    ->with($name, $value)
-                    ->willReturn($builder);
-            }
-        } else {
-            $builder->expects(self::any())->method('setParameter')->willReturn($builder);
-        }
-
-        $builder->expects(self::once())->method('addViolation');
-
-        $context = $this->createMock(ExecutionContextInterface::class);
-        $context->expects(self::once())
-            ->method('buildViolation')
-            ->with($message)
-            ->willReturn($builder);
-
-        return $context;
-    }
-
-    public function testNoOptionsAcceptsAnyString(): void
-    {
         $engine = new StringIdentifier();
-        $definition = $this->makeDefinition([]);
-        $identifier = $this->makeIdentifier($definition, 'abc123');
+        $violations = $engine->validate($identifier, $definition);
 
-        $engine->validate($this->expectNoViolation(), $identifier, $definition);
+        self::assertIsArray($violations);
+        self::assertCount(0, $violations);
     }
 
-    public function testOnlyNumbersValid(): void
+    public function testValidateOnlyNumbersOption(): void
     {
-        $engine = new StringIdentifier();
         $definition = $this->makeDefinition(['only_numbers' => true]);
-        $identifier = $this->makeIdentifier($definition, '123456');
+        $engine = new StringIdentifier();
 
-        $engine->validate($this->expectNoViolation(), $identifier, $definition);
+        // valid numeric content
+        $identifierOk = $this->makeIdentifier($definition, '123456');
+        $violationsOk = $engine->validate($identifierOk, $definition);
+        self::assertCount(0, $violationsOk);
+
+        // invalid alphanumeric content
+        $identifierBad = $this->makeIdentifier($definition, '12AB');
+        $violationsBad = $engine->validate($identifierBad, $definition);
+        self::assertCount(1, $violationsBad);
+        self::assertSame('person_identifier.only_number', $violationsBad[0]->message);
+        self::assertSame('2a3352c0-a2b9-11f0-a767-b7a3f80e52f1', $violationsBad[0]->code);
     }
 
-    public function testOnlyNumbersInvalid(): void
+    public function testValidateFixedLengthOption(): void
     {
-        $engine = new StringIdentifier();
-        $definition = $this->makeDefinition(['only_numbers' => true]);
-        $identifier = $this->makeIdentifier($definition, '12AB');
-
-        $engine->validate($this->expectSingleViolation('person_identifier.only_number'), $identifier, $definition);
-    }
-
-    public function testFixedLengthValid(): void
-    {
-        $engine = new StringIdentifier();
         $definition = $this->makeDefinition(['fixed_length' => 5]);
-        $identifier = $this->makeIdentifier($definition, '12345');
+        $engine = new StringIdentifier();
 
-        $engine->validate($this->expectNoViolation(), $identifier, $definition);
+        // valid exact length
+        $identifierOk = $this->makeIdentifier($definition, 'ABCDE');
+        $violationsOk = $engine->validate($identifierOk, $definition);
+        self::assertCount(0, $violationsOk);
+
+        // invalid length (too short)
+        $identifierBad = $this->makeIdentifier($definition, 'AB');
+        $violationsBad = $engine->validate($identifierBad, $definition);
+        self::assertCount(1, $violationsBad);
+        self::assertSame('person_identifier.fixed_length', $violationsBad[0]->message);
+        self::assertSame('2b02a8fe-a2b9-11f0-bfe5-033300972783', $violationsBad[0]->code);
+        self::assertSame(['limit' => '5'], $violationsBad[0]->parameters);
     }
 
-    public function testFixedLengthInvalid(): void
+    public function testValidateOnlyNumbersAndFixedLengthTogether(): void
     {
+        $definition = $this->makeDefinition(['only_numbers' => true, 'fixed_length' => 4]);
         $engine = new StringIdentifier();
-        $definition = $this->makeDefinition(['fixed_length' => 5]);
-        $identifier = $this->makeIdentifier($definition, '1234');
 
-        $engine->validate($this->expectSingleViolation('person_identifier.fixed_length', ['limit' => 5]), $identifier, $definition);
+        // valid: numeric and correct length
+        $identifierOk = $this->makeIdentifier($definition, '1234');
+        $violationsOk = $engine->validate($identifierOk, $definition);
+        self::assertCount(0, $violationsOk);
+
+        // invalid: non-numeric and wrong length -> two violations expected
+        $identifierBad = $this->makeIdentifier($definition, 'AB');
+        $violationsBad = $engine->validate($identifierBad, $definition);
+        self::assertCount(2, $violationsBad);
+        // Order is defined by implementation: numbers check first, then length
+        self::assertSame('person_identifier.only_number', $violationsBad[0]->message);
+        self::assertSame('person_identifier.fixed_length', $violationsBad[1]->message);
     }
 }

src/Bundle/ChillPersonBundle/Tests/PersonIdentifier/Validator/ValidIdentifierConstraintValidatorTest.php

@@ -13,12 +13,15 @@ namespace Chill\PersonBundle\Tests\PersonIdentifier\Validator;
 
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifier;
 use Chill\PersonBundle\Entity\Identifier\PersonIdentifierDefinition;
+use Chill\PersonBundle\PersonIdentifier\IdentifierViolationDTO;
+use Chill\PersonBundle\PersonIdentifier\PersonIdentifierEngineInterface;
 use Chill\PersonBundle\PersonIdentifier\PersonIdentifierManagerInterface;
 use Chill\PersonBundle\PersonIdentifier\PersonIdentifierWorker;
 use Chill\PersonBundle\PersonIdentifier\Validator\ValidIdentifierConstraint;
 use Chill\PersonBundle\PersonIdentifier\Validator\ValidIdentifierConstraintValidator;
 use PHPUnit\Framework\Attributes\CoversClass;
 use Prophecy\PhpUnit\ProphecyTrait;
+use Prophecy\Prophecy\ObjectProphecy;
 use Symfony\Component\Validator\Test\ConstraintValidatorTestCase;
 
 /**
@@ -29,51 +32,79 @@ final class ValidIdentifierConstraintValidatorTest extends ConstraintValidatorTe
 {
     use ProphecyTrait;
 
-    private object $manager;
-    private PersonIdentifierManagerInterface|\Prophecy\Prophecy\ObjectProphecy $managerProphecy;
-
-    protected function createValidator(): ValidIdentifierConstraintValidator
-    {
-        // $this->manager is set in setUp() before parent::setUp() calls this method
-        return new ValidIdentifierConstraintValidator($this->manager);
-    }
+    /**
+     * @var ObjectProphecy|PersonIdentifierManagerInterface
+     */
+    private ObjectProphecy $manager;
 
     protected function setUp(): void
     {
-        // Prepare manager prophecy and reveal it before parent::setUp()
-        $managerProphecy = $this->prophesize(PersonIdentifierManagerInterface::class);
-        $this->manager = $managerProphecy->reveal();
-
-        // Store the prophecy itself for later configuration in tests
-        $this->managerProphecy = $managerProphecy; // dynamic property for test methods
-
+        $this->manager = $this->prophesize(PersonIdentifierManagerInterface::class);
         parent::setUp();
     }
 
-    public function testItCallsEngineValidateThroughWorker(): void
+    protected function createValidator(): ValidIdentifierConstraintValidator
     {
-        // Arrange a definition and corresponding identifier
-        $definition = new PersonIdentifierDefinition(label: ['en' => 'Any'], engine: 'any.engine');
+        return new ValidIdentifierConstraintValidator($this->manager->reveal());
+    }
+
+    public function testAddsViolationFromWorker(): void
+    {
+        $definition = new PersonIdentifierDefinition(['en' => 'SSN'], 'string');
+        // set definition id via reflection for definition_id parameter
+        $ref = new \ReflectionClass($definition);
+        $prop = $ref->getProperty('id');
+        $prop->setValue($definition, 1);
+
         $identifier = new PersonIdentifier($definition);
+        $identifier->setValue(['value' => 'bad']);
 
-        // Create an engine prophecy; we only care that validate() is called once with expected args
-        $engineProphecy = $this->prophesize(\Chill\PersonBundle\PersonIdentifier\PersonIdentifierEngineInterface::class);
-        $engineProphecy
-            ->validate($this->context, $identifier, $definition)
-            ->shouldBeCalled();
+        $violation = new IdentifierViolationDTO('Invalid Identifier', '0000-1111-2222-3333', ['{{ foo }}' => 'bar']);
 
-        // Build a real worker that wraps our mocked engine and the concrete definition
-        $worker = new PersonIdentifierWorker($engineProphecy->reveal(), $definition);
+        // engine that returns one violation
+        $engine = new class ([$violation]) implements PersonIdentifierEngineInterface {
+            public function __construct(private array $violations) {}
 
-        // Configure the manager to return our worker for this definition
-        $this->managerProphecy
+            public static function getName(): string
+            {
+                return 'dummy';
+            }
+
+            public function canonicalizeValue(array $value, PersonIdentifierDefinition $definition): ?string
+            {
+                return null;
+            }
+
+            public function buildForm(\Symfony\Component\Form\FormBuilderInterface $builder, PersonIdentifierDefinition $personIdentifierDefinition): void {}
+
+            public function renderAsString(?PersonIdentifier $identifier, PersonIdentifierDefinition $definition): string
+            {
+                return '';
+            }
+
+            public function isEmpty(PersonIdentifier $identifier): bool
+            {
+                return false;
+            }
+
+            public function validate(PersonIdentifier $identifier, PersonIdentifierDefinition $definition): array
+            {
+                return $this->violations;
+            }
+        };
+        $worker = new PersonIdentifierWorker($engine, $definition);
+
+        $this->manager
             ->buildWorkerByPersonIdentifierDefinition($definition)
             ->willReturn($worker);
 
-        // Act: run the validator
-        $this->validator->validate($identifier, new ValidIdentifierConstraint());
+        $constraint = new ValidIdentifierConstraint();
+        $this->validator->validate($identifier, $constraint);
 
-        // Assert: no explicit assertion needed; Prophecy will fail if method wasn't called
-        $this->assertNoViolation();
+        $this->buildViolation('Invalid Identifier')
+            ->setParameters(['{{ foo }}' => 'bar'])
+            ->setParameter('{{ code }}', '0000-1111-2222-3333')
+            ->setParameter('definition_id', '1')
+            ->assertRaised();
     }
 }