Skip to content

Added support for custom handlers for primitive types. #507

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

Closed
wants to merge 17 commits into from

Conversation

TiS
Copy link

@TiS TiS commented Oct 14, 2015

This PR allows developers to register custom handlers for primitive types.
This is also the work-around for issue #337.

Why: Sometimes primitive values require custom handling during serialization / deserialization. I have mentioned one of such cases in #337 - big integers, which are not parsed by javascript correctly and should be serialized to strings. Other case may be encoding HTML during serialization.

I have written a test for this PR to ensure, that everything works correctly.

@holtkamp
Copy link
Contributor

Similar to #222 ?

@TiS
Copy link
Author

TiS commented Oct 14, 2015

Well, yes, it is similar. What is different is that it allows ONLY primitives with actually attached custom handlers to be executed. I was careful to modify existing behaviour as little as possible to maintain compatibility.

@TiS
Copy link
Author

TiS commented Oct 20, 2015

Any more comments? Can this be merged (if not - why)?

@fstr
Copy link

fstr commented Nov 2, 2015

I would absolutely appreciate if this PR was merged. The current state of creating a custom type that is basically just a class that wraps a scalar value is very cumbersome.

I don't agree as stated in #222 that it is not a common use case wanting to have custom handlers for primitive types. By default all the primitive type handlers do a typecast, which causes default PHP type casting behaviour to be applied to the input JSON/XML data.

This default behaviour is very bad for a validation that might follow the deserialization process.

If I define @type("float") and someone provides a string "hello" in his JSON to my endpoint, it's deserialized and casted to a (double) 0.
Same goes for @type("boolean") and people providing values like "false" (which is casted to true) and so on. This default behaviour of the type casting prevents the subsequent validation to find any errors in the input JSON.

An additional downside is, that other bundles might read the Type annotation. For example NelmioApiDocBundle. This way the API docs get very confusing, because they list that some custom type is required, but actually it's just a bool, float, int and so on.

See also:
schmittjoh/JMSSerializerBundle#455
schmittjoh/JMSSerializerBundle#385

@itinance
Copy link

itinance commented Nov 2, 2015

@kr1x
Copy link

kr1x commented Nov 2, 2015

+1 @fstr

@TiS
Copy link
Author

TiS commented Nov 2, 2015

@fstr Just to clarify. This PR does not meet your requirements (unfortunately). It still requires, that custom serializer type is defined for each value, which requires custom handling (eg. you can define Integer type for your integer values with custom handling). It just allows to define custom types (and handlers) for primitive types, which was impossible earlier (serializer detected that value is a scalar and threw error).

Full custom handler support for primitive types will be much better than that, this is also some workaround without destroying current functionality.

@TiS
Copy link
Author

TiS commented Nov 2, 2015

The problem with custom handlers for primitive types is that they can create huge unwanted side effects, as primitives are for sure used by other bundles providing serialization. I am myself not sure, if allowing full custom handler support for primitive types is advisable. That's why I opted for custom type definition instead.

@fstr
Copy link

fstr commented Nov 2, 2015

@TiS you are right. It doesn't meet my requirements exactly.

I don't see anything wrong with giving the developer the possibility to overwrite behavior for primitive type handling though. Especially if the default behavior is already performing the unwanted side effects I am not able to prevent (in an elegant way).

@TiS
Copy link
Author

TiS commented Apr 28, 2016

Just a question - how do you see this pr @schmittjoh ?

@goetas
Copy link
Collaborator

goetas commented May 6, 2016

yes it is similar to #222
there are also a lot of changes that are similar to #578 :)

@goetas
Copy link
Collaborator

goetas commented May 6, 2016

@TiS Can you rebate this?

@aramalipoor
Copy link

👍

@goetas
Copy link
Collaborator

goetas commented Aug 3, 2016

Already implemented with #610

@goetas goetas closed this Aug 3, 2016
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

7 participants