Skip to content

Conversation

CvekCoding
Copy link
Contributor

I have added a test which shows my issue - if you (or DI) instantiate basic symfony serializer, it invokes setSerializer method of all SerializerAware services and rewrites serializer property from DunglasSerializer to SymfobySerializer.
This breaks the further normalization.

@CvekCoding
Copy link
Contributor Author

The added test is totally the same like first one with only one difference - in a first line I instantiate SymfonySerializer. This way I imitated what happens in my project - when Symfony starts, it creates all the listeners, and some of them have SerializerInterface dependency.

@Toflar
Copy link
Contributor

Toflar commented Jul 26, 2019

@ajgarlag is that releated to your changes maybe? 😊

@ajgarlag
Copy link
Contributor

Surely! I think this is related to symfony/symfony#28544. @CvekCoding Can you try to revert a8c3c0a and tell us if it works?

@CvekCoding
Copy link
Contributor Author

@ajgarlag yes, this helped, thanks. I'll continue my investigations.

Copy link
Contributor

@ajgarlag ajgarlag left a comment

Choose a reason for hiding this comment

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

The raw_object suffix was inherited from the legacy implementation. We can use object here.

@@ -5,10 +5,20 @@
xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

<services>
<service id="dunglas_doctrine_json_odm.normalizer.raw_object" class="Symfony\Component\Serializer\Normalizer\ObjectNormalizer" public="false">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<service id="dunglas_doctrine_json_odm.normalizer.raw_object" class="Symfony\Component\Serializer\Normalizer\ObjectNormalizer" public="false">
<service id="dunglas_doctrine_json_odm.normalizer.object" class="Symfony\Component\Serializer\Normalizer\ObjectNormalizer" public="false">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

<argument type="service" id="serializer.denormalizer.array" />
<argument type="service" id="serializer.normalizer.object" />
<argument type="service" id="dunglas_doctrine_json_odm.normalizer.array" />
<argument type="service" id="dunglas_doctrine_json_odm.normalizer.raw_object" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<argument type="service" id="dunglas_doctrine_json_odm.normalizer.raw_object" />
<argument type="service" id="dunglas_doctrine_json_odm.normalizer.object" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Toflar
Copy link
Contributor

Toflar commented Jul 26, 2019

Can I ask you to fix the CS issues? Thank you!

@CvekCoding
Copy link
Contributor Author

Can I ask you to fix the CS issues? Thank you!

Done

@CvekCoding CvekCoding mentioned this pull request Jul 26, 2019
@CvekCoding
Copy link
Contributor Author

Added a few important notes here https://github.com/dunglas/doctrine-json-odm/pull/74/files
Please, look into them.

@otakupahp
Copy link

Since there is only a conflict in the README.md, could anyone fix this and merge this pull?

@Toflar
Copy link
Contributor

Toflar commented Oct 23, 2019

@ajgarlag sorry to ping you again but I haven't looked into this project anymore since you reworked the serializer part. Can I ask you to review this and tell me what to do with the README?
I don't currently have the time to review in depth and it seems like @dunglas doesn't either 😄

@ajgarlag
Copy link
Contributor

@Toflar I think this this PR should be merged and tagged as rc2. The README changes should be applied too.

@Toflar Toflar merged commit aecbaae into dunglas:master Oct 24, 2019
@Toflar
Copy link
Contributor

Toflar commented Oct 24, 2019

Thank you @CvekCoding and @ajgarlag! I've also tagged rc2! Sorry folks that it took so long!
I set myself a reminder in about 2 weeks time and I'd like to ask you to test everything again and if everything is fine, I'll tag 1.0.0 stable in 2 weeks!

@Toflar
Copy link
Contributor

Toflar commented Nov 6, 2019

As promised: Version 1.0.0 stable is here!

Thanks to everyone involved! You may now update your Composer requirements and require the stable version 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants