Allow Enum objects part of a type map to be serialized and deserialized#105
Allow Enum objects part of a type map to be serialized and deserialized#105agustingomes wants to merge 4 commits intoCrell:masterfrom
Conversation
| "require": { | ||
| "php": "~8.2", | ||
| "crell/attributeutils": "~1.3", | ||
| "crell/attributeutils": "dev-master#a118b55 as 1.3.1", |
There was a problem hiding this comment.
Before merging this PR (if that is the direction we want to move towards) I will make sure I update this to a released version.
There was a problem hiding this comment.
I should probably go make a new release of that soon... 😅
| use PHPUnit\Framework\Attributes\Test; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| final class TypeMapTest extends TestCase |
There was a problem hiding this comment.
Using a separate test class means you don't get any of the automatic "test on all formatters" logic. This should instead be a method in the SerdeTestCases class (with appropriate Group). Then follow the pattern in ArrayBasedFormatterTestCases to decode the data to ensure it serialized correctly. That way we get a full round-trip test on all formatters.
There was a problem hiding this comment.
It may be possible to use one of the existing data providers on the main bulk-test, although that may be too complicated. A new method is fine if so, as long as it follows the established pattern.
There was a problem hiding this comment.
Will look into applying the changes for the tests the next days.
There was a problem hiding this comment.
Sorry it took longer than I anticipated.
I took a look at the classes you mentioned, I think I got the gist of how you wanted me to organize the test additions. Let me know if what was added works for you.
PHPStan latest version (2.1.44 at the time of this commit) likely is detecting the first `match` statement throws an exception, and therefore flags the second `match` statement with a `match.alwaysTrue` rule.
This change uses the upstream commit allowing Crell#10 to be fixed. NOTE: it is expected a new release of `crell/attributeutils` to be released. using directly the hash is for the purpose of validating the implementation changes in this PR
This commit implements serialization of an enum within a Type Map.
Therefore, individual enums being passed in a list, would be serialized as:
```
{"type": "enum", "value": "the-enum-value"}
```
| $result = $s->deserialize($serialized, from: $this->format, to: TypeMappedMixedElements::class); | ||
|
|
||
| self::assertEquals($data, $result); | ||
| } |
There was a problem hiding this comment.
Is there a reason the new data provider just can't get used on the existing round-trip test function?
There was a problem hiding this comment.
The only one was to be able to put the test under the typemap group.
But adding the interface_typemap_and_enum_permutations to the round_trip test also works, if you feel is not needed to keep this test under the typemap group.
Should I just remove this test and add the new data provider to the round_trip test?
2 participants
Description
This proposed change aims to allow Enum objects part of a TypeMap to be serialized and deserialized by establishing a serialized structure expected when working with enums in this context.
Motivation and context
fixes #10
How has this been tested?
composer all-checkshelped validate the proposed changes.Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:Checklist:
Go over all the following points, and put an
xin 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.