SonataPageBundle
SonataPageBundle copied to clipboard
sonata-project
Transformer use Symfony Serializer
Subject
This makes the Transformer use Symfony Serializer to normalize/denormalize the Content Array.
Under 4.4 Symfony i had the following restrictions:
- it wasn't easy to exclude attributes, you only could ignore them either global via name, but don't class specific like i wanted, the current solution is to use a group name
- the group name also has the benefit that it orders the attributes, without it, it seems to use the order inside the php class
- You can't denormalize for an Interface, or abstract class. so when it tired to load the block children, it had no idea what to do. And i didn't want to use a discriminator map and type attribute inside the array. I solved this using my Own Denormalizer that is looking for an Interface and Context parameters.
I will add more Testcases soon with doing a functional test using the Symfony Kernel.
I am targeting this branch, because i don't know if that could count as BC or not.
Closes #1503.
Changelog
### Added
- Added some `Class::newMethod()` to do great stuff.
### Changed
### Deprecated
### Removed
### Fixed
### Security
Adding a mandatory param to a constructor is a BC break. The BC way is to use null as a default and if null is provided either do new Service(), either use the old logic.
Here I'm not sure it's worth it to provide a BC PR, it might be easier to target directly 4.x WDYT @jordisala1991 ?
I think it might be nice to have this branch for testing purposes on real apps that are actually using 3.x, but the real PR should target 4.x
Adding a mandatory param to a constructor is a BC break. The BC way is to use null as a default and if null is provided either do new Service(), either use the old logic.
Here I'm not sure it's worth it to provide a BC PR, it might be easier to target directly 4.x WDYT @jordisala1991 ?
I guess just add the serializer as optional, and if it's not passed get from the container, it's fine
I think it might be nice to have this branch for testing purposes on real apps that are actually using 3.x, but the real PR should target 4.x
Do you think it's too much for a minor?
Adding a mandatory param to a constructor is a BC break. The BC way is to use null as a default and if null is provided either do new Service(), either use the old logic. Here I'm not sure it's worth it to provide a BC PR, it might be easier to target directly 4.x WDYT @jordisala1991 ?
I guess just add the serializer as optional, and if it's not passed get from the container, it's fine
You dont have the container either in this class
For the last issue, should i just suppress it?
if i don't it just complains somewhere else in the Denormalizer
For the last issue, should i just suppress it?
if i don't it just complains somewhere else in the Denormalizer
The interface ContextAwareDenormalizerInterface is deprecated since Symfony 6.1 https://github.com/symfony/serializer/blob/6.1/Normalizer/ContextAwareDenormalizerInterface.php#L21
Since like it will be added directly to https://github.com/symfony/Serializer/blob/6.1/Normalizer/DenormalizerInterface.php#L59
So I would recommend to not use the interface. Then we will see the issue you get
especially this one i don't know how to fix:
supportsDenormalization() invoked with 4 parameters, 2-3 required.
For
Method Sonata\PageBundle\Serializer\InterfaceDenormalizer::supportsDenormalization() has parameter $context with no value type specified in iterable type array.
You need to add phpdoc.
For
Method Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization() invoked with 4 parameters, 2-3 required. We will ignore the error (@phpstna-ignore-next-line) with a message to explain why
For
@psalm-suppress MoreSpecificImplementedParamType
I think we have to make the serializerAwareInterface generic in Symfony
@Hanmac there is a DenormalizerAwareInterface.
Dont you think you should both implement DenormalizerAwareInterface and SerializerAwareInterface ? This will avoid to override the phpdoc of setSerializer and avoid the psalm issue.
@Hanmac there is a
DenormalizerAwareInterface.Dont you think you should both implement DenormalizerAwareInterface and SerializerAwareInterface ? This will avoid to override the phpdoc of setSerializer and avoid the psalm issue.
hm i was using ArrayDenormalizer as inspiration, seems it changed after 4.4 for some reason
hm where does it want me to do the psalm thing?
I would say it's because you're using \* and not \**
curious, the tests are broken on my system
assert(): assert(null !== $id) failed
/opt/project/src/CmsManager/CmsPageManager.php:174
/opt/project/src/CmsManager/BaseCmsPageManager.php:52
/opt/project/src/CmsManager/CmsPageManager.php:61
/opt/project/tests/CmsManager/CmsPageManagerTest.php:108
assert(): assert(null !== $id) failed
/opt/project/src/CmsManager/CmsPageManager.php:174
/opt/project/src/CmsManager/BaseCmsPageManager.php:57
/opt/project/src/CmsManager/CmsPageManager.php:63
/opt/project/tests/CmsManager/CmsPageManagerTest.php:149
assert(): assert(null !== $id) failed
/opt/project/src/CmsManager/CmsPageManager.php:174
/opt/project/src/CmsManager/BaseCmsPageManager.php:72
/opt/project/src/CmsManager/CmsPageManager.php:65
/opt/project/tests/CmsManager/CmsPageManagerTest.php:190
assert(): assert(null !== $id) failed
/opt/project/src/CmsManager/CmsSnapshotManager.php:147
/opt/project/src/CmsManager/BaseCmsPageManager.php:72
/opt/project/src/CmsManager/CmsSnapshotManager.php:68
/opt/project/tests/CmsManager/CmsSnapshotManagerTest.php:129
I'll fix it with https://github.com/sonata-project/SonataPageBundle/pull/1527
Just give a 👍🏼 if you have tested it in some project!
I think you're the only one with a project with PageBundle. So we will need you to test it.
Just give a 👍🏼 if you have tested it in some project!
I think you're the only one with a project with PageBundle. So we will need you to test it.
well I just created small project to test, this that I'm doing!
Because after bump the dependencies on the last release, I can't test it from the project that I'm working on!
who create the PR must be responsible to test it!
Two testers are better than one. I thought you could tested it with your own personal/work project.
I personally never used this project, dunno what's it useful for, so I'm not sure I'll be the best tester.
In the same way I tried a bugfix for 4.x release with https://github.com/sonata-project/SonataPageBundle/pull/1524 but it requires to be tested first. Is the repository you used to report all the bug on 4.x branch public ? I could see if my branch fix the issue.
who create the PR must be responsible to test it!
Two testers are better than one. I thought you could tested it with your own personal/work project.
I personally never used this project, dunno what's it useful for, so I'm not sure I'll be the best tester.
In the same way I tried a bugfix for 4.x release with #1524 but it requires to be tested first. Is the repository you used to report all the bug on 4.x branch public ? I could see if my branch fix the issue.
No it's not public :/ But I was thinking to use or create a new SymfonyKernel in test folder to setup a simple project, and we could use this to test, what do you think?
Sure ! if it can be done with https://github.com/sonata-project/SonataPageBundle/blob/3.x/tests/App/AppKernel.php it's great. More test functional would be a great thing for this project with 58% coverage
that's why i was creating mockup tests before doing this one. (the functions affected by the serializer already have coverage now)
but i can try to add more tests to use the Transformer in an AppKernel Test
There are a lot of functional test on bundles like SonataMediaBundle, SonataClassificationBundle and SonataUserBundle. Those test check all the commands, admin routes and, in the UserBundle, check for all the auth process.
You might want to take a look at them. If not, my idea after doing a few more PRs was to start creating this kind of tests.
Please if you do them, open separate PRs, and try to maintain the structure of those bundles.
yeah i know how to write Functional Tests, i did it for the QueryBuilder updates too Especially when the transformer would touch database stuff.
for the Transformer class, i thought that doing mock tests would be good enough for now
yeah i know how to write Functional Tests, i did it for the QueryBuilder updates too Especially when the transformer would touch database stuff.
for the Transformer class, i thought that doing mock tests would be good enough for now
I guess mock tests are enough too, my point is just to make a manual test and see if broken something!