Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ErrorHandler] Serialize FlattenException#38800
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| publicfunctionsetTrace($trace,$file =null,$line =null):self | ||
| { | ||
| $this->trace = []; | ||
| if ($file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see that this is WIP :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had to leave it after 2 hours that late last night.
I would like to get some feedback from@yceruto and/or someone that knows about property access.
Also, is it a good idea to modify a class "just because" serialization?
| * @return $this | ||
| */ | ||
| publicfunctionsetTrace($trace,$file,$line):self | ||
| publicfunctionsetTrace($trace,$file =null,$line =null):self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn't this a BC break for inheriting classes? Any other idea?
| /** @var \ReflectionParameter $parameter */ | ||
| $parameter =$method->getParameters()[0]; | ||
| if (\array_key_exists('value',$context) &&null ===$context['value'] && !$parameter->allowsNull()) { | ||
| $errors[] =sprintf('The method "%s" in class "%s" was found but does not allow null.',$methodName,$class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This breaksPropertyAccess test suite (see Travis's failure)
7e170c5 to9686df0Compare
yceruto left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey! As far as I understand, thisFlattenException should be handled bySymfony\Component\Messenger\Transport\Serialization\Normalizer\FlattenExceptionNormalizer andSymfony\Component\Messenger\Transport\Serialization\Serializer when the Messenger component is installed.
I did some tests using this code and it works without any other change:
useSymfony\Component\Messenger\Transport\Serialization\Normalizer\FlattenExceptionNormalizer;useSymfony\Component\Messenger\Transport\Serialization\SerializerasMessengerSerializer;// ...publicfunctiontestSerialize(){$serializer =newSerializer([newFlattenExceptionNormalizer()], [newJsonEncoder()]);$context = [MessengerSerializer::MESSENGER_SERIALIZATION_CONTEXT =>true];$flattened = FlattenException::createFromThrowable(new \LogicException('Bad things happened'));$trace =$flattened->getTraceAsString();$data =$serializer->serialize($flattened,'json',$context);$restored =$serializer->deserialize($data, FlattenException::class,'json',$context);$restoredTrace =$restored->getTraceAsString();$this->assertSame(substr($trace,0,100),substr($restoredTrace,0,100));$this->assertSame(\count(explode(\PHP_EOL,$trace)),\count(explode(\PHP_EOL,$restoredTrace)));}
The matter is that thisFlattenExceptionNormalizer doesn't exist in 5.1, but since 5.2#37087, and the related issue is talking about this error on 5.1, but also it happens in 4.3#32719
By the way, I can't reproduce the issue on 5.2:
Exception:==========(no data)Maybe I'm missing something, but it seemsAddErrorDetailsStampListener is not registered by default, hence theErrorDetailsStamp is not added.
Nyholm commentedNov 1, 2020
Thank you for the explanation. So we are closing this as "wontfix" for 4.4?
That is true. I though that was intentional... but Im not sure. |
yceruto commentedNov 1, 2020
Well, it's possible to fix it if we use
Ok it's being fixed here#38941. |
Nyholm commentedNov 1, 2020
Im happy with closing this issue then. |
The
FlatternExceptionis used with messenger. When it is serialized with the Serializer component we fail to restore all its properties.When
FlatternException::$traceAsStringis null and we callFlatternException::getTraceAsString()we get an exception thrown in our face. That is what#38792 is about.This PR does two things (sorry about that).
It modifies
FlatternException::setTrace()so the serializer can populate the trace property when unserializing. I also try to rebuild the traceAsString from the$traceIt modifies the PropertyAccess and PropertyInfo component to be aware of nullable setters. Ie, It finds
FlatternException::setPrevious(), but that function does not allow nullable parameters, but it still try to set the valuenulland we get another exception thrown in our face.