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

Fix "]]>" serialization in text nodes #207

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Holzhaus
Copy link

This fixes issue #164, which is a regression caused by commit 47fa9b8.
Before, text nodes were serialized correctly (thanks to commit 22fff92).

Section 2.4 of the XML 1.0 (5th Ed) recommendation states:

The right angle bracket (>) may be represented using the string
">", and MUST, for compatibility, be escaped using either ">"
or a character reference when it appears in the string "]]>" in
content, when that string is not marking the end of a CDATA section.

See https://www.w3.org/TR/2008/REC-xml-20081126/#syntax for details.

Thus, this commit escapes the right angle bracket in text nodes if it
appears as part of "]]>". If not, the right angle bracket is not
escaped.

The unittest that was broken since commit 47fa9b8 has been fixed, too.

This fixes issue jindw#164, which is a regression caused by commit 47fa9b8.
Before, text nodes were serialized correctly (thanks to commit 22fff92).

Section 2.4 of the XML 1.0 (5th Ed) recommendation states:

    The right angle bracket (>) may be represented using the string
    ">", and MUST, for compatibility, be escaped using either ">"
    or a character reference when it appears in the string "]]>" in
    content, when that string is not marking the end of a CDATA section.

See https://www.w3.org/TR/2008/REC-xml-20081126/#syntax for details.

Thus, this commit escapes the right angle bracket in text nodes if it
appears as part of "]]>". If not, the right angle bracket is not
escaped.

The unittest that was broken since commit 47fa9b8 has been fixed, too.
Copy link

@smoke smoke left a comment

Choose a reason for hiding this comment

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

I highly support that!!!

@smoke
Copy link

smoke commented Nov 8, 2017

@Holzhaus you might consider incorporating #132 for fullness of the PR.

@santiagoaguiar
Copy link

Also hit this issue, given this seems to be stuck here, we'll need to fork and port. Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants