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

add test case checking decoration of empty NodeSet, which fails on JRuby #1319

Conversation

jkraemer
Copy link
Contributor

It seems that empty NodeSets do not have any decorators applied to them on JRuby, while they indeed get decorated on Ruby. The problem seems to be that initialize in XmlNodeSet.java expects the first element of the node set as an argument, where it retrieves the document from which is then used for decoration. empty set -> no first element -> no decoration takes place. I have a test case illustrating the problem, but unfortunately no idea for a solution.

Loofah (and therefore also rails-html-sanitizer) has several test cases failing on JRuby because of this.

@marutosi
Copy link
Contributor

marutosi commented Sep 1, 2015

What is status? flavorjones/loofah#88 (comment) says this is Rails 4.2 with JRuby.

@marutosi
Copy link
Contributor

As I wrote at flavorjones/loofah#94 (comment), I cannot understand loofah and nokogiri support policy.

@flavorjones
Copy link
Member

Looking at this now.

@flavorjones
Copy link
Member

Just an update -- not sure how to fix this without rewriting all of NodeSet. And so the delay in fixing.

@flavorjones
Copy link
Member

@kares This is the second important JRuby issue, which is also blocking Loofah and Rails on JRuby.

I've pushed branch 1319_jkraemer_jruby-empty-nodeset-not-decorated with the above failing test on it.

@kares
Copy link
Contributor

kares commented Feb 10, 2017

@flavorjones thanks, wondered about this as I stumbled at the doc retrieval code described in the issue
-> believe it might need some internal refactoring, a 'quick' hack/fix might not be possible here

@kares kares mentioned this pull request Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants