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

Dom namespacenode improvement #6939

Closed
wants to merge 1 commit into from

Conversation

veewee
Copy link

@veewee veewee commented May 4, 2021

This PR makes DOMNamespaceNode extend from DOMNode.
This allows for easier data access without having to worry about all possible types all the times:

<hello xmlns="http://www.hello.com" xmlns:foo="http://foo">
    <item xmlns:bar="http://bar">A</item>
</hello>

Given following example,
The result can either be DOMAttr|DOMNamespaceNode|false:

$xmlns = $hello->getAttributeNode('xmlns');
$foo = $hello->getAttributeNode('xmlns:foo');

(Since the xmlns attributes are very hidden away in the dom extension, you'll need to directly fetch it based on exact attribute name.)

Meaning that if you want to use the result of a the getAttributeNode(),
you might always have to keep in mind the possibility of it being a DOMNameSpaceNode:

function doSomethingWithAttribute(DOMAttr | DOMNamespaceNode $node)
{
    // do something
}

If you are dealing with only one function, this might be OK.
But if you were to build a library around DOM, this becomes quite messy and it works clumsy.

A better solution is to - at least - make DOMNamespaceNode extends DOMNode.
This way you can at least use the base type:

function doSomethingWithAttribute(DOMNode $node)
{
    return match(true) {
        $node instanceof DOMAttr => 'attr',
        $node instanceof DOMNameSpaceNode => 'xmlns attribute',
    };
}

This PR is in line with the java jaxon implementation:
http://www.cafeconleche.org/jaxen/apidocs/org/jaxen/dom/NamespaceNode.html

An alternative solution might to make it extend DOMAttr. However, I don't think this is the way to go, since you can also fetch the namespaces through xpath. In this case it would make less sense for it to be an attribute:

$namespaces = $xpath->query('./namespace::*', $node);

@nikic
Copy link
Member

nikic commented May 4, 2021

cc @beberlei

@veewee
Copy link
Author

veewee commented May 6, 2021

@beberlei : Saw your comment in #6936 (comment)

there are a bunch of thing sin ext/dom just to make libxml happy, for example DOMNameSpaceNode not being a DOMNode and others.

Can you give me an example in how libxml wouldnt be happy about this? Haven't found any erroneous cases myself yet.

Ran testcases on 2 repo's without any libxml specific problems:

(Both contain quite some namespaces, but none of them contain really big XML files)

@ramsey
Copy link
Member

ramsey commented May 7, 2021

What can you do with a DOMNameSpaceNode? It doesn't appear to have any methods or properties.

@ramsey ramsey added the Feature label May 7, 2021
@veewee
Copy link
Author

veewee commented May 7, 2021

@ramsey It contains a representation of an xmlns namespace attribute:

<hello xmlns:test="http://test" />
var_dump($doc->documentElement->getAttributeNode('xmlns:test'));
object(DOMNameSpaceNode)#90 (8) {
      ["nodeName"]=>
      string(10) "xmlns:test"
      ["nodeValue"]=>
      string(11) "http://test"
      ["nodeType"]=>
      int(18)
      ["prefix"]=>
      string(4) "test"
      ["localName"]=>
      string(4) "test"
      ["namespaceURI"]=>
      string(11) "http://test"
      ["ownerDocument"]=>
      string(22) "(object value omitted)"
      ["parentNode"]=>
      string(22) "(object value omitted)"
    }

@nikic
Copy link
Member

nikic commented Jul 21, 2021

@beberlei : Saw your comment in #6936 (comment)

there are a bunch of thing sin ext/dom just to make libxml happy, for example DOMNameSpaceNode not being a DOMNode and others.

Can you give me an example in how libxml wouldnt be happy about this? Haven't found any erroneous cases myself yet.

I'd be interested in that as well. I found this discussion on internals: https://externals.io/message/104687

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants