IBX-9727: Fixed strict types of translatable Exceptions and Values #590

alongosz · 2025-06-10T15:33:57Z

🎫 Issue	IBX-9727

Related PRs:

Description:

TL;DR;

There are two interfaces with translatable contracts:

\Ibexa\Core\Base\Translatable
\Ibexa\Contracts\Core\Repository\Translatable
fixed strict types there and aligned code extending them. Mostly: exceptions and validation errors.

Interesting parts

After adding strict types I've stumbled upon two "gems" spotted by PHPStan (see: 37833d8):

We were not instantiating ValidationError correctly inside of the BinaryBase::validateValidatorConfiguration method (binary field types), passing target as the part of $values argument instead of $target argument. It's used by strtr so the bug was harmless and invisible - superfluous value with index 0 was ignored. However if you'd placed literal '0' anywhere in the message, it would get replaced :-)
We passed an array to $values's value, which accepts only scalars. BlockingLimitationType would crash with Array to string conversion error in such case. This Limitation is not used anymore in the system, so it was also invisible.

The second thing spotted by PHPStan was the fact that we allow for UnauthorizedException properties which values are non-strings, but also: other scalars, Stringables (hypothetical case tbh), and nulls. This was my mistake, which was fixed and squashed into ac9304c.

While investigating those issues, I've created 2 unit test cases for easier debugging of "things" - ValidationErrorTest and UnauthorizedExceptionTest. Added them here for the future reference of how these "things" work.

Minor out of scope change: it's recommended in PHP 8 to use

get_debug_type($var)

instead of e.g.:

is_object($var) ? get_class($var) : gettype($var),

That looks more compact, however it's not 1:1, as e.g.: "integer" becomes "int". Aligned the existing test case with that change.

For QA:

Vast scope. Regression run needs to be enough.

alongosz

Below you can find some comments to the changes:

src/lib/MVC/Symfony/Templating/Exception/InvalidResponseException.php

alongosz · 2025-06-10T22:19:27Z

src/lib/Limitation/BlockingLimitationType.php

                null,
                [
-                    'value' => $limitationValue->limitationValues,
+                    'identifier' => $limitationValue->getIdentifier(),


Note
This is my best guess of what the Author meant to place here. At least it makes the most sense.

alongosz · 2025-06-10T22:24:06Z

src/lib/Base/Exceptions/UnauthorizedException.php

        $this->setParameters(['%module%' => $module, '%function%' => $function]);

-        if ($properties) {
+        if (!empty($properties)) {


Note
Actually needs to behave the same for [] and null (discovered by test coverage).

alongosz · 2025-06-10T22:26:35Z

src/lib/FieldType/BinaryBase/Type.php

                    if (!is_int($parameters['maxFileSize']) && $parameters['maxFileSize'] !== null) {
                        $validationErrors[] = new ValidationError(
-                            'Validator %validator% expects parameter %parameter% to be of %type%.',
+                            'Validator %validator% expects parameter %parameter% to be of %type% type.',


Note
The message actually is not grammatically correct without the type at the end (see the unit test coverage). Hopefully 🤞 it's not covered literally by behat anywhere. Are those in xliff files somewhere though?

alongosz · 2025-06-10T22:28:17Z

src/lib/Persistence/Legacy/Content/Type/Handler.php


        if (count($groups) !== 1) {
-            throw new Exception\TypeGroupNotFound($groupId);
+            throw new Exception\TypeGroupNotFound((string)$groupId);


Note
We're using that one with both numeric IDs and string identifiers. I prefer this way instead of doing int|string on the exception class argument type.

alongosz · 2025-06-10T22:33:05Z

tests/lib/Base/Exceptions/ContentFieldValidationExceptionTest.php

+final class ContentFieldValidationExceptionTest extends TestCase
+{
+    /**
+     * @see \Ibexa\Core\Base\Exceptions\ContentFieldValidationException::MAX_MESSAGES_NUMBER
+     */
+    private const int MAX_MESSAGES_NUMBER = 32;
+
+    public function testTranslatableMessageValidationErrorLimit(): void
+    {
+        $errors = [];
+        for ($fieldId = 1; $fieldId <= self::MAX_MESSAGES_NUMBER + 1; ++$fieldId) {
+            $errors[$fieldId] = [
+                'eng-GB' => [new ValidationError("Field $fieldId error message")],
+            ];
+        }
+        $exception = ContentFieldValidationException::createNewWithMultiline($errors);
+        self::assertStringEndsWith(
+            sprintf('- Limit of %d validation errors has been exceeded.', self::MAX_MESSAGES_NUMBER),
+            $exception->getMessage()
+        );
+    }
+}


Note
Needed to fix ContentFieldValidationException so validation messages overflow error is an instance of ValidationError too (previously was a string), for the type consistency in the array. Hence the test double checking the logic.

alongosz · 2025-06-10T22:34:32Z

tests/lib/Repository/Values/Filter/FilterTest.php

            ],
            'Expected an instance of "Ibexa\Contracts\Core\Repository\Values\Filter\FilteringSortClause", ' .
-            'got "integer" at position 1',
+            'got "int" at position 1',


Note
The mentioned side-effect of using get_debug_type

src/contracts/FieldType/Generic/ValidationError/ConstraintViolationAdapter.php

src/contracts/FieldType/ValidationError.php

src/lib/Base/Exceptions/Httpable.php

src/lib/Base/Exceptions/InvalidArgumentValue.php

src/lib/Base/Exceptions/UnauthorizedException.php

src/lib/FieldType/ValidationError.php

src/lib/MVC/Symfony/Templating/Exception/InvalidResponseException.php

tests/lib/Base/Exceptions/UnauthorizedExceptionTest.php

src/lib/FieldType/ValidationError.php

adamwojs · 2025-06-15T16:58:34Z

@alongosz Do we need to adapt code base in other repositories, before the merge ?

alongosz · 2025-06-15T19:04:14Z

@alongosz Do we need to adapt code base in other repositories, before the merge ?

@adamwojs regressions are clear, but given we like to extend things from core we shouldn't, maybe I'll run PHPStan against this branch on all repos. Just occurred to me now 😅

…ructor with the changes

…tification

Co-Authored-By: Konrad Oboza <[email protected]>

sonarqubecloud · 2025-06-16T14:30:41Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* [Tests] Fixed strict types after ibexa/core#590 changes * [PHPStan] Removed resolved issues from the baseline

github-actions bot assigned alongosz, tischsoic and mikadamczyk Jun 10, 2025

alongosz force-pushed the ibx-9727-translatable-strict-types branch from 2c28150 to f954065 Compare June 10, 2025 21:48

alongosz marked this pull request as ready for review June 10, 2025 22:16

alongosz mentioned this pull request Jun 10, 2025

Run regression for IBX-9727: ibexa/core#590 ibexa/commerce#1343

Closed

alongosz commented Jun 10, 2025

View reviewed changes

alongosz force-pushed the ibx-9727-translatable-strict-types branch from 578764c to ee563bf Compare June 10, 2025 22:43

alongosz requested a review from a team June 11, 2025 07:47

ezrobot requested review from Steveb-p, ViniTou, adamwojs, barw4, ciastektk, mikadamczyk and wiewiurdp and removed request for a team June 11, 2025 07:48

tbialcz reviewed Jun 11, 2025

View reviewed changes

src/contracts/FieldType/Generic/ValidationError/ConstraintViolationAdapter.php Outdated Show resolved Hide resolved

konradoboza reviewed Jun 11, 2025

View reviewed changes

konradoboza approved these changes Jun 11, 2025

View reviewed changes

barw4 approved these changes Jun 11, 2025

View reviewed changes

src/lib/FieldType/ValidationError.php Outdated Show resolved Hide resolved

src/lib/FieldType/ValidationError.php Outdated Show resolved Hide resolved

tbialcz approved these changes Jun 11, 2025

View reviewed changes

alongosz force-pushed the ibx-9727-translatable-strict-types branch from 9377fb4 to b3f3870 Compare June 13, 2025 11:20

adamwojs approved these changes Jun 15, 2025

View reviewed changes

alongosz and others added 13 commits June 16, 2025 13:42

Fixed strict types of Translatable classes

1da8c74

Fixed strict types for translatable contracts

4a661d2

Fixed strict types for translatable Exception classes

c2a6b08

Aligned related code with the changes

75be102

[Tests] Aligned related tests with the changes

0bb4123

Fixed incorrect instantiation of ValidationError across the codebase

c0e1f3f

[Tests] Added unit test for ValidationError class

33e0209

[Tests] Added unit test for UnauthorizedException class

1d8220c

[Tests] Aligned the expected exception message thrown by Filter const…

f324831

…ructor with the changes

Fixed InvalidArgumentException usage in NotificationService::createNo…

ac14249

…tification

Changed some ctor arg list to be multiline to increase readability

f309af5

Marked InvalidResponseException as final

cab29f8

Co-Authored-By: Konrad Oboza <[email protected]>

[PHPStan] Removed resolved issues from the baseline

19ab54a

alongosz force-pushed the ibx-9727-translatable-strict-types branch from b3f3870 to 19ab54a Compare June 16, 2025 11:47

alongosz added 2 commits June 16, 2025 16:28

Allowed null values for validation message interpolation parameters

f7d20c1

[Tests] Added coverage for ValidationError with null value

8052733

alongosz merged commit be546a9 into main Jun 17, 2025
16 checks passed

alongosz deleted the ibx-9727-translatable-strict-types branch June 17, 2025 16:35

alongosz added a commit to ibexa/fieldtype-richtext that referenced this pull request Jun 24, 2025

[Tests] Fixed strict types after ibexa/core#590 changes

8c17d4c

alongosz mentioned this pull request Jun 24, 2025

IBX-9727: [Tests] Fixed strict types after ibexa/core#590 changes ibexa/fieldtype-richtext#243

Merged

adamwojs pushed a commit to ibexa/fieldtype-richtext that referenced this pull request Jun 25, 2025

IBX-9727: [Tests] Fixed strict types after ibexa/core#590 changes (#243)

3a4f246

* [Tests] Fixed strict types after ibexa/core#590 changes * [PHPStan] Removed resolved issues from the baseline

This was referenced Jul 21, 2025

[UI][i18n] Added missing translations #622

Merged

[UI][i18n] Extracted missing translations ibexa/fieldtype-query#45

Merged

[UI][i18n] Extracted missing translations ibexa/user#116

Merged

Uh oh!

IBX-9727: Fixed strict types of translatable Exceptions and Values #590

IBX-9727: Fixed strict types of translatable Exceptions and Values #590

Uh oh!

Conversation

alongosz commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related PRs:

Description:

TL;DR;

Interesting parts

For QA:

Uh oh!

alongosz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alongosz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

alongosz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

alongosz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

alongosz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

alongosz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

alongosz Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamwojs commented Jun 15, 2025

Uh oh!

alongosz commented Jun 15, 2025

Uh oh!

sonarqubecloud bot commented Jun 16, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alongosz commented Jun 10, 2025 •

edited

Loading