Use static when calling deserializationCallback()

[MarkRedeman]

This pull request changes the deserialize function of Serializable to call the deserializationCallbacks from static instead of self. This enables us to have an abstract BaseketEvent and a BasketWasPickedUp class extending it. However this does require us to make the properties of both the parent and the child classes protected.

Serializing an object which inherits from an abstract class containing the serializable trait does not work when said object has a private property becuase the get_object_vars function won't be able to find the private property.

When instead the parent class has a private property then serialization does work, but deserialization does not (since the calling class would be the child class).

I've added a few testcases that show that serializing and deserializing objects using inheritance with private properties does not work, which is why I needed so many fixtures. Maybe someone else can look into "fixing" this issue, though I'm not sure if it is worth the time investment..

[coveralls]

Coverage remained the same at 100.0% when pulling f6531350696c4406d544694c67e3757175a29430 on MarkRedeman:use-static-when-deserializing into 85a5583339280a62b72ab3a7fc10d8d2e06f56bc on matthiasnoback:master.

[MarkRedeman]

The failing test is due to #8.

[coveralls]

Coverage remained the same at 100.0% when pulling fc50846a59196953e149bf7d92e6c35d23e99671 on MarkRedeman:use-static-when-deserializing into de52b13ecdb6f8efee4bdceb913734ead003dca9 on matthiasnoback:master.

[matthiasnoback]

Just to make this clear:

• If you add the Serializable trait to the child class and all the properties of the parent class are protected, everything is fine, right?
• I think this is fine though and not something that requires a fix, but it might be something to add to the README as a "known limitation", what do you think?
• If you agree, I think we don't need the "negative" test cases, only a test case showing that my first point is true (which then proves that this is still a usable, valuable library ;))

[MarkRedeman]

Yes it will work when the parent class has protected properties, however the child itself can't have private properties see it_does_not_serialize_private_properties_of_a_child_class.

I think it is fine to have protected properties for messages. By using inheritance you could now create an abstract Event that uses the Serializable trait and implements the SerializableInterface so that your other events only have to extend the abstract Event class making them a bit less coupled to Broadway et al. So yeah it will still be a very useful and valuable library. :)

Just to be clear the only test that was fixed by changing self to static was the it_uses_the_callbacks_of_a_child_class, since that one requires a custom callback on the child class which wasn't called before.

I will add some comments in the readme regarding inheritance. I could remove the negative test cases by either rebasing or making a new commit (so that it stays in the history and perhaps when someone thinks of a way to fix it then he or she could reuse the tests), which would you prefer?

[matthiasnoback]

Ah, but can't that problem be fixed by moving the trait to the child class?

[MarkRedeman]

Ah yes if you move the trait to child then it works if the parent has no private properties. You do need to be careful though that the Serializable trait will override the callbacks of the parent, so you need write a callback function in your child class that returns or merges the callbacks of the parent.

[matthiasnoback]

@MarkRedeman What's the status of this, should it be merged?

[matthiasnoback]

Hey @MarkRedeman, are you still interested in fixing this?

[matthiasnoback]

Ping @MarkRedeman :)

[damonjones]

I'd very much like to see this merged.

[matthiasnoback]

@damonjones If I understand it correctly, this isn't a fix to be merged, but more the proof of a problem. @MarkRedeman Am I right? Or should this actually be merged? If so, could you please rebase on the master?