-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] make XmlEncoder stateless thus reentrant #38534
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] make XmlEncoder stateless thus reentrant #38534
Conversation
6b27d66
to
39615f4
Compare
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.
can you please add a test case and rebase+target the lowest branch where this issue exists (3.4 I suppose?)
Rather than maintaining a stack of current document (which is not properly maintained in case of errors btw, as the unstacking should be done in a |
@stof I have tested the ownerDocument method. It seems to be a working solution and the tests are running but. At the appendNode method the $parentNode argument can be DomDocument so the onwerDocument is null: |
@connorhu if the parentNode is already a DomDocument, you already have the DomDocument. That's fine to me. |
39615f4
to
06ac911
Compare
If we want to make the encoder re-entrant, I think the state in |
06ac911
to
a8c344f
Compare
b58889b
to
85a02f5
Compare
I rebased the PR for branch 3.4 but there are some failure to fix, could you please have a look? |
@nicolas-grekas Of course! Thank you! |
89852c4
to
db628e6
Compare
I think my original bug is now fixed. The current version of XmlEncoder in symfony 3.4 is not so complex. I don't see how can cause the context property a bug like the dom did. |
@connorhu the issue is exactly the same that with |
@stof I see the possibility of the context bug, but If you use nothing from context after initialization it can't cause a bug. Safer to remove context property instead of making a note for the future code changes. |
@connorhu the thing is, the XmlEncoder is using the context in some of the private methods. |
@stof Should I merge the context property remove patch into in this PR? The referred bug only describes the bug about the dom property. |
db628e6
to
693f9ab
Compare
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 rebased and implemented the remaining)
Thank you @connorhu. |
Changing dom property of XmlEncoder to stackable. It fixes a DOMException "Wrong document error". When you calling XmlEncoder->encode() in parallel createDomDocument replaces the DomDocument object.
Test code:
https://github.com/connorhu/symfony-serializer/blob/master/test.php