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

Nullable #177

Merged
merged 16 commits into from
Oct 17, 2012
Merged

Nullable #177

merged 16 commits into from
Oct 17, 2012

Conversation

hashnz
Copy link
Contributor

@hashnz hashnz commented Sep 23, 2012

We've decided we want to preserve keys when serializing null values, this adds a flag on each visitor (default false) as to whether to use this behaviour.

$this->nullable = (bool) $serializeNull;
}

public function isNullable()
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming looks weird. The visitor is not nullable. It serializes null values

@hashnz
Copy link
Contributor Author

hashnz commented Sep 24, 2012

Thanks @stof, I've cleaned those up.

@dennisoehme
Copy link

+1 for serialize_null :-)

@schmittjoh
Copy link
Owner

Could you rebase this pull request on master branch, and add a method visitNull to the VisitorInterface?

@schmittjoh
Copy link
Owner

You have a possible null pointer method call:
http://jmsyst.com/reviews/8d433de6-69c7-4472-9ff5-bca7430291a5#comments-F5L75

Could you also add some more tests. For example, visitNull is not covered at the moment:
http://jmsyst.com/reviews/8d433de6-69c7-4472-9ff5-bca7430291a5#comments-F4L89

I soon plan to split the serializer code into a separate standalone library. Please let me know whether you have time to finish this until Friday, otherwise it would need to be rebased again on the new repository.

@hashnz
Copy link
Contributor Author

hashnz commented Oct 16, 2012

@schmittjoh I've added a test on serializing/desreializing null value, let me know if anything else needs to be done, I'd like to get it in before refactor.

$this->serializeNull = (bool) $serializeNull;
}

public function getSerializeNull()
Copy link
Owner

Choose a reason for hiding this comment

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

Can you rename this to shouldSerializeNull?

@schmittjoh
Copy link
Owner

There are just a few minor things, otherwise this looks good.

@hashnz
Copy link
Contributor Author

hashnz commented Oct 16, 2012

The method in the serializer could be more flexible and possibly take over the config. I've left it simple for the moment.

@schmittjoh schmittjoh merged commit 5323bdd into schmittjoh:master Oct 17, 2012
@schmittjoh
Copy link
Owner

I've reverted the config changes for now.

Thanks for your work!

@ruudk
Copy link
Contributor

ruudk commented Nov 8, 2012

Great feature but why can't I enable this inside the configuration? I want this to happen automatically, not with a function call (because I use FOSRestBundle with the ViewResponseListener)

@ruudk
Copy link
Contributor

ruudk commented Nov 8, 2012

For now I fixed this by creating my own serializer:

<service id="nullable_serializer" parent="jms_serializer.serializer">
    <call method="setSerializeNull">
        <argument>true</argument>
    </call>
</service>
fos_rest:
  service:
    serializer: nullable_serializer

@fernandopg
Copy link

I used the fix that @ruudk said (creating my own serializer) and worked properly, but I update throw composer today and stopped working:

Fatal error: Call to undefined method JMS\Serializer\Serializer::setSerializeNull()

This is because the setSerializeNull function isn't more in JMS\Serializer\Serializer...

Any solution?

Thanks a lot!!

@stof
Copy link
Contributor

stof commented Feb 26, 2013

Please read the upgrade file. in 0.12-dev, it is now in the SerializationContext

@fernandopg
Copy link

Thanks @stof, I saw the changes, now you can pass a Context as third parameter in the serialize call, but I want to set the serialize null by configuration because I use FOSRestBundle with the ViewResponseListener.

Really I don't know how to do this now...

Any help will be appreciated!

@AAtticus
Copy link

AAtticus commented Apr 8, 2013

Any news on this?
I'd like the serialization of JSON to also include empty values of existing keys because Knockout.js won't work with the returned JSON data.

@artworkad
Copy link

@fernandopg same problem for me

@ankitkhedekar
Copy link

@artworkad and @fernandopg even im facing the same problem. Did you'll find a way to get it working with FOSRestBundle?

@artworkad
Copy link

@ankitkhedekar Instead of just checking jObject.field != null, where field is some json object attribute. I have implemented a routine that checks if the field exists. Could avoid that if jmsserializer would output null values. This issued definitely should be solved.

@AAtticus
Copy link

AAtticus commented Jun 3, 2013

@fernandopg Did you find a solution? We're also using FOSRestBundle and it's incredibly frustrating that we can't serialize NULL values with their keys.

@fernandopg
Copy link

No.... @AAtticus @ankitkhedekar @artworkad :-(

Finally that I do is create the Views in the controller:

$view = View::create()->setStatusCode(200)->setData($data)->setSerializationContext(SerializationContext::create()->setSerializeNull(true));

I hope this help us!

@hglattergotz
Copy link

@fernandopg Thanks! I looked at the source in FOSRest for this issue and it looks like it will be a bit of an effort to refactor to make it honor the config. With your solution one could add a check for the serialize_null setting in the config and take the appropriate action.

@lsmith77
Copy link
Contributor

There is a setting on FOSRestBundle for serializing null values, but its about serializing a null value in the data of the View, not about configuring the serializer to enable serializing nulls within an object graph. We might need to adjust this setting to allow separate configuration of these two things.

see FriendsOfSymfony/FOSRestBundle#459

@hglattergotz
Copy link

@lsmith77 Thanks for the clarification. I actually never realized that there is a distinction between the two until you mentioned it. From what I read here and on similar issues I think what most people are looking for is the ability to serialize the data as it is and not have it drop keys with null values - the version where the SerializationContext is configured to serialize null values).
I know this is a feature of the serializer, but it would seem much more intuitive to have to explicitly turn OFF null value serialization than the other way around.
I was rather surprised to find my api data "incomplete" when I first got it working.

@AAtticus
Copy link

@hglattergotz Exactly my opinion on this. I find it rather strange that keys with NULL values will be dropped by default. One of the constraints of our API is that it's HATEOAS. We use a front-end system where the client traverses through our API by resource links but they can't discover the attributes of the resource when they are NULL and omitted from the json or xml response.

@lsmith77
Copy link
Contributor

I dont think its a good idea for FOSRestBundle to have a different default, furthermore you are seeing mostly comments from people that want NULLs serialized, because the others already get what they want and therefore do not need to post tickets.

Anyway, hopefully someone can work on the referenced FOSRestBundle ticket and make it possible to at least configure the behavior globally. That being said, any global option like this can lead to incompatible Bundle code (ie. one bundle wanted the setting set to off the other to on).

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.

None yet