Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:104690
MIME-Version: 1.0
References: <CAEPJdu=BuS8rf7_=GikUtCArdHSUD4AqY3+27tAJ1HPCP4Jm4A@mail.gmail.com>
In-Reply-To: <CAEPJdu=BuS8rf7_=GikUtCArdHSUD4AqY3+27tAJ1HPCP4Jm4A@mail.gmail.com>
Date: Wed, 13 Mar 2019 02:28:40 +0100
Message-ID: <CAAKq5po6nJJzgLWZekEZOFjpMZLi645hSnBXi+tjQeO_x3ThcQ@mail.gmail.com>
To: Benjamin Eberlei <kontakt@beberlei.de>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] On fixing DOMNameSpaceNode and DOM NS API Inconsistency Problems
From: dzuelke@salesforce.com (David Zuelke)

I agree this should be fixed. It's pretty hilarious how this exact
case (fiddling with the xmlns prefix) is the only comment for
DOMElement::setAttributeNS:
http://php.net/manual/en/domelement.setattributens.php (although
unnecessary, as you don't have to "add namespaces" to a document, that
automatically happens internally).

My hypothesis as to why this special class even exists, and why it is
all implemented this way: whoever wrote that code didn't know about
this "hard-coded" part of the spec, or the spec wasn't even fully
finished regarding that part at that time.

For those who are curious, it's defined in Section 3 of the
"Namespaces in XML 1.1 (Second Edition)" spec:
https://www.w3.org/TR/2006/REC-xml-names11-20060816/#ns-decl

However, pay particular attention to the following note (emphasis mine):

"The prefix xmlns is used only to declare namespace bindings and is by
definition bound to the namespace name http://www.w3.org/2000/xmlns/.
It **must not be declared or undeclared**. Other prefixes must not be
bound to this namespace name, and it must not be declared as the
default namespace. Element names must not have the prefix xmlns."

The DOM Level 2 specification section 1.1.8 ("XML Namespaces")
contains another related definition:

"Note: In the DOM, all namespace declaration attributes are by
definition bound to the namespace URI:
"http://www.w3.org/2000/xmlns/". These are the attributes whose
namespace prefix or qualified name is "xmlns". Although, at the time
of writing, this is not part of the XML Namespaces specification
[Namespaces], it is planned to be incorporated in a future revision."

This means that both "xmlns:foo" and "xmlns" attributes are both in
this "hard-coded" namespace.

Both of these cases are already handled correctly by, I assume, the
underlying libxml; if you set "xmlns:blah" or "xmlns" qualified name
attributes in that namespace:

$doc = new DOMDocument();
$root = new DOMElement("root");
$doc->appendChild($root);
$root->setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns:foo", "urn:bar");
$root->setAttributeNS("http://www.w3.org/2000/xmlns/", "xmlns", "urn:baz");
$root->setAttributeNS("urn:lol", "erp:foo", "whatup");
echo $doc->saveXML();

the namespace "xmlns" does not get declared, and PHP/libxml correctly produce:

<?xml version="1.0"?>
<root xmlns:foo="urn:bar" xmlns="urn:baz" xmlns:erp="urn:lol" erp:foo="whatup"/>

Honestly, I can't imagine there being much code around that even
relies on any of the internal classes, at least not code that deserves
to be broken, because it was written by clowns who to this day would
claim that XML is terrible, when in fact they simply didn't understand
how namespaces work, that prefixes are irrelevant, and that XML
shouldn't be parsed with regular expressions ;)

There is really no use case in the DOM that ever requires anyone to
directly deal with "xmlns" attributes, as their creation (and removal)
is something that just happens automatically; the only real case where
you even have to worry about prefixes is with
DOMElement::setAttributeNS(), where you can cause prefix collisions.
But even that is trivial; internally, the mappings always get
optimized, e.g. in the case of duplicate prefixes for the same
namespace URI:

$doc = new DOMDocument();
$root = new DOMElement("root");
$doc->appendChild($root);
$root->setAttributeNS("urn:lol", "prefixone:foo", "whatup");
$root->setAttributeNS("urn:lol", "prefixtwo:bar", "whatup");
echo $doc->saveXML();

becomes, correctly:

<?xml version="1.0"?>
<root xmlns:prefixone="urn:lol" prefixone:foo="whatup" prefixone:bar="whatup"/>

Anyway, I think an RFC and maybe some test cases would be good. I'll
be happy to help dig out my old XML-fu and help :)

David

On Tue, Mar 12, 2019 at 11:54 PM Benjamin Eberlei <kontakt@beberlei.de> wrote:
>
> Hi everyone,
>
> While looking for things to work on in php-src my friend Thomas pointed me
> to a peculiar special case in ext/dom that leads to massive inconsistency
> problems in the API.
>
> There is an undocumented class DOMNameSpaceNode that gets returned from
> DOMElement::getAttributeNode(NS) if you select an attribute that represents
> a namespace (xmlns). This special case is intentionally handled in the
> code, contrary to Pythons DOM Extension which doesn't do this and contrary
> to the DOM Specification, which does not have a special DOMNameSpaceNode.
> Its all DOMAttr.
>
> Problematically DOMNameSpaceNode doesn't extend from DOMAttr, you cannot
> pass this class to DOMElement::removeAttributeNode(DOMAttr $attr):
>
>     Fatal error: Uncaught TypeError: Argument 1 passed to
> DOMElement::removeAttributeNode() must be an instance of DOMAttr, instance
> of DOMNameSpaceNode given
>
> Code example here: https://3v4l.org/jkC5s
>
> In addition the DOMNameSpaceNode renames all properties compared to
> DOMAttr, clearly violating the interface documented here
> http://php.net/manual/de/domelement.getattributenode.php
>
> Two potential fixes come to mind:
>
> 1. Have DOMNameSpaceNode extend DOMAttr - maybe this was the originally
> intended behavior, becuase the properties are all named differently?
> 2. Have DOMElement::removeAttributeNode accept also a DOMNameSpaceNode
> (3.) Remove the non standard DOMNameSpaceNode class and use a DOMAttr
> instead
>
> I think approach #1 is the right one from a BC perspective and also fixing
> the DOM API to be more consistent with the standard.
>
> But I am also not opposed to #3 in the longer term, looking at Github
> DOMNamespaceNode in the open source world is only used for one apc test, in
> PHP Compat and when dumping it in Symfony Var Dumper. I imagine its more
> deeply used in closed source code working deeply with XML.
>
> The second problem that this inconsistency creates is
> DOMElement::removeAttributeNS($namespaceUri, $localName). When you want to
> use it to remove a namespace attribute (xmlns). By default the xmlns
> namespace has the uri http://www.w3.org/2000/xmlns/. Hence removing
> xmlns:$name namespace would expected to be:
>
> DOMElement::removeAttributeNS("http://www.w3.org/2000/xmlns/", "foo") //
> removes xmlns:foo
>
> But thats not how it works, there is special code implemented that requires
> you to pick the URI from xmlns:foo value:
>
> <root xmlns:foo="urn:foo" />
>
> DOMElement::removeAttributeNS("urn:foo", "foo");
>
> But if your node has an attribute in this namespace with the same name, it
> gets removed as well:
>
> <root xmlns:foo="urn:foo" foo:foo="bar" />
>
> Would incorrectly become this, removing 2 attributes:
>
> <root />
>
> This is a bug that cannot be fixed in a BC way. I have no clue how to fix
> this completly broken behavior in a good way. I propose to break BC here
> and requiring "http://www.w3.org/2000/xmlns/" as namespaceURI from PHP 8.0
> to delete a namespace (xmlns) attribute.
>
> Should I put both issues into an RFC or are these rather bugs that can be
> fixed without an RFC?
>
> greetings
> Benjamin