bug#26533: 26.0.50; xml-parse-region's symbol-qname argument is ignored

bug#26533: 26.0.50; xml-parse-region's symbol-qname argument is ignored

[Christopher Wellons]

A bug was introduced in aea67018 that causes the special "symbol-qnames"
value for PARSE-NS to be ignored, as if it were nil. This information is
discarded by the change to xml-parse-attlist, so functions further down
the line see the argument as if it was set to nil.

Here's an example of the bug:

    (with-temp-buffer
      (insert "<root a:b='c'></root>")
      (let ((xml-default-ns ()))
        (xml-parse-region nil nil nil nil 'symbol-qnames)))

Prior to this commit (Emacs 25.1 and earlier) the result is:

    ((root ((b . "c"))))

After this commit:

    ((root ((a:b . "c"))))

This is the same as PARSE-NS being set to nil.

bug#26533: 26.0.50; xml-parse-region's symbol-qname argument is ignored

[David Engster]

Christopher Wellons writes:
> A bug was introduced in aea67018 that causes the special "symbol-qnames"
> value for PARSE-NS to be ignored, as if it were nil. This information is
> discarded by the change to xml-parse-attlist, so functions further down
> the line see the argument as if it was set to nil.
>
> Here's an example of the bug:
>
>     (with-temp-buffer
>       (insert "<root a:b='c'></root>")
>       (let ((xml-default-ns ()))
>         (xml-parse-region nil nil nil nil 'symbol-qnames)))
>
> Prior to this commit (Emacs 25.1 and earlier) the result is:
>
>     ((root ((b . "c"))))
>
> After this commit:
>
>     ((root ((a:b . "c"))))
>
> This is the same as PARSE-NS being set to nil.

Thanks for the report.

You are right that the fix for bug #23440 was not correct. I now pushed
a hopefully better version to master.

Note however that your test above has two problems: First, it's invalid
XML since you're using an undeclared prefix (so the parser should rather
throw an error, but I'm not eager to make the xml parser more strict, as
there's a lot of invalid XML in the wild). Second, I don't understand
why you let-bind `xml-default-ns' to nil. This will break namespace
expansion, and it will actually do this for the whole Emacs session if
xml.el gets autoloaded during the above.

-David

bug#26533: 26.0.50; xml-parse-region's symbol-qname argument is ignored

[Christopher Wellons]

Thanks, David! Your fix works fine as far as I can tell.

I'm using this trick in Elfeed (a syndication feed reader) as a fast
method to strip all namespaces from the XML as it's being parsed. As you
said, there's a lot of invalid XML in the wild. I've found it works a
lot better to ignore namespaces and strictness, instead extracting the
required information heuristically as long as it's reasonably close.
Otherwise there would be a whole lot more feeds that wouldn't work well,
or at all, in Elfeed.

I had noticed with symbol-qnames that xml-parse-region drops unknown
namespaces. Since this information comes from an alist, that seemed like
reasonable behavior and I assumed it was intentional -- though signaling
an error would also be reasonable. To tightly control which namespaces
are stripped, I bind xml-default-ns to my own alist for that call. This
feels like the natural and lispy way to use this function.

The file that binds xml-default-ns requires the xml package explicitly,
so there's no risk of it autoloading while it's bound. Though that's an
interesting consequence I hadn't considered before. I _have_ seen
similar issues with accept-process-output when arbitrary process events
are handled while the stack is in an unusual state.