Skip to content

Commit

Permalink
fix: do not dup text siblings in reparent_node_with(xmlAddChild) (#2918)
Browse files Browse the repository at this point in the history
**What problem is this PR intended to solve?**

Closes #2916

Related to #283 and #595

**Have you included adequate test coverage?**

Yes

**Does this change affect the behavior of either the C or the Java
implementations?**

Brings C in line with Java behavior.
  • Loading branch information
flavorjones committed Jul 3, 2023
2 parents 8798e75 + 3b1bc8e commit 1d00a22
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)!)


Expand Down
2 changes: 1 addition & 1 deletion ext/nokogiri/xml_node.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions test/xml/test_node_reparenting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand Down

0 comments on commit 1d00a22

Please sign in to comment.