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

encoding/xml: encoder.EncodeToken fails when given a xml.ProcInst with target "xml" #7380

Closed
gopherbot opened this Issue Feb 21, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@gopherbot
Copy link

gopherbot commented Feb 21, 2014

by martin.n.angers:

When using:

package main

import (
    "bytes"
    "encoding/xml"
    "fmt"
)

func main() {
    var in, out bytes.Buffer
    in.WriteString(`<?xml version="1.0" encoding="UTF-8"?>
<root>
</root>   
`)
    dec := xml.NewDecoder(&in)
    enc := xml.NewEncoder(&out)
    for t, err := dec.Token(); err == nil; t, err = dec.Token() {
        fmt.Printf("%#v\n", t)
        err = enc.EncodeToken(t)
        if err != nil {
            fmt.Println("ERROR:", err)
            break
        }
    }
}

(runnable example: http://play.golang.org/p/AdKaliuV4w)

It prints the error "xml: EncodeToken of ProcInst with invalid Target"

And in EncodeToken, there's this check:

    case ProcInst:
        if t.Target == "xml" || !isNameString(t.Target) {
            return fmt.Errorf("xml: EncodeToken of ProcInst with invalid Target")
        }

Am I missing something, or should it be t.Target != "xml" instead?

This is the xml encoder, and it refuses to encode if the target is "xml". I
can easily work around this by writing the xml.Header constant manually to the
destination, but that looks suspicious and I thought I should bring it up.

Thanks,
Martin
@davecheney

This comment has been minimized.

Copy link
Contributor

davecheney commented Feb 24, 2014

Comment 1:

Thanks for the repo, this looks pretty straight forward.

Labels changed: added release-go1.3, repo-main.

Status changed to Accepted.

@jasdel

This comment has been minimized.

Copy link
Contributor

jasdel commented Feb 25, 2014

Comment 2:

Posted a CR for this https://golang.org/cl/68280044/
@jasdel

This comment has been minimized.

Copy link
Contributor

jasdel commented Mar 5, 2014

Comment 3:

The CR I posted solves the problem incorrectly.  ProcInst encoding correctly should
still not allow xml targets, but the xml.Decoder should not parse the XML declaration
header as a ProcInst.
Maybe a new XML declaration header token type is needed? I think all that is needs to
change is if the first token looks like a ProcInst with a target of xml return the new
XML declaration type instead.
I don't think the xml.Encoder should process the xml declaration at all though, because
encoding/xml doesn't support non utf-8 encodings.  But it may be useful to accept the
new xml declaration token so that a user could set the standalone property. But i'm not
familiar enough with the standalone property to have a good opinion on it.
Thoughts?
@jasdel

This comment has been minimized.

Copy link
Contributor

jasdel commented Mar 7, 2014

Comment 4:

I realized an additional declaration type wasn't strictly needed, and EncodeToken just
needed to be updated to allow the xml declaration to be the first token written. This
change maintains the existing restriction of ProcInst with a Target of xml not being
allowed to be encoded after the first token.
CL: https://golang.org/cl/72410043/
@gopherbot

This comment has been minimized.

Copy link
Author

gopherbot commented Apr 9, 2014

Comment 5:

CL https://golang.org/cl/68280044 references this issue.
@gopherbot

This comment has been minimized.

Copy link
Author

gopherbot commented Apr 9, 2014

Comment 6:

CL https://golang.org/cl/72410043 references this issue.
@gopherbot

This comment has been minimized.

Copy link
Author

gopherbot commented Apr 9, 2014

Comment 7:

CL https://golang.org/cl/68280044 references this issue.
@rsc

This comment has been minimized.

Copy link
Contributor

rsc commented May 13, 2014

Comment 8:

This issue was closed by revision 92440fb.

Status changed to Fixed.

@rsc rsc added this to the Go1.3 milestone Apr 14, 2015

@rsc rsc removed the release-go1.3 label Apr 14, 2015

@golang golang locked and limited conversation to collaborators Jun 25, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.