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.

Sign up for GitHub

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

Jump to bottom

DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on libxml2 version #11625

Closed
fluffycondor opened this issue Jul 7, 2023 · 3 comments
Closed

DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on libxml2 version #11625

fluffycondor opened this issue Jul 7, 2023 · 3 comments
Assignees
@nielsdos
Labels
Bug Extension: dom Status: Verified

Comments

@fluffycondor
Copy link

fluffycondor commented Jul 7, 2023 •
edited
Loading

Description

The following code:

<?php
$html = <<<HTML
<!DOCTYPE HTML>
<html>
<body>
    <div></div>
</body>
</html>
HTML;

$dom = new DOMDocument();
$dom->loadHTML($html);

$divs = iterator_to_array($dom->getElementsByTagName('div')->getIterator());
foreach ($divs as $div) {
    $fragment = $dom->createDocumentFragment();
    $fragment->appendXML('<p>Hi!</p>');
    $div->replaceWith($fragment);
}

echo $dom->saveHTML();

Resulted in this output:

<!DOCTYPE HTML>
<html>
<body>
    
</body>
</html>

But I expected this output instead:

<!DOCTYPE HTML>
<html>
<body>
    <p>Hi!</p>
</body>
</html>

It works as expected if I replace the line $div->replaceWith($fragment); with $div->replaceWith(...$fragment->childNodes);.

PHP Version

PHP 8.2.8

Operating System

Windows 10 x64.
@fluffycondor
Copy link
Author

fluffycondor commented Jul 7, 2023 •
edited
Loading

Wow, the resulting output differs by operating system.
My laptop with Windows 10 x64, PHP 8.2.8 (cli) (built: Jul 4 2023 15:53:15) (ZTS Visual C++ 2019 x64):

<!DOCTYPE HTML>
<html>
<body>
    
</body>
</html>

https://onlinephp.io/c/a552a with PHP 8.2.8:

<!DOCTYPE HTML>
<html><body>
    <><p>Hi!</p></>
</body></html>

This is a wrong result too, DOMDocumentFragment shouldn't produce those weird <></>.
But looks like this result also true for previous PHP versions: https://3v4l.org/LqkYK
(I never saw it in the wild though, because passing DOMDocumentFragment to replaceWith() while fiddling with some real-world html code always resulted in segfault until recent php-dom fixes in PHP 8.2.8.)

@fluffycondor fluffycondor changed the title PHP 8.2.8: DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just delete node PHP 8.2.8: DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on the OS Jul 7, 2023
@nielsdos
Copy link
Member

nielsdos commented Jul 7, 2023

I wonder if the output difference is because of a different libxml2 version. The Windows builds use an older version...

So on first sight it looks like two things are going on:

  • The new code depends on a behaviour of libxml2 only true in recent versions.
  • There's a bug going back to even PHP 8.0 that is yet to be fixed.

I'll investigate.

@nielsdos nielsdos self-assigned this Jul 7, 2023
@nielsdos
Copy link
Member

nielsdos commented Jul 7, 2023 •
edited
Loading

Actually, this isn't a regression at all in PHP 8.2.8, and this does not depend on the OS. I can reproduce this on old PHP 8.1.x versions with a recent libxml2. This only depends on the libxml2 version. Looks like it's simply not defined what happens when you add a document fragment. So depending on the version you'll get nothing, or <>...</>. The fix seems to be simple though: unwrap fragments in dom_zvals_to_fragment.

EDIT: interestingly, having 2 divs caused a segfault previously and leaks now, lovely.

@nielsdos nielsdos changed the title PHP 8.2.8: DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on the OS DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on libxml2 version Jul 7, 2023
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 7, 2023
@nielsdos
Fix phpGH-11625: DOMElement::replaceWith() doesn't replace node with …
f397ad0 
…DOMDocumentFragment but just deletes node or causes wrapping <></> depending on libxml2 version

Depending on the libxml2 version, the behaviour is either to not
render the fragment correctly, or to wrap it inside <></>. Fix it by
unpacking fragments manually. This has the side effect that we need to
move the unlinking check in the replacement function to earlier because
the empty child list is now possible in non-error cases.
Also fixes a mistake in the linked list management.
@nielsdos nielsdos linked a pull request Jul 7, 2023 that will close this issue
Fix GH-11625: DOMElement::replaceWith() doesn't replace node with DOM… #11627
Closed
nielsdos added a commit that referenced this issue Jul 10, 2023
@nielsdos
Merge branch 'PHP-8.1' into PHP-8.2
57ff1c3 
* PHP-8.1:
  Fix GH-11630: proc_nice_basic.phpt only works at certain nice levels
  Fix GH-11629: bug77020.phpt tries to send mail
  Fix GH-11625: DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on libxml2 version
nielsdos added a commit that referenced this issue Jul 10, 2023
@nielsdos
Merge branch 'PHP-8.2'
4863d93 
* PHP-8.2:
  Fix GH-11630: proc_nice_basic.phpt only works at certain nice levels
  Fix GH-11629: bug77020.phpt tries to send mail
  Fix GH-11625: DOMElement::replaceWith() doesn't replace node with DOMDocumentFragment but just deletes node or causes wrapping <></> depending on libxml2 version
@timnolte timnolte mentioned this issue Aug 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nielsdos nielsdos

Labels
Bug Extension: dom Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix GH-11625: DOMElement::replaceWith() doesn't replace node with DOM… nielsdos/php-src
2 participants
@nielsdos @fluffycondor