fix: Node.new second argument is typechecked

It must be a Node, it should be a Document (and a warning will be
emitted if it is not).

Closes #975

CHANGELOG.md

@@ -58,6 +58,11 @@ This release ends support for:
 * [JRuby] `Node#line` behavior has been modified to return the line number of the node in the _final DOM structure_ (the value returned in JRuby is increased by 1 to count the XML prolog). This behavior is different from CRuby, which returns the node's position in the _input string_. This difference is not ideal, but at least is now officially documented and tested. The real-world impact of this change is that the value returned in JRuby is greater by 1 to account for the XML prolog in the output. [[#2380](https://github.com/sparklemotion/nokogiri/issues/2380)] (Thanks, [@dabdine](https://github.com/dabdine)!)
 
 
+### Deprecated
+
+* Passing a `Nokogiri::XML::Node` as the second parameter to `Node.new` is deprecated and will generate a warning. This will become an error in a future version of Nokogiri. [[#975](https://github.com/sparklemotion/nokogiri/issues/975)]
+
+
 ## 1.12.5 / 2021-09-27
 
 ### Security

ext/java/nokogiri/XmlNode.java

@@ -305,6 +305,14 @@ public class XmlNode extends RubyObject
     IRubyObject name = args[0];
     IRubyObject doc = args[1];
 
+    if (!(doc instanceof XmlNode)) {
+      throw context.runtime.newArgumentError("document must be a Nokogiri::XML::Node");
+    }
+    if (!(doc instanceof XmlDocument)) {
+      // TODO: deprecate allowing Node
+      context.runtime.getWarnings().warn("Passing a Node as the second parameter to Node.new is deprecated. Please pass a Document instead, or prefer an alternative constructor like Node#add_child. This will become an error in a future release of Nokogiri.");
+    }
+
     Document document = asXmlNode(context, doc).getOwnerDocument();
     if (document == null) {
       throw context.runtime.newArgumentError("node must have owner document");

ext/nokogiri/xml_node.c

@@ -1765,24 +1765,31 @@ rb_xml_node_line_set(VALUE rb_node, VALUE rb_line_number)
 static VALUE
 rb_xml_node_new(int argc, VALUE *argv, VALUE klass)
 {
-  xmlDocPtr doc;
-  xmlNodePtr node;
-  VALUE name;
-  VALUE document;
+  xmlNodePtr c_document_node;
+  xmlNodePtr c_node;
+  VALUE rb_name;
+  VALUE rb_document_node;
   VALUE rest;
   VALUE rb_node;
 
-  rb_scan_args(argc, argv, "2*", &name, &document, &rest);
+  rb_scan_args(argc, argv, "2*", &rb_name, &rb_document_node, &rest);
 
-  Data_Get_Struct(document, xmlDoc, doc);
+  if (!rb_obj_is_kind_of(rb_document_node, cNokogiriXmlNode)) {
+    rb_raise(rb_eArgError, "document must be a Nokogiri::XML::Node");
+  }
+  if (!rb_obj_is_kind_of(rb_document_node, cNokogiriXmlDocument)) {
+    // TODO: deprecate allowing Node
+    rb_warn("Passing a Node as the second parameter to Node.new is deprecated. Please pass a Document instead, or prefer an alternative constructor like Node#add_child. This will become an error in a future release of Nokogiri.");
+  }
+  Data_Get_Struct(rb_document_node, xmlNode, c_document_node);
 
-  node = xmlNewNode(NULL, (xmlChar *)StringValueCStr(name));
-  node->doc = doc->doc;
-  noko_xml_document_pin_node(node);
+  c_node = xmlNewNode(NULL, (xmlChar *)StringValueCStr(rb_name));
+  c_node->doc = c_document_node->doc;
+  noko_xml_document_pin_node(c_node);
 
   rb_node = noko_xml_node_wrap(
               klass == cNokogiriXmlNode ? (VALUE)NULL : klass,
-              node
+              c_node
             );
   rb_obj_call_init(rb_node, argc, argv);
 

lib/nokogiri/html5.rb

@@ -254,6 +254,7 @@ def self.fragment(string, encoding = nil, **options)
     #  * :follow_limit => number of redirects which are followed
     #  * :basic_auth => [username, password]
     def self.get(uri, options = {})
+      # TODO: deprecate
       warn("Nokogiri::HTML5.get is deprecated and will be removed in a future version of Nokogiri.",
         uplevel: 1, category: :deprecated)
       get_impl(uri, options)

lib/nokogiri/xml/node.rb

@@ -119,7 +119,7 @@ class Node
       #
       # [Parameters]
       # - +name+ (String)
-      # - +document+ (Nokogiri::XML::Document)
+      # - +document+ (Nokogiri::XML::Document) The document to which the the returned node will belong.
       # [Yields] Nokogiri::XML::Node
       # [Returns] Nokogiri::XML::Node
       #

test/xml/test_node.rb

@@ -726,6 +726,16 @@ def test_xml_node_new_with_block
           assert_equal(node, block_param, "Node.new block should be passed the new node")
         end
 
+        def test_xml_node_new_must_take_document_type
+          assert_raises(ArgumentError) do
+            Nokogiri::XML::Node.new("input", "not-a-document")
+          end
+
+          assert_output(nil, /deprecated/) do
+            Nokogiri::XML::Node.new("input", xml.root.children.first)
+          end
+        end
+
         def test_to_str
           name = xml.xpath("//name").first
           assert_match(/Margaret/, "" + name)

test/xml/test_unparented_node.rb

@@ -91,7 +91,7 @@ def test_each
       end
 
       def test_new
-        assert(node = Nokogiri::XML::Node.new("input", @node))
+        assert(node = Nokogiri::XML::Node.new("input", @node.document))
         assert_equal(1, node.node_type)
       end
 
@@ -423,7 +423,7 @@ def test_encode_special_chars
       end
 
       def test_content
-        node = Nokogiri::XML::Node.new("form", @node)
+        node = Nokogiri::XML::Node.new("form", @node.document)
         assert_equal("", node.content)
 
         node.content = "hello world!"
@@ -445,7 +445,7 @@ def test_replace
         first = set[0]
         second = set[1]
 
-        node = Nokogiri::XML::Node.new("form", @node)
+        node = Nokogiri::XML::Node.new("form", @node.document)
         first.replace(node)
 
         assert(set = @node.search(".//employee"))