Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-8190: Add Symfony's Serializer support #121

Open
wants to merge 98 commits into
base: main
Choose a base branch
from
Open

Conversation

barw4
Copy link
Contributor

@barw4 barw4 commented Aug 12, 2024

🎫 Issue IBX-8190

Related PRs:

https://github.com/ibexa/scheduler/pull/111

Description:

AdapterNormalizer

A new normalizer with low priority is implemented called AdapterNormalizer that handles data normalization or visiting data using the existing visitors. Currently, the visitors are prioritized over incoming (new) normalizers.

Unfortunately, the problem is that in order to visit value objects the Visitor and Generator have to be created and passed as arguments to ValueObjectVisitor instance.

        $valueObjectVisitor->visit($visitor, $generator, $object);

Ibexa\Contracts\Rest\Output\ValueObjectVisitorDispatcher

It is deleted now and the responsibility for finding a proper visitor is moved to the newly introduced ValueObjectVisitorResolverInterface.

Ibexa\Contracts\Rest\OutputVisitor

The visitValueObject method has to behave exactly the same as before as it's used throughout the app so we should keep BC here. I don't think we can change it to use normalizer as it uses the original Visitor's generator.

Concept made by Paweł.

For QA:

TBD

Documentation:

TBD

@barw4 barw4 self-assigned this Aug 12, 2024
@barw4 barw4 changed the title [POC] [Work in progress] Add Symfony's Serializer support [POC] Add Symfony's Serializer support Aug 26, 2024
@barw4 barw4 requested a review from a team August 26, 2024 12:59
@Steveb-p
Copy link
Contributor

Since I consulted this approach I'll give some explanations too.

Whole concept relies on using Serializer as primary entry point for data normalization.

While analyzing how Visitors worked with Generator, it became apparent that Generator basically serves as a Data Container that changes it's internal "pointer" as instructed by Visitor. This is "shared" between XML and JSON Generators, with the difference being that XML outputs it's state during this process.

So, since the primary difference between Symfony normalization and our Visitor's processing is that Symfony returns data and Visitors pass around Generator (as data storage), we can give Visitors a "fake" Generator - which whole purpose is to store data - and use it's state at the end as normalization result. This results in both processes behaving in a similar way.

@barw4 barw4 changed the title [POC] Add Symfony's Serializer support [POC] IBX-8190: Add Symfony's Serializer support Aug 29, 2024
@barw4 barw4 added Feature New feature request Ready for review and removed Work in progress labels Oct 24, 2024
phpunit.integration.xml Outdated Show resolved Hide resolved
phpunit.integration.xml Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/VisitorAdapterNormalizer.php Outdated Show resolved Hide resolved
src/lib/Output/Generator/Xml/FieldTypeHashGenerator.php Outdated Show resolved Hide resolved
src/lib/Output/Normalizer/ArrayListNormalizer.php Outdated Show resolved Hide resolved
@barw4 barw4 requested a review from Steveb-p November 4, 2024 09:27
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

A lot of solid work 💪

Some remarks & questions:

src/bundle/Resources/config/services.yml Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
src/contracts/Output/Generator.php Outdated Show resolved Hide resolved
Comment on lines 134 to 135
$this->json->$name = $object;
$this->json = $object;
Copy link
Member

Choose a reason for hiding this comment

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

The first line doesn't make sense here, as it's immediately overridden by the next line.

tests/integration/Serializer/SerializerTest.php Outdated Show resolved Hide resolved
use NormalizerAwareTrait;

/**
* @return array<string, mixed>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return array<string, mixed>
* @return array<string, mixed>
*
* @throws \Symfony\Component\Serializer\Exception\ExceptionInterface

tests/integration/bootstrap.php Outdated Show resolved Hide resolved
Comment on lines +293 to +304
if ($isXml) {
$expectedXml = new \DOMDocument();
$expectedXml->preserveWhiteSpace = false;
$expectedXml->formatOutput = true;
$expectedXml->load($fixtureFile);

$actualXml = new \DOMDocument();
$actualXml->preserveWhiteSpace = false;
$actualXml->formatOutput = true;
$actualXml->loadXML($actualResult);

self::assertSame($expectedXml->saveXML(), $actualXml->saveXML());
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried to use assertXmlStringEqualsXmlFile instead?

Copy link
Contributor Author

@barw4 barw4 Nov 4, 2024

Choose a reason for hiding this comment

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

Yes and I think this method expects clean xml, without:

<?xml version="1.0" encoding="UTF-8"?>

because I'm getting:

Could not read <?xml version="1.0" encoding="UTF-8"?>

with it.

@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<elements>
<element attribute="attribute value 1">element value 1</element>
<element attribute="attribute value 2">element value 2</element>
<element2 attribute="attribute value 2">element value 2</element2>
Copy link
Member

Choose a reason for hiding this comment

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

What's the source of/reason for that change?

Copy link
Contributor Author

@barw4 barw4 Nov 4, 2024

Choose a reason for hiding this comment

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

The reason for this is that startHashElement does not support multiple elements of the same name, just like Json Generator. The proper way to handle this would be to use the startHashElement with conjunction of the startList like it's done for example in the ContentFieldValidationException visitor:

        $generator->startHashElement('errorDetails');
        $generator->startList('fields');

We are imo safe here with this change because our visitors are both handling Json and Xml and if we had something like this in some visitor:

        $generator->startHashElement('elements');

        $generator->startValueElement('element', 'element value 1', ['attribute' => 'attribute value 1']);
        $generator->endValueElement('element');

        $generator->startValueElement('element', 'element value 2', ['attribute' => 'attribute value 2']);
        $generator->endValueElement('element');

        $generator->endHashElement('elements');

JSON output would be also broken which was never the case.

@alongosz alongosz requested a review from a team November 4, 2024 13:50
Copy link

sonarcloud bot commented Nov 4, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature request Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants