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

Allow to use custom serializer with primitive type #222

Closed
wants to merge 2 commits into from

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Jan 28, 2014

This patch will allow to create custom handles for primitive types (strings for example)

(i use it make translations during serialization)

@goetas
Copy link
Collaborator Author

goetas commented Feb 6, 2014

ping....

@cmodijk
Copy link

cmodijk commented Feb 20, 2014

I wanted to use this to create a custom type to transform the string of a uploaded file to a full url with VichUploaderBundle.

@goetas
Copy link
Collaborator Author

goetas commented Feb 20, 2014

I'm using this feature to translate some properties "on-the-fly" (based on accept language) during serialization .

@schmittjoh
Copy link
Owner

Please use a custom type in that case instead of decorating built-in types.

@schmittjoh schmittjoh closed this Feb 20, 2014
@goetas
Copy link
Collaborator Author

goetas commented Feb 20, 2014

What are custom types?

(sorry for my ignorance..)

@schmittjoh
Copy link
Owner

Something like @Type("translatable_string")

@goetas
Copy link
Collaborator Author

goetas commented Feb 20, 2014

I have already tried to adopt this solution, but GraphNavigator::accept() will try to visit a translatable_string property, supposing that it is an object.

Here https://github.com/schmittjoh/serializer/blob/master/src/JMS/Serializer/GraphNavigator.php#L143 GraphNavigator will try to call isVisiting, and it will raise an exception...

@cmodijk
Copy link

cmodijk commented Feb 20, 2014

I have tried to register a custom handler and i'm getting the exception

Expected object but got string. Do you have the wrong @Type mapping or could this be a Doctrine many-to-many relation?"

@goetas
Copy link
Collaborator Author

goetas commented Feb 20, 2014

@cmodijk i have explained the situation just now

@stof
Copy link
Contributor

stof commented Feb 20, 2014

@schmittjoh custom types cannot be used on primitive types currently. See #194

@schmittjoh
Copy link
Owner

Ah, I see.

I'm afraid, then you'll probably need to overwrite the visitors or make the changes you proposed in your fork. This use-case is not common enough that I want to add more ifs to the code base and I'm also not really sure that the serializer is the right layer for these transformations (at least the current architecture was not designed for this).

@goetas
Copy link
Collaborator Author

goetas commented Feb 20, 2014

I think that this feature will be useful.
I'm also using it with array, choosing the right array element based on some context informations, and serializing it as simple property..

https://gist.github.com/goetas/9110276 (in this way i'm hiding the implementation of my service)

@funkyan
Copy link

funkyan commented Feb 20, 2014

This feature would be very useful.

JMS is powerfull but quite frustrating if we have to create similar virtual methods to handle a recurrent output serialization for a primitive type such as strings and arrays. In many situation, primitive types are best suitable and people would not enjoy to wrap it in object just to get them handled by JMS.

Next the JMS process, visiting our object graphs, is quite heavy, so that people like me choosing this library actually expect it to handle a maximum of stuff during this "visit".

Custom handlers seems the perfect hook for this, and it is completly leave to developer responsability.
Providing custom type handling is great but a bit useless if we are forced to wrap our properties in objects.

Having said this, I think it would be enough to allow people to easily extends graph navigator for this purpose, here it is final and every attribute are private... is there any way to plug a custom graph navigator without having to fully rewrite it ?

@schmittjoh
Copy link
Owner

@funkyan, what's your use case for extending primitive type handling?

@funkyan
Copy link

funkyan commented Feb 20, 2014

Some key value serialized data, stored into string fields (doctrine), that we always parse and output the same way when outputing it in json. I would prefer to handle those same custom types in one unique place.

@goetas
Copy link
Collaborator Author

goetas commented Jul 16, 2014

Hi @schmittjoh,
can you re-consider this?

I'm writing a small tool that converts an XSD into PHP classes and JMS serializer YAML metatata, that allows to serialize/unserialize complex schemas as OTA.

XMLSchema defines boolean type as true, false, and 0 or 1 as valid values.

The current implementation of jms-serializer does not allow 1 and 0, and does not allow a custom handler on boolean type to overwrite this behavior.

There are a lot of use case, where a custom handler on primitive types can be useful.

@stof Do you agree with me?

@schmittjoh
Copy link
Owner

@goetas, I'd be happy to merge support for 0 and 1.

@goetas
Copy link
Collaborator Author

goetas commented Jul 16, 2014

The same thing will happen with float... XML Schema allows also -.45E-6 as valid float... and current implementation will not be enough...

@schmittjoh
Copy link
Owner

Sure, happy to make improvements, just send individual pull requests, but I don't think that we need a general extension point here.

@RSully
Copy link

RSully commented Nov 26, 2014

Any plans to separate this into individual pull requests to get the ball rolling?

@luxifer
Copy link

luxifer commented Dec 17, 2014

My use case would be to transform simple strings (containing binary data such as image or pdf) to base 64 during the serialization and decode it during the deserialization

@holtkamp
Copy link
Contributor

+1 in combination with using the XSD2PHP functionality of @goetas.

I encountered potential UseCases when dealing with large object structures in various industries, for example:

Sometimes XML signing / base encoding, or other transformations are needed during (de)serialization, some flexibility / influence during this phase would be nice. Custom handlers seem "the obvious way to go"...

@hiend
Copy link

hiend commented Apr 7, 2015

+1

@holtkamp
Copy link
Contributor

@schmittjoh please consider reopening this, here is an example of a SimpleType that would be useful to have a Custom Handler dealing with:

https://github.com/juriansluiman/SlmIdealPayment/blob/master/data/xsd/AcceptantAcquirer.xsd#L394-L398

Ok, the type: DateTime<"Y-m-d\TH:i:s.u\Z"> can be used, but a CustomHandler would provide more flexibility.

@goetas
Copy link
Collaborator Author

goetas commented May 5, 2016

Rebased again.
@schmittjoh Please consider reopening this. There are many use cases for this feature

@niabot
Copy link

niabot commented Jul 5, 2016

Without custom primitives the validation of input data is more or less impossible. The default implementation for that types creates valid - but wrong - data before validation.

@goetas
Copy link
Collaborator Author

goetas commented Jul 9, 2016

@schmittjoh

This use-case is not common enough that I want to add more ifs to the code base and I'm also not really sure that the serializer is the right layer for these transformations (at least the current architecture was not designed for this).

please re-consider this, is already a long time that some people (a lot) are requesting this feature. some example are #507 goetas-webservices/xsd2php-runtime#1 AuthorizeNet/sdk-php#146 AuthorizeNet/sdk-php#73 and probably many other.

the feature is BC and does not introduce a high amount of logic

@aramalipoor
Copy link

👍

@schmittjoh
Copy link
Owner

@goetas it's not so much about the amount of code required, but about the performance impact. Most of the types we handle are primitive types, and moving them through the event system/handler system will severely impact performance. Did you benchmark this?

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

@schmittjoh
The approach used in this PR has no real performance impact. The reason is that is still not possible to register an event handler for a type string, so no event managers are triggered.

To use a custom mapping on this PR is necessary to declare a custom type. Example:

class Author {
    /**
     * @Type("MyNonExistentClass")
     * @var string
     */
    private $name; // is a string
}

The type of $name is string, but @Type annotation declares something different (in this case MyNonExistentClass). This allows to register an handler for MyNonExistentClass and to process it.

This has no general performance impact, and the serialization of the type string is not affected.
To be even more clear, will still be not possible to register custom handlers for @Type("string") declarations.

I have also tried a general approach in goetas#3, allowing to have custom handlers for @Type("string") definitions, and that created a 20% penality (using tests/benchmark.php benchmark).
This PR is way simpler, and there are not real performance changes.

@schmittjoh
Copy link
Owner

Ah sorry, I was looking at that other PR yesterday before reading the comments here.

Could you fix the coding-style (indentation) so we can merge this. I also cannot re-open this PR because GitHub says there is already one open. Did you create another one?

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

Ok, will rebase and create a new one!

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

#610 rebased version of this

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

🎉

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

@schmittjoh thank you!

@holtkamp
Copy link
Contributor

Great, @goetas this means https://github.com/goetas/xsd2php does not rely on a patched version of the Serializer anymore?

@goetas
Copy link
Collaborator Author

goetas commented Jul 25, 2016

not yet :( there were few more commits that were not yet published as PR here... working on it. One of them is #611 , but probably two more are coming

@holtkamp
Copy link
Contributor

Aah, ok, well, kudos for your perseverance 😄

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.