Skip to content

Conversation

@Sztig
Copy link
Contributor

@Sztig Sztig commented Mar 12, 2025

🎫 Issue IBX-9678, IBX-10267

Edit from @konradoboza:

It seems, Symfony 7+ has its own counterpart for JsonSerializableNormalizer, removed our implementation.

Description:

JsonSerializableNormalizer is hint typed to return string but we also normalize objects like Money/Money that return an array. If we try to create an order through API on latest Ibexa 5.0 the order would get created but on the return we would get an 500 error. I have changed the hint type to cover array as well.

I have also added a test to cover this scenario. Rest package doest not uses moneyphp/money package anywhere directly so instead of adding a dependency I have created a dummy object with minimal implementation to simulate the behavior of real Money object that returns an array.

Discovered while testing https://github.com/ibexa/order-management/pull/137 on 5.0

For QA:

Documentation:

@Sztig Sztig requested a review from a team March 12, 2025 15:38
@sonarqubecloud
Copy link

Copy link
Contributor

@barw4 barw4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's an overkill but it would be great to have an integration test with a bit more complicated case, like object that uses MoneyObjecy, please see https://github.com/ibexa/rest/blob/main/tests/integration/Serializer/SerializerTest.php

Maybe you could add another case there and use TestDataObject with MoneyObject.

@barw4 barw4 requested a review from a team March 12, 2025 16:09
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, whole JsonSerializableNormalizer class can be removed, as Symfony\Component\Serializer\Normalizer\JsonSerializableNormalizer already exists.

☝️ all we need to take care is to properly register this as a service (under a non-class id, with class declared, if using YAML service files) to the correct serializers.

@konradoboza konradoboza added Bug Something isn't working Ready for review labels Jul 4, 2025
@konradoboza konradoboza self-assigned this Jul 4, 2025
@konradoboza konradoboza force-pushed the IBX-9678-JsonSerializableNormalizer-fix branch from bebc091 to 2aecebc Compare July 4, 2025 08:03
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 4, 2025

Copy link

@KamilSznajdrowicz KamilSznajdrowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Approved.
I checked the REST action on the API orders and confirmed that the 500 error in the console is no longer occurring.

@konradoboza konradoboza merged commit 0b71586 into main Jul 8, 2025
11 checks passed
@konradoboza konradoboza deleted the IBX-9678-JsonSerializableNormalizer-fix branch July 8, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working QA approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants