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] 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Serializer] make XmlEncoder stateless thus reentrant#38534
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6b27d66 to39615f4Compare
nicolas-grekas 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.
can you please add a test case and rebase+target the lowest branch where this issue exists (3.4 I suppose?)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
stof commentedOct 13, 2020
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 |
connorhu commentedOct 13, 2020
@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: |
stof commentedOct 13, 2020
@connorhu if the parentNode is already a DomDocument, you already have the DomDocument. That's fine to me. |
39615f4 to06ac911Comparestof commentedOct 13, 2020
If we want to make the encoder re-entrant, I think the state in |
06ac911 toa8c344fCompareb58889b to85a02f5Comparenicolas-grekas commentedOct 14, 2020
I rebased the PR for branch 3.4 but there are some failure to fix, could you please have a look? |
connorhu commentedOct 14, 2020
@nicolas-grekas Of course! Thank you! |
89852c4 todb628e6Compareconnorhu commentedOct 14, 2020
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. |
stof commentedOct 15, 2020
@connorhu the issue is exactly the same that with |
connorhu commentedOct 15, 2020
@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. |
stof commentedOct 15, 2020
@connorhu the thing is, the XmlEncoderis using the context in some of the private methods. |
connorhu commentedOct 16, 2020
@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 to693f9abCompare
nicolas-grekas 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.
(I rebased and implemented the remaining)
nicolas-grekas commentedJan 31, 2022
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