Improved json schema validation handling by daniel-myparcel · Pull Request #7 · MyParcelCOM/json-schema-form-builder

daniel-myparcel · 2025-11-21T14:47:03Z

✨ Moved to a more up-to-date Json schema validation library
♻️ Refactored validation to be under its own module (Validation\Validator)

Doing this will allow us to validate values against a schema without requiring initializing a Form instance

Jira Ticket

src/Form/Form.php

composer.json

NickVries · 2025-11-25T14:26:10Z

src/Validation/Exceptions/ValidationException.php

+{
+    private array $errors;
+    public function __construct(
+        private readonly ValidationResult $validationResult,


Seems that this variable is only used inside of the constructor itself. If that's the case, we don't need it as a class property.

Suggested change

private readonly ValidationResult $validationResult,

ValidationResult $validationResult,

NickVries · 2025-11-26T08:12:34Z

src/Validation/Validator.php

+        if (!$result->isValid()) {
+            throw new ValidationException($result);
+        }


I would return the result of the validator here instead and use evaluate the result and handle accordingly in the class that uses the validator instead.

The responsibility of this class should be to validate the incoming data against a schema and then return a result. Whether to actually stop the execution of the code and throw an exception is not the concern of the Validator itself, because context is important, which the Validator class is lacking.

src/Form/Form.php

NickVries · 2025-11-26T08:15:31Z

tests/Form/FormTest.php

                ),
            ),
        );
+        dump($this->form->toJsonSchema());


Suggested change

dump($this->form->toJsonSchema());

NickVries · 2025-11-26T08:21:25Z

tests/Form/FormTest.php

-    /**
-     * @throws FormValidationException
-     */
-    public function test_it_validates(): void


These tests related to validation have been removed, likely because the validation logic has shifted. But still, I would expect there to be at least some tests related to validation, since the validate method still exists.

If you're struggling with testing the static method, then you might consider injecting the validator, either through a constructor or a method parameter. That way the validator object can be mocked and its failed and success flows can be tested here.

NickVries · 2025-11-26T08:36:23Z

tests/Validation/ValidatorTest.php

+    protected array $schema;
+    protected function setUp(): void
+    {
+        parent::setUp(); // TODO: Change the autogenerated stub


Suggested change

parent::setUp(); // TODO: Change the autogenerated stub

parent::setUp();

NickVries · 2025-11-26T08:40:17Z

tests/Validation/ValidatorTest.php

+    public function test_it_fails_to_validate_required_missing(): void
+    {
+        $this->expectException(ValidationException::class);
+        Validator::validate([
+            'name_1' => 'value',
+            'name_2' => false,
+            'name_4' => [
+                'name_1' => 'value',
+                'name_3' => 'a',
+            ],
+        ], $this->schema);
+    }
+
+    public function test_it_fails_to_validate_wrong_property_type(): void
+    {
+        $this->expectException(ValidationException::class);
+        Validator::validate([
+            'name_1' => 5,
+            'name_2' => 'hello',
+            'name_3' => 'a',
+            'name_4' => [
+                'name_1' => 'value',
+                'name_3' => 'a',
+            ],
+        ], $this->schema);
+    }
+
+    public function test_it_fails_to_validate_invalid_enum_value(): void
+    {
+        $this->expectException(ValidationException::class);
+        Validator::validate([
+            'name_1' => 'value',
+            'name_2' => true,
+            'name_3' => 'x',
+            'name_4' => [
+                'name_1' => 'value',
+                'name_2' => true,
+                'name_3' => 'y',
+            ],
+        ], $this->schema);
+    }


If you decide to test different use cases where validation can fail, I expect an assertion that that specific error is what failed the validation as well. For example, the first test expects validation to fail on missing required properties. If you decide to test that specifically (which you might argue is not even the validator's responsibility, because we didn't write that logic) I expect you to also have an assertion on the thrown Exception that reflects that specific problem.

I added assertions to the error values, if you think it makes more sense to drop these tests altogether I'm not all too attached to it.

In my previous comment I mentioned that I don't think the specific cases are the responsibility of this validator. I think a better test would be to make sure the external validator class is called and that our Validator class handles according to its return value.

Ah right now I get you. I will make the change

I replaced the testing in Validator with mock so that we only assert the third party library code is called as expected. I hope that I didn't miss something in your explanation definitly got a little confused with this one 🫠

NickVries

Whoops, forgot to click "Request changes" with my review 🙃

daniel-myparcel · 2025-11-27T14:28:11Z

src/Form/Form.php

     * @throws FormValidationException
     */
-    public function validate(array $values): void
+    public function validate(array $values, ?Validator $validator = null, ?ErrorFormatter $errorFormatter = null): void


I'm not gonna say I'm the biggest fan of this approach but since the $errorFormatter behaviour started requiring a lot of mocking I opted for also passing it in the validate method.

Lmk what you think.

Yikes, I'm absolutely not a fan of this haha. The caller of $form->validate() now has to pass an $errorFormatter so that the Form then can pass it to the Validator, who can potentially pass it to the FormValidationException in case validation fails. That means that 4 classes are now aware of this formatter that has logic that should really only concern one of them and I don't think it's the Exception class.

Our own Validator class is already tightly coupled with the package, so it wouldn't add any coupling to have the Validator also instantiate the ErrorFormatter. Furthermore, I would personally make the Exception as dumb as possible with as little dependencies as possible. The validator can get a getErrors method that uses the ErrorFormatter to produce the errors in the required format so that the exception can just be dumb and act as an Exception only and not contain logic to format errors. Exceptions can be thrown anywhere with whatever context is available at that time. Making one accept a dependency also tightly couples the context of the Exception to the ErrorFormatter because it needs to accept the expected data to pass into the ErrorFormatter in order to produce a result. That's way too tightly coupled for my taste.

NickVries

Whoops, forgot to click "Request changes" with my review 🙃

NickVries · 2025-12-01T13:55:11Z

src/Form/Form.php

     * @throws FormValidationException
     */
-    public function validate(array $values): void
+    public function validate(array $values, ?Validator $validator = null, ?ErrorFormatter $errorFormatter = null): void


Yikes, I'm absolutely not a fan of this haha. The caller of $form->validate() now has to pass an $errorFormatter so that the Form then can pass it to the Validator, who can potentially pass it to the FormValidationException in case validation fails. That means that 4 classes are now aware of this formatter that has logic that should really only concern one of them and I don't think it's the Exception class.

Our own Validator class is already tightly coupled with the package, so it wouldn't add any coupling to have the Validator also instantiate the ErrorFormatter. Furthermore, I would personally make the Exception as dumb as possible with as little dependencies as possible. The validator can get a getErrors method that uses the ErrorFormatter to produce the errors in the required format so that the exception can just be dumb and act as an Exception only and not contain logic to format errors. Exceptions can be thrown anywhere with whatever context is available at that time. Making one accept a dependency also tightly couples the context of the Exception to the ErrorFormatter because it needs to accept the expected data to pass into the ErrorFormatter in order to produce a result. That's way too tightly coupled for my taste.

…ception directly in the Form class

NickVries · 2025-12-03T10:19:49Z

src/Form/Form.php

     * @throws FormValidationException
     */
-    public function validate(array $values): void
+    public function validate(array $values, ?Validator $validator = null): void


Nice! I really like this entire method and how it reads now! Good job.

NickVries · 2025-12-03T10:29:03Z

src/Validation/Validator.php

+            : [];
+    }
+
+    private function getValidationResult(): ValidationResult


This method is called multiple times, but relies on the read-only class properties, and will therefore not yield different results. Let's cache the result to remove this duplicated step. We can cache the result on the class after the first time it's performed and check if that property is set in each call (and set it otherwise of course).

Good point!

I took a slightly different approach: init an instance of ValidationResult in the constructor and then use it to get both the isValid and getErrors. Since it never changes this will work exactly the same 👍

NickVries · 2025-12-03T10:34:03Z

tests/Form/FormTest.php

