-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
Type error in UnionHandler
#1566
Comments
I can confirm this issue. Unfortunately, it breaks existing code. For the time being, we had to pin jms/serializer to v3.30.0 because of this. It might be a good idea to release a new version reverting these changes until the feature supports arrays. (Or, as I see the issue with that, to specify a workaround for disabling the new feature.) #1552 isn’t working on this, I think. The MR is about “unions” (not really) in arrays, not arrays in unions. |
@scyzoryck would it make sense to revert the union types thing that is causing this error? from my understanding, #1546 is the guilty one, right? |
We also I think have 2 MRs on top of #1546 so it might need some additional reverts. As an example: #1563
Soo, personally I would go with fixing the issues, and going with the fix release this week with support for missing types. |
👍 |
@dmaicher could you test newest |
@scyzoryck with my example code mentioned in the issue description I get
because in my case Maybe private function testPrimitive(mixed $data, string $type, string $format): bool
{
if ($type === 'array') {
return is_array($data);
}
if (!is_scalar($data)) {
return false;
}
switch ($type) {
case 'integer':
case 'int':
return (string) (int) $data === (string) $data;
case 'double':
case 'float':
return (string) (float) $data === (string) $data;
case 'bool':
case 'boolean':
return (string) (bool) $data === (string) $data;
case 'string':
return is_string($data);
}
return false;
} Then the warning is gone in my case. Another issue I noticed: If I change my property to |
Sooo, it looks like we need a bit more development on Unions. I would propose the next steps:
@goetas does it sounds good for you?
Yes, I think you are right. I think we have "type recognition" logic to determine the correct type - soo "666" is parsed as 666 during both - serialization and deserialization at the moment. So it seems to be similar issue as #1573 #1575 - FYI @fabianerni IMO we should have the following logic for the type choosing: Serialization:
Deserialization:
|
@scyzoryck adding It would be good to revert schmittjoh/JMSSerializerBundle#954 as well |
Steps required to reproduce the problem
Expected Result
Actual Result
The text was updated successfully, but these errors were encountered: