Skip to content

Conversation

@NathanFreeman
Copy link
Contributor

@cmb69
Copy link
Member

cmb69 commented Jul 28, 2022

I think the problem with Travis CI is that it tries to test against "master", although the target branch is PHP-8.0.

@NathanFreeman
Copy link
Contributor Author

NathanFreeman commented Jul 28, 2022

I choose wrong target branch to merge at the begining.
I close wrong pr and make a new one.
But Travis CI didn't run again.

@NathanFreeman
Copy link
Contributor Author

cc @beberlei

@beberlei
Copy link
Contributor

Sorry i wont have time to review this PR. Many tests are good, so cautiosly optimisic about this

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! It seems to me this approach is going in the right direction, but is not yet complete. Besides my comment below, DOMNode::insertBefore() has the same issue, and maybe other methods, too.

ext/dom/node.c Outdated
}

if (new_child == NULL) {
if (!child->ns && nodep->nsDef) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether this shouldn't be:

Suggested change
if (!child->ns && nodep->nsDef) {
if (!child->ns && nodep->ns) {

That would solve:

$dom = new \DOMDocument();
$dom
  ->appendChild($dom->createElementNS('some:namespace', 'foo'))
  ->appendChild($dom->createElement('bar'))
  ->appendChild($dom->createElement('baz'));
echo ($xml = $dom->saveXML());

$xpath = new \DOMXPath($dom);
$xpath->registerNamespace('n', 'some:namespace');
echo count($xpath->query('/n:foo/bar')) . " should be 0\n";
echo count($xpath->query('/n:foo/n:bar')) . " should be 1\n";
echo count($xpath->query('/n:foo/n:bar/baz')) . " should be 0\n";
echo count($xpath->query('/n:foo/n:bar/n:baz')) . " should be 1\n\n";

Copy link
Contributor Author

@NathanFreeman NathanFreeman Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodep->ns is not a nullptr but point to a invalid address if nodep is the root node.

@NathanFreeman NathanFreeman requested a review from cmb69 August 12, 2022 16:19
@kamil-tekiela kamil-tekiela changed the title Fix bug #81468 Fix bug #81468: Inconsistent default namespace inheritance Aug 16, 2022
@nielsdos
Copy link
Member

nielsdos commented Dec 6, 2024

This is already fixed in a better way in the new DOM classes. The old classes retain the old behaviour.
The fix in this PR is also wrong btw: adding an element should not inherit the namespace of the parent, namespaces can't change after the element's creation according to spec.

@nielsdos nielsdos closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants