-
-
Notifications
You must be signed in to change notification settings - Fork 311
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
Added DoctrineObjectConstructor #185
Conversation
|
||
namespace JMS\SerializerBundle\Serializer\Construction; | ||
|
||
use Symfony\Bridge\Doctrine\ManagerRegistry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should typehint the interface instead (available in Doctrine Common)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was exactly what you wanted me to modify. Have I missed something?
It was the ManagerInterface and you requested to be the ManagerRegistry, so I modified it.
Which one do you prefer? Interface of this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guilhermeblanco you were using the BC (deprecated) RegistryInterface of the bridge. I wanted you to use the ManagerRegistry interface available in Doctrine Common, not the Symfony\Bridge\Doctrine\ManagerRegistry
implementation of Doctrine\Common\Persistence\ManagerRegistry
What is the correct way to enable this functionality in a Symfony app? I was able to hack it on by editing line 154 of vendor/jms/serializer-bundle/JMS/SerializerBundle/Resources/config/services.xml to read: <service id="jms_serializer.object_constructor" alias="jms_serializer.doctrine_object_constructor" public="false" /> Obviously, this is not the right way to do it. Shouldn't there be a config parameter to say which constructor to use? Or am I misunderstanding something. |
@sgarner, yep, seems to be an incomplete PR |
@sgarner what did you do to get it working? (in detail, how did you hack the config? ;) I have some missing references after persisting to doctrine too.. |
@bartrail I just edited that line referenced in my previous comment. That's all. Of course if you want that change to be deployable then you need to first create your own fork of JMSSerializerBundle in which to make the change, and then set your project to use your fork in composer. When I have time I will look into making a patch to expose this functionality from config, if nobody else cares to do that before me :) |
@sgarner thanks, after reading the config twice, I got it ;) then I added this line to my servies.yml
which does basically the same (register the DoctrineObjectConstructor) by overloading the |
composer is preinstalled on travis
Fixed #141