Skip to content

Commit

Permalink
fix(jruby): XML::DocumentFragment.dup to another document (#3372)
Browse files Browse the repository at this point in the history
**What problem is this PR intended to solve?**

Back in b92660e (#1834 fixing #1063) I omitted support in JRuby for the
"new_parent_document" argument to `Node#dup` because there was no
performance reason to implement it. So the test was skipped.

However, in 1e7d38a and other commits in #3117 (fixing #316), I
introduced a call to `initialize_copy_with_args` that passes the new
parent document as an argument on both CRuby and JRuby implementations.
Because the test was skipped, I didn't catch that this broke on JRuby.

In particular this was a problem for Loofah which relies on decorators,
and even more particularly this broke the `Loofah::TextBehavior`
formatting concern for `Loofah::*::DocumentFragment` objects.


**Have you included adequate test coverage?**

The skipped test is no longer skipped!

Maybe we should be running downstream tests with JRuby, too? But that
feels like a big investment right now so I'll avoid scarring on the
first cut, and wait to see if it happens again.


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

Brings the Java impl into agreement with the C impl.
  • Loading branch information
flavorjones authored Dec 12, 2024
2 parents 42cd756 + dda0be2 commit 769a0cc
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 3 deletions.
3 changes: 2 additions & 1 deletion ext/java/nokogiri/XmlNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -978,10 +978,11 @@ public class XmlNode extends RubyObject

@JRubyMethod(visibility = Visibility.PROTECTED)
public IRubyObject
initialize_copy_with_args(ThreadContext context, IRubyObject other, IRubyObject level, IRubyObject _ignored)
initialize_copy_with_args(ThreadContext context, IRubyObject other, IRubyObject level, IRubyObject document)
{
boolean deep = level instanceof RubyInteger && RubyFixnum.fix2int(level) != 0;
this.node = asXmlNode(context, other).node.cloneNode(deep);
setDocument(context, (XmlDocument)document);
return this;
}

Expand Down
2 changes: 0 additions & 2 deletions test/xml/test_node.rb
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,6 @@ def test_dup_same_parent_document_is_default
end

def test_dup_different_parent_document
skip_unless_libxml2("Node.dup with new_parent arg is only implemented on CRuby")

doc1 = XML::Document.parse("<root><div><p>hello</p></div></root>")
doc2 = XML::Document.parse("<div></div>")

Expand Down

0 comments on commit 769a0cc

Please sign in to comment.