Skip to content

Conversation

@NathanFreeman
Copy link
Contributor

See the example.

$dom = new DOMDocument();
$root = $dom->createElement('html');
$dom->appendChild($root);
$el1 = $dom->createElement('p1');
$el1->setAttribute('id', 'foo');
$el1->setIdAttribute('id', true);
$root->appendChild($el1);

$root->removeChild($dom->getElementById('foo'));
var_dump($dom->getElementById('foo'));  // it is not null.

I have removed the $dom->getElementById('foo') from $root, but the output of var_dump($dom->getElementById('foo')) is not null.

About the bug https://bugs.php.net/bug.php?id=79701
I found it difficult to fix this bug after I read the source code of libxml2 and the easiest way to fix it is to throw the exception if the duplicate id is found.
But it will lead to a BC.

@NathanFreeman
Copy link
Contributor Author

cc @beberlei

@beberlei
Copy link
Contributor

Cool!

Test might be more robust when rendering back to xml rather than based on many properties that are not relevant.

@NathanFreeman
Copy link
Contributor Author

Cool!

Test might be more robust when rendering back to xml rather than based on many properties that are not relevant.

I have just updated my test.

@NathanFreeman
Copy link
Contributor Author

Can merge it in to PHP-8.0? @beberlei @nikic @cmb69 Thanks.

@cmb69
Copy link
Member

cmb69 commented Oct 4, 2022

Thank you for the PR! However, it doesn't look like the correct fix. First, if the ID attribute isn't the first property, it will not be removed; we would need to traverse all child->properties until child->properties->next == NULL. And, while I have not checked it, I assume the same issue happens when the node is removed by other means than DOMNode::removeChild(). Worse, what happens when a parent node of the node with the ID is removed? I'm afraid that has the same issue (i.e. it won't remove the ID attribute).

And finally, this wouldn't solve the closely related https://bugs.php.net/79701. I wonder whether this is actually a libxml2 issue, and should be fixed upstream.

@NathanFreeman
Copy link
Contributor Author

NathanFreeman commented Oct 4, 2022

@cmb69 Thank you for your reply.

@nielsdos
Copy link
Member

nielsdos commented Dec 6, 2024

Fixed already in a better way.

@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