Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Fix: Report Xml warning/error instead of silently returning a wrong xml#54346
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
9a5c7de to0fa27a9CompareVincentLanglet commentedMar 20, 2024
Others failure (especially lint) are unrelated. |
nicolas-grekas commentedMar 20, 2024
Don't we get an exception from ErrorHandler in debug mode at the moment? |
VincentLanglet commentedMar 20, 2024
Yes, but not in production. So it silently return a wrong xml in prod.
Ok for 7.1. Should I do the PR like this or with an option to passe to the encoder@nicolas-grekas ? |
nicolas-grekas commentedMar 20, 2024
No options on my side. |
Uh oh!
There was an error while loading.Please reload this page.
0fa27a9 to2af997bCompare007f56d to9516303CompareVincentLanglet commentedMar 20, 2024
Done@nicolas-grekas :) |
| $ignorePiNode =\in_array(\XML_PI_NODE,$encoderIgnoredNodeTypes,true); | ||
| if ($datainstanceof \DOMDocument) { | ||
| return$data->saveXML($ignorePiNode ?$data->documentElement :null); | ||
| set_error_handler(staticfunction ($type,$message) {thrownewNotEncodableValueException($message); }, \E_ERROR | \E_WARNING); |
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.
that doesn't work: php won't fallback on the previous handler for deprecation
instead, I suggest using the@ operator and using error_get_last to compute the exception message when needed
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.
$result = @$data->saveXML($ignorePiNode ? $data->documentElement : null);if (false === $result) { throw Exception();}doesn't cover all the case.
For instance
$dom = new DOMDocument();$root = $dom->createElement('root');$dom->appendChild($root);$text = $dom->createTextNode("Invalid character: " . chr(7));$root->appendChild($text);$saved = $dom->saveXML();// Check if saving was successfulif ($saved === false) { echo "Error saving XML\n";} else { echo "XML saved successfully\n"; echo $saved;}gives
Warning: DOMDocument::saveXML(): xmlEscapeEntities : char out of range in /home/user/scripts/code.php on line 9XML saved successfully<?xml version="1.0"?><root></root>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.
It should be better with41c83a3@nicolas-grekas
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
9c821fb to4173286Comparefabpot commentedApr 5, 2024
Thank you@VincentLanglet. |
Uh oh!
There was an error while loading.Please reload this page.
When
DomDocument::saveXMLencounter an error/warning, for examplethe method will return false or an empty/incomplete XML.
In case of
false, since symfony doesn't use strict type, it will be cast intostring(for 6.4+ version with native typehint) so theencodemethod will return an empty string.In case of empty/incomplete XML, symfony returns it as if, without any notice about the error/warning.
I think Symfony should not silently ignore such XML error when decoding.
Or should it be an option ?