Fixed #353 - skip custom handler to default (de)serialization process

[TheCelavi]

Following this issue: schmittjoh/JMSSerializerBundle#353

I have stumbled upon the fact that it is impossible to fall down to default process of serialization/deserialization in our custom handler.

Use case is very simple, in one context I want to use my own handler, in other, I would like to use a default one. This is mostly related to usage of Doctrine Entities when it comes to relations, as well as file uploads.

We want to be able to use both approaches, without inventing fictive types.

Proposed solution is quite easy - if we decide in our custom handler that we would like to continue by using default handling of serialization/deserialization, we just throw 'JMS\Serializer\Exception\UnsupportedFormatException' and GraphNavigator will continue its process.

This is improvement of current functionality and does not introduces any BC breaks.

[goetas]

see #464 (comment)

[TheCelavi]

@goetas Your explanation of closing this PR lacks of any kind of explanation nor call for action.

You are referencing to a PR which is closed, from November 2013! PR is closed not because feature is bad idea, but because of changes in meantime. Author is obviously abandoned contribution.

Now - if I abandon this PR, again, this feature will be abandoned even though it has a valid use-case as well as idea how to implement it.

Now, lets be productive and not to bury this, again -> what is the real reason for rejecting the PR and how we can pass that? Is that a just a missing tests an there for if I provide you with tests as well, this can be merged?

Is there anything else that is wrong and related to this topic?

Thank you

[TheCelavi]

Yeah - see now that proposed solution is to introduce some kind of fictive, imaginary type.

This is somewhat unnatural approach, forcing us to introduce additional mapping for objects, it forces us to add additional mappings for same types, under different name depending on use-cases, increasing our code base and complicates maintainability.

Per example, if some Object (Entity) is modified - gets new attribute, per example, we have to modify all our mappings, the "real" one, as well as fictive one.

@schmittjoh explained this approach as better since interrupting with exception decrease performances.

For one -> I will always accept less code vs faster code. Interrupting with exception was the easiest and simplest Non-BC solution.

However, there are different approaches as well. Per example, Handler can return some kind object which is instance of "SkipHandling" (per example) that can denote that handler will not handle serialization/deserialization -> if performances are only issue.

I am very willing to push this implementation - @schmittjoh, @goetas is there any possibility for you to reconsider this topic?

Give me instructions and I will do the coding -> but rest assure, introducing new type and more configurations just to "skip handler" is just wrong, increases code base exponentially in order to achieve simple "continue" instruction.

Thanks.

[goetas]

You are referencing to a PR which is closed, from November 2013! PR is closed not because feature is bad idea, but because of changes in meantime. Author is obviously abandoned contribution.

i have referenced directly a comment where i have suggested an alternative approach to the issue.

Now, lets be productive and not to bury this, again -> what is the real reason for rejecting the PR and how we can pass that? Is that a just a missing tests an there for if I provide you with tests as well, this can be merged?

Currently I think that this feature can be achieved without modifying code of the serializer.

A first approach is:

// i want to serialize this class 
class Class1
{
  // @type("SomeSkipableType")
  // @var Class2
  protected $skipablePropertyType; 
}

// i want to serialize this class with a custom handler, 
// but sometimes i want to fallback to the standard behavior
class Class2
{
  protected $name; 
}

class SomeSkipableTypeHanlder implements SubscribingHandlerInterface
{
    public static function getSubscribingMethods()
    {
            return [array(
                    'type' => 'SomeSkipableType',
                    'format' => 'json',
                    'direction' => GraphNavigator::DIRECTION_SERIALIZATION,
                    'method' => 'serializeMe',
                )];
     }
     public function serializeMe(VisitorInterface $visitor, $object, array $type, Context $context)
     {
            if (something) {
                 // custom serialization rules
            } else{
                // skip this handler and fallback to the standard one
                $type['name'] = get_class($object);
                return $visitor->getNavigator()->accept($data, $type, $context);
            }
     }
}

This should work. Notice that SomeSkipableType class does not exist, it is just a trick to have a custom handler on it. (@var and @type are different)
Then if (something) gives true you can use your custom rule to serialize the object, otherwise it will fallback to the standard behavior. With this trick you achieved to skip the SomeSkipableTypeHanlder

[goetas]

The main reason for rejecting this PR is because the same behavior can be achieved with user code, without adding new features to the core.

[TheCelavi]

I found a workaround too, applicable in my use case as well.

Sorry, I have to disagree with you - If something can be done in one way, that does not mean that it should be done as such. http://www.psychicdonut.com/wp-content/uploads/2014/06/common-sense-just-because-you-can-doesn-t-mean-you-should.jpg

I still believe that simple, elegant "continue" is less hacky approach in comparison to introduction of "tricks" in code. Imagine someone who is reviewing that "hacky" code and it is not to much familiar with library constraints...

Thank you for your time, I am not going to pursue this anymore, I have provided you with all the reasoning and arguments behind it, and you have made your decision.

[asimonf]

I have to agree with @TheCelavi here. The proposed solution by @goetas is not maintainable. Anyone not familiar with the original intention of the implementation will not understand at first sight (or second for that matter) the intention of said hack. It simply obscures details. This functionality should be implemented at the core.

[goetas]

@asimonf this issue originally started with schmittjoh/JMSSerializerBundle#353
For that specific question, i have already answered and the solution is reality simple.

and if we want to cite

http://www.psychicdonut.com/wp-content/uploads/2014/06/common-sense-just-because-you-can-doesn-t-mean-you-should.jpg

If the "skip" handler is the only solution for your problem, maybe you are solving the wrong problem.

I still lack to see a real world problem (not an example built ad-hoc to use this "feature") that has not a possible solution without this feature.

[TheCelavi]

@goetas "I still lack to see a real world problem" -> just because you fail to see it, that does not mean that it does not exists. I have provided you with a real-world example in my first, initial post that explains a motivation of creating a PR (file upload via base 64 decoded string where I need different serialization/deserialization logic).

Following this conversation, as well as conversation here: schmittjoh/JMSSerializerBundle#353 it can be concluded that there are interested parties in this feature/approach who do have success in spotting a real-world application as well.

Now, there is approach within this PR, which is elegant, clear, simple to implement and maintain, without introducing BC break, and there is your proposal - to introduce additional class with fictive type on which we are complaining because we believe that it is unclear, ambiguous, hard to maintain and make us do additional/unwanted work. And to be honest - I am still failing to see how did you come to the decision to reject PR considering cons vs. pro.

In general - you are forcing us to hack with presumption that we all are doing something wrong and we do not understand the context of our problem well:

If the "skip" handler is the only solution for your problem, maybe you are solving the wrong problem.

However, your discriminatory right is to decline PR, and unfortunately we can not do anything more about it. Or maybe hire you to solve our problem in "right way" :)

[asimonf]

@goetas I think it's easy to imagine a situation in which you want to handle a particular property of a class with custom logic, but wish to continue using the default behavior for all others. Since there are no per-property handlers, the only way would be to use a custom handler that handles the whole class. However, as it is right now, I would have to either resort to hacks, or simply write a handler that handles all the properties. Neither of those solutions are ideal. Hacks lead to obfuscated code (code whose purpose is not immediately obvious to another programmer), which in turn, leads to hard to maintain code. The custom handler for all properties is not ideal because I would lose the benefit of using builtin functionality, meaning that my custom code will be less tested and I will not gain in functionality/security/reliability whenever the library is updated.

You might argue that if there's a problem with the format that needs specific custom handling, then maybe I should redesign the format. I'd argue that you might have a point, however, not everyone gets to choose the structure of the serialized content.

It is your prerogative to determine code gets in and which code gets left out, I'm not going to argue that. You're ultimately free to do what you think is best. I believe, though, that this particular use-case is currently not covered by the library and the solution you proposed is ill-conceived for production software intended to be maintainable, as opposed to what is being suggested in this PR. I honestly see no reason to reject the PR outright.

[goetas]

@asimonf Thanks for taking the time to elaborate your answer. I agree with you that the code of the serializer should be flexible enough to handle a variety of cases without hacks... and this is also my goal in maintaining this library.

One of the reasons why I'm not sure about merging this PR is because it tastes as an "if condition to solve in a perfect way a very specific use case".
Solving specific problems with specific solutions rarely gives a good result.

What I would like to find is a "feature" to add into the serializer (possibly not in the core) that solves this problem and many others. What I see here is people asking me to merge something to solve a specific use-case, without challenging them-self in trying to find a second or third way to solve the the problem, and later trying to analyze which approach is the best.

And to answer to some more detailed part of your message:

think it's easy to imagine a situation in which you want to handle a particular property of a class with custom logic, but wish to continue using the default behavior for all others. Since there are no per-property handlers, the only way would be to use a custom handler that handles the whole class.

This is a situation that can be solved with one of this approaches:

• @VirtualProperty per method
• @VirtualProperty per class with expression language that will be introduced with Expression language based virtual properties #708
• post_serialize listener adding some data with addData
• post_serialize listener adding some data with $navigator->accept(new StaticPropertyMetadata(...),...)

Some of this solutions might look hacky for some specific problem while some might look the right approach.

The custom handler for all properties is not ideal because I would lose the benefit of using builtin functionality, meaning that my custom code will be less tested and I will not gain in functionality/security/reliability whenever the library is updated.

Yes, this solution might look not optimal, but since the problem exposed at the beginning of this pull request is not well defined, no tests and no rea-use-case, this means that probably any proposed solution might look not optimal. Here is as having the cure for a symptom,
without knowing the illness.

It is your prerogative to determine code gets in and which code gets left out, I'm not going to argue that. You're ultimately free to do what you think is best. I believe, though, that this particular use-case is currently not covered by the library and the solution you proposed is ill-conceived for production software intended to be maintainable, as opposed to what is being suggested in this PR. I honestly see no reason to reject the PR outright.

Currently this PR adds just an "if" in the core class of the library. This "feature" has almost a single propose, I will be happy to hear some other options before merging this (plus some tests).

At the end, what I'm trying to challenge and to understand:

Is a custom handler with a skip feature, the only solution for a set of problems?

[asimonf]

@goetas Thanks for the detailed answer. In my particular case I'm trying to deserialize data from an XML response to a service I don't control. This particular property is best represented by a hashmap and XmlKeyValuePair works ok for it if I wanted to serialize it from an object to XML again, but deserialization doesn't work because it isn't implemented with XmlKeyValuePair. So, at present, I have to handle deserialization on my own. Having a way to handle serialization or deserialization of a specific property would be ideal, but at present the only way I can handle it is with a custom handler for the whole object.

To be honest, I haven't looked for another solution within the library aside from using a custom handler. I reached this particular issue while searching why deserialization for XmlKeyValuePair wasn't working. I, however, decided to comment because I didn't agree with the proposed solution instead of going this way.

I agree with you that a discussion for better ways to solve this is warranted, and with that in mind I'd like to say that, while your solution is hackish, I like the approach it takes to solving it. The only thing that bothers me is that it specifies a fake type for the purpose of setting up a listener. Maybe, we could just be able to specify a custom handler for a property in a more direct way through another annotation that does the same thing underneath?

[goetas]

Regarding your specific use case, a solution I saw often working is a virtual property in a different serialization group (with #708 will be even easier) .

class User
{
    /**
     * @Type("integer")
     * @Group({"de_serialization"})
     */
    protected $id;
    /**
     * @Type("string")
     * @Group({"de_serialization", "serialization", "Default"})
     */
    protected $name;
    /**
     * @Type("string")
     * @Group({"serialization"})
     */
   public function getId()
   {
       return $this->id;
   }
}

Going back to the original topic:

The only thing that bothers me is that it specifies a fake type for the purpose of setting up a listener.

Fake types are handled as ordinary types... in the current system, a type (fake or not) is just a string, there is no magic.

Types should always be provided using the @Type annotation, if not provided, the serializer does its best to find a corresponding string value. This often means just calling gettype or get_class (see here).

If that string (string, boolean, DateTime, MyFancyClass, MyFakeClass) has a handler, it is invoked, if has not, than the serializer tries its best with some default behaviors defined in the visitors.

In my projects, i tend to specify always manually the type, using as example: @Type("Float<precision, round_model>").
Float is a fake type, and allows to override the default ("poor" ?) behavior adding whatever you need for any type.

Types are just strings, and they do not have to match class names.

[goetas]

Maybe, we could just be able to specify a custom handler for a property in a more direct way through another annotation that does the same thing underneath?

Using a fake type is exactly doing it

[asimonf]

@goetas I understand, but I guess what I mean is that it's not immediately obvious that a fake type is there for that specific purpose. It feels hackish and, for newcomers, it could be a source of confusion.

[TheCelavi]

I didn't had time to dedicate some time to this and to rethink harder about all of this sooner, so here it is:

This issue (and all related issues) and proposes solutions lacks of several critical parts in order to be handled properly:

• A reasonable, valid, intelligent and fact based argue: although we have provided a reasoning why fake types are too hectic and unintuitive approach; that is unclear and hard to maintain as well as it give us a difficulty to introduce a new team member in project maintenance without additional effort/education, we have received two counter arguments: that we are "probably" doing something wrong and there is a "probably" a better approach, for solving our problem that requires changes in library core; and second one that there is a "fake type system" that can be used and should be adopted as such.

In order to claim that we are "doing something wrong" means that person who claims that "fully understands our context and problem" that we are trying to solve. However, I have not spotted an effort from anyone here to extract all the details about my/our problem(s) and context in order to draw such conclusion - so forgive my bad French here, but claiming that is extremely arrogant, bias and it is in the borderline with stupidity.

• Dogmatic approach to type system and lack of imagination: this library is based on node visitor pattern, where for each serializing/deserializing node a handler is chosen from a map of handlers. So, in general, "skipping" handler and fallback to default one violates a core implementation. However, there is a missed opportunity here, for which we all (including me) are missing to spot - implementation of "chain of responsibility" pattern for type handlers. Namely -> not only that we should be able to fall back to default handler, but we should be able to register several handlers for same type in prioritized list - why not? So, when some type should be serialized/deserialized, a type handler should be asked if he can "support" operation based on context and other state variables in the system...

However, we can not move to that position, to ultimate improvement and flexibility of library because a decision maker has, as he claims, difficulty to see a real-life problem that we are claiming to have. For every question/proposal that we have, we got an ultimate, dogmatic, answer "use fake types".

What is frustrating here is that we are fighting tooth and nails here using reasoning, examples, arguments, imagination - but we are getting rejected without provided counter-arguments.

Per example, if we claiming that "fake types" are hectic, hackish, hard to maintain and it will introduce confusion within project, I believe that we are entitled to expect an answer which is something like: "You are wrong, it is not, because [ENTER A VALID, REASONABLE EXPLANATION HERE]". Or even, "You are right, but benefits which are [ENTER BENEFITS HERE] overweights all negative properties which you are claiming that exists."

If we are claiming that we should be able to fall back to default handler, or to have "chain of responsibility" of handlers (including a default one) registered for one type, we should be provided with response like "You are wrong because [ENTER VALID REASONABLE EXPLANATION HERE]".

Stating that we are "probably doing something wrong" because "decision maker can not imagine real-life problem" that we have; and for every question there is an ultimate, dogmatized answer: fake types - is extremely frustrating.

I really think that answer "This will not be implemented because I say so. End of story." would be less frustrating, rather then:

We: fake types are wrong
You: well, you should use fake types
(repeat until die)

From each PR that I have made in my life, regardless of its rejection or approval -> I have learned something new and valuable. Especially from rejected ones! This is the first time that a lot of time got wasted and nothing was gained. Literally - nothing!

[goetas]

After #1236 (comment), #1238 implemented this feature.