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

CDATA in MarkupParser #528

Open
mbeckerle opened this issue May 21, 2021 · 3 comments
Open

CDATA in MarkupParser #528

mbeckerle opened this issue May 21, 2021 · 3 comments

Comments

@mbeckerle
Copy link

I've had to override this method of MarkupParser due to what I think is a bug:

def xCharData: NodeSeq = {
      xToken("[CDATA[")
      def mkResult(pos: Int, s: String): NodeSeq = {
         handle.text(pos, s) // NOTE: Handle the text
         PCData(s)             // Ignores the result of handling, and creates a PCData with the original string!
      }
      xTakeUntil(mkResult, () => pos, "]]>")
}

I put comments in there to illustrate what I think is the issue.

I think this is the fix:

  override def xCharData: NodeSeq = {
    xToken("[CDATA[")
    def mkResult(pos: Int, s: String): NodeSeq = {
       cdata(pos, s)
    }
    xTakeUntil(mkResult, () => pos, "]]>")
  }

  // This method below  gets a prototype on MarkupHandler and is overridden here 
  // with a default implementation that creates a PCData node.

  override def cdata(pos: Int, s: String): NodeSeq = {
    PCData(s) // by default, just create PCData with the string.
  }

I am not sure whether that method should be named cdata or pcdata.

If this makes sense, I can create a PR for this. But I wanted to run it by you first.

@SethTisue
Copy link
Member

does @dubinsky's #558 fix this?

@ashawley
Copy link
Member

ashawley commented Sep 9, 2021

I believe #558 only fixes when the Java Xerces parser reads XML.

I believe MarkupParser is part of the Scala-based ConstructingParser.

@ashawley
Copy link
Member

ashawley commented Sep 9, 2021

I see no reason to not have both XML facilities in the library support CDATA going forward, so a PR would be welcomed.

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

No branches or pull requests

3 participants