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.

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

Deprecate use of non-namespaced XPath custom functions #2147

Closed
3 of 4 tasks
flavorjones opened this issue Dec 26, 2020 · 1 comment
Closed
3 of 4 tasks

Deprecate use of non-namespaced XPath custom functions #2147

flavorjones opened this issue Dec 26, 2020 · 1 comment

Comments

@flavorjones
Copy link
Member

flavorjones commented Dec 26, 2020

With respect to custom XPath query functions, the Java implementation was forced (by the underlying Java APIs) to implement different behavior from CRuby,, namely that any customer handler functions should be namespaced with the nokogiri prefix, e.g.:

set = if Nokogiri.uses_libxml?
        @xml.search('//employee/address[my_filter(., "domestic", "Yes")]', @handler)
      else
        @xml.search('//employee/address[nokogiri:my_filter(., "domestic", "Yes")]', @handler)
      end

In commit 468c757 we tried to introduce some inference to the JRuby codebase where:

  • if a handler is provided,
  • but no namespace prefix is used in the query
  • then replace any method name in the query with a namespaced equivalent

This inference is problematic in that it led to issues like #1890, and I started planning to deprecate the use of non-namespaced custom XPath functions.

The plan, as of 2022-08-29:

@flavorjones flavorjones added this to the v1.12.0 milestone Dec 26, 2020
flavorjones added a commit that referenced this issue Dec 27, 2020
…space-inference

fix(jruby): make less-naive inference of xpath custom function ns

---

**What problem is this PR intended to solve?**

Closes #1890
Related to #2147

JRuby's logic for inferring a namespace for the custom XPath function handler has been naive since it was introduced in 468c757. This PR makes it less naive and less likely to break things by looking for function calls in the query via regex, and looking for matches among the handler's declared Ruby methods.

Ideally we should require namespaces in both JRuby and CRuby implementations at some point to avoid having to do this kind of heavy lifting. See #2147 for a proposal.

**Have you included adequate test coverage?**

Sure.


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

Only changes the JRuby implementation, to do a better job of matching the CRuby behavior.
@flavorjones flavorjones modified the milestones: v1.12.0, v1.13.0 Aug 2, 2021
@flavorjones flavorjones modified the milestones: v1.13.0, v1.14.0 Jan 6, 2022
@flavorjones
Copy link
Member Author

Here's the plan I'd like to follow:

Introduce, in v1.14, support for the nokogiri: namespace in the CRuby implementation.

Introduce, in v1.15, a deprecation warning if custom functions are invoked without the nokogiri: namespace. Something like this seems to work:

diff --git a/ext/nokogiri/nokogiri.h b/ext/nokogiri/nokogiri.h
index 63549ec..a81ff66 100644
--- a/ext/nokogiri/nokogiri.h
+++ b/ext/nokogiri/nokogiri.h
@@ -210,9 +210,9 @@ NOKOPUBFUN VALUE Nokogiri_wrap_xml_document(VALUE klass,
 #define DISCARD_CONST_QUAL_XMLCHAR(v) DISCARD_CONST_QUAL(xmlChar *, v)
 
 #if HAVE_RB_CATEGORY_WARNING
-#  define NOKO_WARN_DEPRECATION(message) rb_category_warning(RB_WARN_CATEGORY_DEPRECATED, message)
+#  define NOKO_WARN_DEPRECATION(message...) rb_category_warning(RB_WARN_CATEGORY_DEPRECATED, message)
 #else
-#  define NOKO_WARN_DEPRECATION(message) rb_warning(message)
+#  define NOKO_WARN_DEPRECATION(message...) rb_warning(message)
 #endif
 
 void Nokogiri_structured_error_func_save(libxmlStructuredErrorHandlerState *handler_state);
diff --git a/ext/nokogiri/xml_xpath_context.c b/ext/nokogiri/xml_xpath_context.c
index b8eb633..f412067 100644
--- a/ext/nokogiri/xml_xpath_context.c
+++ b/ext/nokogiri/xml_xpath_context.c
@@ -284,6 +284,13 @@ lookup(void *ctx,
        const xmlChar *ns_uri)
 {
   VALUE xpath_handler = (VALUE)ctx;
+  if (ns_uri == NULL) {
+    NOKO_WARN_DEPRECATION(
+      "A custom XPath or CSS handler function named '%s' is being invoked without a namespace."
+      " Please update your query to reference this function as 'nokogiri:%s'."
+      " Invoking custom handler functions without a namespace is deprecated and support will be removed in a future release of Nokogiri.",
+      name, name);
+  }
   if (rb_respond_to(xpath_handler, rb_intern((const char *)name))) {
     return ruby_funcall;
   }

Then, in some future version of Nokogiri, we'll remove support for non-namespaced custom functions.

flavorjones added a commit that referenced this issue Aug 29, 2022
flavorjones added a commit that referenced this issue Aug 29, 2022
@flavorjones flavorjones modified the milestones: v1.14.0, v1.15.0 Aug 30, 2022
flavorjones added a commit that referenced this issue Apr 28, 2023
flavorjones added a commit that referenced this issue Apr 28, 2023
flavorjones added a commit that referenced this issue Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant