Fix various namespace prefix conflict resolution bugs #11777
Conversation
028a08d to
d975b79
Compare
|
I'll try to have a look later today or tomorrow morning - unless somebody beats me to it. |
| string(18) "http://php.net/ns2" | ||
| string(18) "http://php.net/ns3" | ||
| <?xml version="1.0"?> | ||
| <container xmlns="http://php.net/ns1" xmlns:default="http://php.net/ns2" xmlns:default1="http://php.net/ns3" xmlns:default2="http://php.net/ns4" default2:hello=""/> |
There was a problem hiding this comment.
This test is confusing me, as it claims to test createAttributeNS but it mainly tests setAttributeNodeNS. Apart from that, I'm not convinced that the observed/asserted behavior and the serialization are correct:
The W3C-Spec (https://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-ElSetAtNodeNS) for setAttributeNodeNS says: "If an attribute with that local name and that namespace URI is already present in the element, it is replaced by the new one." This is not the case for your example, as the NS differes, so it should return null for all cases. As the NS is different, I'd expect all 4 attributes to be set.
I'm also confused by the serialization:
-
The
<container>-Node was created without a namespace. Now, when re-parsing this, the node would be inhttps://php.net/ns1. That seems wrong. -
There's no output of the intermediate step, but even the very first
setAttributeNodeNScall should have created a prefix as the NS clearly differs from the NS of the element node it's being attached to. -
If your implementation were correct, the replaced attribute's namespace should be dropped, because now they are redundant.
There was a problem hiding this comment.
For the record, this is what firefox does (formatting by me):
'<container xmlns="http://www.w3.org/1999/xhtml"
a0:hello="" xmlns:a0="https://php.net/ns1"
a1:hello="" xmlns:a1="https://php.net/ns2"
a2:hello="" xmlns:a2="https://php.net/ns3"
a3:hello="" xmlns:a3="https://php.net/ns4">
</container>'There was a problem hiding this comment.
First of all thanks for checking!
This test is confusing me, as it claims to test createAttributeNS but it mainly tests setAttributeNodeNS.
All subtests test the behaviour of createAttributeNS, it was there that there was an implementation bug so that's why I named the test like this. The first half tests the behaviour with setAttributeNode (no NS) and the second half setAttributeNodeNS.
The W3C-Spec (https://www.w3.org/TR/DOM-Level-2-Core/core.html) for setAttributeNodeNS says: "If an attribute with that local name and that namespace URI is already present in the element, it is replaced by the new one." This is not the case for your example, as the NS differes, so it should return null for all cases. As the NS is different, I'd expect all 4 attributes to be set.
Note that the first half tests setAttributeNode, not setAttributeNodeNS: https://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-887236154
However I need to recheck if I did the nodeName test correctly for all cases, I'll do that tomorrow.
The -Node was created without a namespace. Now, when re-parsing this, the node would be in https://php.net/ns1. That seems wrong.
Good catch! Unfortunately it looks like this has been a bug for a long time :(: https://3v4l.org/kid5o
Fixing it in this PR is appropriate, so I'll do that.
If your implementation were correct, the replaced attribute's namespace should be dropped, because now they are redundant.
I don't think this is required by spec right?
There was a problem hiding this comment.
The W3C-Spec (https://www.w3.org/TR/DOM-Level-2-Core/core.html) for setAttributeNodeNS says: "If an attribute with that local name and that namespace URI is already present in the element, it is replaced by the new one." This is not the case for your example, as the NS differes, so it should return null for all cases. As the NS is different, I'd expect all 4 attributes to be set.
Note that the first half tests setAttributeNode, not setAttributeNodeNS: https://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-887236154 However I need to recheck if I did the nodeName test correctly for all cases, I'll do that tomorrow.
I did mention that these longish tests with the expects way later are confusing ;) You're of course right, I missed the NS not being used when scrolling back. I still believe mixing DOM1 and DOM2+ APIs should just be forbidden ;) I know that's not according to the Spec though...
Then I retract my previous statement regarding the setAttributeNode behavior: The result makes sense, given the nodeName is identical and the non-NS-aware API should probably just ignore the Namespace.
Which would lead to the question, whether those xmlns-declarations should be there at all: To add a new attribute node with a qualified name and namespace URI, use the setAttributeNodeNS method. Wouldn't that imply they should be ignored?
If your implementation were correct, the replaced attribute's namespace should be dropped, because now they are redundant.
I don't think this is required by spec right?
Not as far as i can tell. Leaving things laying around is still a bit messy ;)
There was a problem hiding this comment.
I did mention that these longish tests with the expects way later are confusing ;)
Sorry, you're right. I'm going to split this test into two files, I should've done so from the start.
I still believe mixing DOM1 and DOM2+ APIs should just be forbidden ;) I know that's not according to the Spec though...
I agree, it's ugly.
Which would lead to the question, whether those xmlns-declarations should be there at all: To add a new attribute node with a qualified name and namespace URI, use the setAttributeNodeNS method. Wouldn't that imply they should be ignored?
HMM. Good question. The fact that this so vaguely worded certainly doesn't help... I'm going to check in detail tomorrow.
Not as far as i can tell. Leaving things laying around is still a bit messy ;)
True. I'm going to fix this separately in a follow-up, as this is not the only place where it gets messy. I want to get the output correct first :)
| string(18) "http://php.net/ns2" | ||
| string(18) "http://php.net/ns3" | ||
| <?xml version="1.0"?> | ||
| <container xmlns="http://php.net/ns1" xmlns:default="http://php.net/ns2" xmlns:default1="http://php.net/ns3" xmlns:default2="http://php.net/ns4" xmlns:foo="http://php.net/ns1" default2:hello=""/> |
| NULL | ||
| string(18) "http://php.net/ns1" | ||
| <?xml version="1.0"?> | ||
| <container xmlns:foo="http://php.net/ns1" foo:hello=""/> |
|
For the record: The rest looks good to me :) |
d975b79 to
23565fa
Compare
|
I've fixed the default namespace issue and split the tests into different files.
I've thought about this, and I've done a little digging into the history of this method. Up until 2015, this method used with namespaced attributes had different behaviours in different browsers. WHATWG then decided to alias setAttributeNode to setAttributeNodeNS. I can't make this change here right now because of BC issues, but I plan to address this in an upcoming effort (in PHP 8.4) for opt-in spec compliance. I've also retargeted this to master, because of the behavioural differences and I don't want to get complaints about this. If necessary, I can backport the default namespace handling without the other behaviour changes in this PR. Finally, since the attribute handling influences how namespaces can be reused, I had to remove the namespace reuse code from |
Filed #11892 as a draft PR to deal with this |
The output could still be improved by removing redundant namespace declarations, but that's another issue. At least the output is correct now.
23565fa to
650917e
Compare
…hift bugs There are two linked issues: - Conflicts couldn't be resolved by changing the prefix name. - Lacking a prefix would shift the namespace as the default namespace, causing elements to suddenly become part of the namespace instead of the attributes. The output could still be improved by removing redundant namespace declarations, but that's another issue. At least the output is correct now. Closes phpGH-11777.
The output could still be improved by removing redundant namespace
declarations, but that's another issue. At least the output is
correct now.
Depends on the fix in GH-11776, so the first commit just applies that here.
Will clean that up once that PR is merged of course.