fix: do not dup text siblings in reparent_node_with(xmlAddChild)

Related to #283 and #595 Closes #2916
sparklemotion · Jul 3, 2023 · 3b1bc8e · 3b1bc8e
1 parent a2c0073
commit 3b1bc8e
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@ Nokogiri follows [Semantic Versioning](https://semver.org/), please see the [REA
 
 ### Fixed
 
+* [CRuby] Replacing a node's children via methods like `Node#inner_html=`, `#children=`, and `#replace` no longer defensively dups the node's next sibling if it is a Text node. This behavior was originally adopted to work around libxml2's memory management (see [#283](https://github.com/sparklemotion/nokogiri/issues/283) and [#595](https://github.com/sparklemotion/nokogiri/issues/595)) but should not have included operations involving `xmlAddChild()`. [[#2916](https://github.com/sparklemotion/nokogiri/issues/2916)]
 * [JRuby] Fixed NPE when serializing an unparented HTML node. [[#2559](https://github.com/sparklemotion/nokogiri/issues/2559), [#2895](https://github.com/sparklemotion/nokogiri/issues/2895)] (Thanks, [@cbasguti](https://github.com/cbasguti)!)
 
 

diff --git a/ext/nokogiri/xml_node.c b/ext/nokogiri/xml_node.c
@@ -350,7 +350,7 @@ reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_reparentee_func
 
   xmlUnlinkNode(original_reparentee);
 
-  if (prf != xmlAddPrevSibling && prf != xmlAddNextSibling
+  if (prf != xmlAddPrevSibling && prf != xmlAddNextSibling && prf != xmlAddChild
       && reparentee->type == XML_TEXT_NODE && pivot->next && pivot->next->type == XML_TEXT_NODE) {
     /*
      *  libxml merges text nodes in a right-to-left fashion, meaning that if

diff --git a/test/xml/test_node_reparenting.rb b/test/xml/test_node_reparenting.rb
@@ -673,7 +673,9 @@ def coerce(data)
             it "should not merge text nodes during the operation" do
               xml = Nokogiri::XML(%(<root>text node</root>))
               replacee = xml.root.children.first
+
               replacee.replace("new text node")
+
               assert_equal "new text node", xml.root.children.first.content
             end
           end
@@ -682,7 +684,9 @@ def coerce(data)
             doc = Nokogiri::XML(%{<parent><child>text})
             replacee = doc.at_css("child")
             replacer = doc.create_comment("<b>text</b>")
+
             replacee.replace(replacer)
+
             assert_equal 1, doc.root.children.length
             assert_equal replacer, doc.root.children.first
           end
@@ -691,11 +695,26 @@ def coerce(data)
             doc = Nokogiri::XML(%{<parent><child>text})
             replacee = doc.at_css("child")
             replacer = doc.create_cdata("<b>text</b>")
+
             replacee.replace(replacer)
+
             assert_equal 1, doc.root.children.length
             assert_equal replacer, doc.root.children.first
           end
 
+          it "replacing a child should not dup sibling text nodes" do
+            # https://github.com/sparklemotion/nokogiri/issues/2916
+            xml = "<root><parent>asdf</parent>qwer</root>"
+            doc = Nokogiri::XML.parse(xml)
+            nodes = doc.root.children
+            parent = nodes.first
+            sibling = parent.next
+
+            parent.inner_html = "foo"
+
+            assert_same(sibling, parent.next)
+          end
+
           describe "when a document has a default namespace" do
             before do
               @fruits = Nokogiri::XML(<<~eoxml)