Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:104687
MIME-Version: 1.0
Date: Tue, 12 Mar 2019 23:54:13 +0100
Message-ID: <CAEPJdu=BuS8rf7_=GikUtCArdHSUD4AqY3+27tAJ1HPCP4Jm4A@mail.gmail.com>
To: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000001b8d330583ed92f0"
Subject: On fixing DOMNameSpaceNode and DOM NS API Inconsistency Problems
From: kontakt@beberlei.de (Benjamin Eberlei)

--0000000000001b8d330583ed92f0
Content-Type: text/plain; charset="UTF-8"

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

--0000000000001b8d330583ed92f0--