Skip to content
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

[Serializer] Fix: Report Xml warning/error instead of silently returning a wrong xml #54346

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Mar 20, 2024

Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #...
License MIT

When DomDocument::saveXML encounter an error/warning, for example

DOMDocument::saveXML(): xmlEscapeEntities : char out of range

the method will return false or an empty/incomplete XML.

In case of false, since symfony doesn't use strict type, it will be cast into string (for 6.4+ version with native typehint) so the encode method 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 ?

@VincentLanglet
Copy link
Contributor Author

Others failure (especially lint) are unrelated.
I don't touch them to minize the conflict with up-merge.

@nicolas-grekas
Copy link
Member

Don't we get an exception from ErrorHandler in debug mode at the moment?
Let's do this on 7.1 so that we're confident we won't break any existing apps?

@VincentLanglet
Copy link
Contributor Author

Don't we get an exception from ErrorHandler in debug mode at the moment?

Yes, but not in production. So it silently return a wrong xml in prod.

Let's do this on 7.1 so that we're confident we won't break any existing apps?

Ok for 7.1. Should I do the PR like this or with an option to passe to the encoder @nicolas-grekas ?

@nicolas-grekas
Copy link
Member

No options on my side.

@VincentLanglet
Copy link
Contributor Author

Let's do this on 7.1 so that we're confident we won't break any existing apps?

Done @nicolas-grekas :)

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 7.1 Mar 21, 2024
@@ -82,7 +82,12 @@ public function encode(mixed $data, string $format, array $context = []): string
$encoderIgnoredNodeTypes = $context[self::ENCODER_IGNORED_NODE_TYPES] ?? $this->defaultContext[self::ENCODER_IGNORED_NODE_TYPES];
$ignorePiNode = \in_array(\XML_PI_NODE, $encoderIgnoredNodeTypes, true);
if ($data instanceof \DOMDocument) {
return $data->saveXML($ignorePiNode ? $data->documentElement : null);
set_error_handler(static function ($type, $message) { throw new NotEncodableValueException($message); }, \E_ERROR | \E_WARNING);
Copy link
Member

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

Copy link
Contributor Author

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 successful
if ($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 9
XML saved successfully
<?xml version="1.0"?>
<root></root>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be better with 41c83a3 @nicolas-grekas

@fabpot
Copy link
Member

fabpot commented Apr 5, 2024

Thank you @VincentLanglet.

@fabpot fabpot merged commit fdedd3b into symfony:7.1 Apr 5, 2024
6 of 9 checks passed
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants