search

Crell / Serde

Robust Serde (serialization/deserialization) library for PHP 8.
Other
299 stars 14 forks source link

Update TypeField to be inheritable and transitive #57

Closed jboctor closed 6 months ago

jboctor commented 6 months ago

Description

This updates the TypeField interface to be transitive to allow for classes to set their own TypeFields.

Motivation and context

This frees from having to set a field on every class for properties we want to serialize a certain way. Instead we can set the field on the class and have the properties transitively use that field for serialization.

How has this been tested?

Please describe in detail how you tested your changes.

Had a serializable event with an entity that was denoted with the EntityField type which was a custom TypeField. Putting that field on the property's class did not allow it to be serializable but after making the current changes, it serialized it just fine. All unit tests pass both personally and in serde.

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests and code style pass.

If you're unsure about any of these, don't hesitate to ask. We're here to help!

jboctor commented 6 months ago

Sorry for not adding tests @Crell I did could not fiend where the TypeField was being tested or how to extend those tests. If you could point me in the right direction, I'm happy to add some.

Crell commented 6 months ago

This makes sense to me, thanks. For testing, right now all field types are tested as part of the importer/exporter that leverages them. TypeFields are basically just extra arguments to an Importer/Exporter. So you'll likely need to make some fake classes for testing (the tests already have dozens, that's fine), and a very small fake importer/exporter (probably just an anon class in the test method that verifies the field is there and has what you expect before passing on to some other default, like a parent class).

The real logic here is all in AttributeUtils, so there's not actually much to test in Serde. It's really just "yep, transitivity works on this attribute, kthxbye." So it's really just 1-2 tests, I think.

jboctor commented 6 months ago

@Crell correct me if im mistaken, but it looks like this may not be as simple as expected. It seems to me that the ObjectImporter or some other class makes some assumptions as to what the TypeField should be. Namely that it should be exclusively on properties. I have some tests that do not seem to be behaving as expected and after digging into them, they are unable to find an expected "type" because there is not type for it. The error that I'm getting when attempting to test the Importer is Crell\Serde\InvalidArrayKeyType: Property transitive is marked as needing an array key of type , but invalid found. Note how it doesn't know what kind type it needs. Do you know if there is something that I may be missing?

Crell commented 6 months ago

Odd. Can you push the code you have that's failing to this branch?

jboctor commented 6 months ago

Done.

Crell commented 6 months ago

The problem with the test was it was testing the wrong thing. :smile: It was testing both that transitive fields work, and that you can reduce an object to a primitive when serializing. The former worked fine once I fixed the tests to be consistent with the other tests. The second appears to have a validation issue. I am investigating that now.

Crell commented 6 months ago

Nope, the code was fine. The fake importer was just buggy. :smile: Here's a working test case that shows it can be done. Obviously in a real implementation the importer would be calling out to a DB repository or whatever.