-    public function test_it_validates(): void
+    public function test_it_validates_success(): void
    {
        $this->expectNotToPerformAssertions();


Not super important, but maybe there is a expectNotToThrowExceptions or similar? Since that is the only thing the method does apart from invoking other classes. Besides, the mocked expectations are assertions, so I don't think we need this statement at all.

There doesn't seem to be such an assertion. Copilot seems to agree:

If you want to explicitly check that no exception is thrown, simply run the code without setting any exception expectations. If an exception is thrown, the test will fail automatically.

NickVries · 2025-12-03T10:38:12Z

tests/Validation/ValidatorTest.php

+        self::assertTrue($validator->isValid());
+        self::assertEquals([], $validator->getErrors());
+
+        $values = [


Let's put this in a separate test, so that we isolate one type of test per method and dont have to figure out what part of this test fails in case it does.

…r to avoid redundantly reinstantiating that class

daniel-myparcel changed the title ~~Feature/mp 7662 improved json schema validation handling~~ Improved json schema validation handling Nov 21, 2025

daniel-myparcel marked this pull request as ready for review November 21, 2025 15:18

M4tini self-assigned this Nov 24, 2025

daniel-myparcel commented Nov 24, 2025

View reviewed changes

src/Form/Form.php Outdated Show resolved Hide resolved

M4tini reviewed Nov 24, 2025

View reviewed changes

composer.json Show resolved Hide resolved

NickVries reviewed Nov 26, 2025

View reviewed changes

NickVries requested changes Nov 26, 2025

View reviewed changes

M4tini assigned NickVries and unassigned M4tini Nov 26, 2025

daniel-myparcel commented Nov 27, 2025

View reviewed changes

NickVries requested changes Dec 1, 2025

View reviewed changes

daniel-myparcel added 19 commits December 3, 2025 10:21

composer dependencies

55c6822

✨ replaced our validation logic to use new lib + fixed failing tests

c3edbc6

Moved validation logic to dedicated module

502d692

updated ->validate()

d468a31

drop redundant class property

422c4a6

return $result instead of throwing exception in validator + handle ex…

b980650

…ception directly in the Form class

delete dump

c83b0d6

Brought back tests for the Form class validation

b9fd233

remove TODO

346ee33

undo new line change

8fd7dea

Added error message assertions

736a70a

Opis json schema now gets mocked instead of called directly

ddc87be

added assertion that returned result is the same as the mocked one

28020d9

improved tests

4c147c3

Refactor code for better testability

2eca39f

Fixed validator test

dc8e9e8

typo

af7f849

typo

1490c79

Fix import mistake after rebase

d19b539

daniel-myparcel force-pushed the feature/MP-7662-improved-json-schema-validation-handling branch from 0e72386 to d19b539 Compare December 3, 2025 09:25

NickVries requested changes Dec 3, 2025

View reviewed changes

daniel-myparcel added 2 commits December 3, 2025 11:51

initialize ValidationResult instance in the constructor of Validato…

b630463

…r to avoid redundantly reinstantiating that class

divide ValidatorTest to 2 separate test cases - success and failure

75c30aa

daniel-myparcel requested a review from NickVries December 3, 2025 11:01

slightly cleaner mocking in FormTest

89b9d08

NickVries approved these changes Dec 3, 2025

View reviewed changes

NickVries merged commit 451c42e into master Dec 3, 2025
1 check passed

NickVries deleted the feature/MP-7662-improved-json-schema-validation-handling branch December 3, 2025 13:24

	private readonly ValidationResult $validationResult,
	ValidationResult $validationResult,

	parent::setUp(); // TODO: Change the autogenerated stub
	parent::setUp();

Comments

Conversation

daniel-myparcel commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

daniel-myparcel Nov 26, 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!

NickVries 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!

NickVries 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!

daniel-myparcel Dec 3, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daniel-myparcel commented Nov 21, 2025 •

edited

Loading

daniel-myparcel Nov 26, 2025 •

edited

Loading

daniel-myparcel Dec 3, 2025 •

edited

Loading