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

Added clearing previous libxml errors #688

Merged
merged 1 commit into from
Mar 9, 2017

Conversation

zerkms
Copy link
Contributor

@zerkms zerkms commented Dec 7, 2016

For the empty document string libxml_* error functions return the previous error, not the error caused by last xml operation.

The simple reproduction script:

https://3v4l.org/RKEb0

$previous = libxml_use_internal_errors(true);
$doc = simplexml_load_string('foo');
libxml_use_internal_errors($previous);

$result = libxml_get_last_error();

$previous = libxml_use_internal_errors(true);
$doc = simplexml_load_string('');
libxml_use_internal_errors($previous);

$result = libxml_get_last_error();

var_dump($doc, $result);

What it means is that if there is any "previous error" it would mask the problem with libxml_get_last_error returning not what the JMSSerializer expects.

In this PR I'm sending the test (so the travis run here fails deliberately) that should have passed and that demonstrates initial problem (masked with the lack of libxml_clear_errors() call).

I additionally sent a php bug report https://bugs.php.net/bug.php?id=73670, to either fix the php documentation or libxml-related implementation.

In either case, JMSSerializer should handle it properly.

@zerkms zerkms force-pushed the DESERIALIZE_EMPTY_XML_ERROR branch from 50386a2 to da778bf Compare December 7, 2016 01:34
zerkms added a commit to zerkms/serializer that referenced this pull request Dec 7, 2016
zerkms added a commit to zerkms/serializer that referenced this pull request Dec 7, 2016
@zerkms
Copy link
Contributor Author

zerkms commented Jan 19, 2017

@goetas this PR has a failing test deliberately: the whole purpose of this PR is to demonstrate a problem.

I did not include a solution for it because for me there is no a really obvious one existing (for me).

So I created the #689 and provided a possible solution that the library maintainers need to assess.

@goetas goetas closed this in #689 Mar 9, 2017
@zerkms
Copy link
Contributor Author

zerkms commented Mar 9, 2017

@goetas I think this PR also need to be merged: it contains the unit test for the #689

@goetas goetas reopened this Mar 9, 2017
@goetas goetas merged commit da778bf into schmittjoh:master Mar 9, 2017
@zerkms
Copy link
Contributor Author

zerkms commented Mar 9, 2017

Oops, too slow :-) I did not have the whole environment set up at a new desktop at home, so could not merge it faster than you :-)

@goetas
Copy link
Collaborator

goetas commented Mar 9, 2017

commits schmittjoh serializer

something weird... was sure that this was included in the firts merge... anyway.. now looks good

@zerkms zerkms deleted the DESERIALIZE_EMPTY_XML_ERROR branch March 9, 2017 08:03
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.

2 participants