Hi, Only adding Rob just in case :)
best, On Wed, Mar 13, 2019, 5:54 AM Benjamin Eberlei <kont...@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 >
