Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix for pull request #794 #797

wants to merge 5 commits into


None yet
3 participants

jvshahid commented Nov 20, 2012

Hi @yokolet,

This is a fix for pull request #794. Can you please eyeball the changes. Most importantly I've removed the content caching from XmlNode.java. Initially I implemented the fix in Java (commit 1928899) but later found it much cleaner to do it in Ruby (commit 96f91eb).

Commits 31dd404 and d556f06 include the two test cases that I was trying to fix.

Do you remember why caching was there ? Is it going to break anything ?



benlangfeld and others added some commits Nov 18, 2012

@benlangfeld @jvshahid benlangfeld Add failing test case for Nokogiri::XML::Node#content inconsistency b…
…etween Java and C
@jvshahid jvshahid add a new test to demonstrate broken content behavior in Java. 31dd404
@jvshahid jvshahid Initial fix for #794. I think the solution can be drastically simplif…
…ied. Details below.

This patch changes the behavior of node.content to recursively
aggregate text from children nodes, unless this is a Text node in
which case the content is returned.

In order to support recursively appending contents of children
elements and to fix the broken append behavior demonstrated by the
test in the previous commit, I had to remove the caching (which I'm
not sure why it was there in the first place) Also since we
recursively append text, we'll have to invalidate the cache when
changes deeper in the document tree occur.

Also I removed the content() from XML::Text since we don't need it
@jvshahid jvshahid pushed the fix for #794 to the ruby land.
This solution is much cleaner, but may not be as efficient.
This can be reverted to the previous commit which was pure
Java if there are issues with this implementation.
@jvshahid jvshahid update the changelog. 788f2d3

yokolet commented Nov 21, 2012

@jvshahid I have a time to work on this weekend. So, I'll have a look on Thursday or so. Give me a couple of days.


benlangfeld commented Nov 21, 2012

Tagging myself here for updates.


yokolet commented Nov 23, 2012

@jvshahid That content caching is mostly from a historical reason. When I started working, the caching was already there. It confused me a while, also it didn't work perfectly. I fixed those as much as possible.

I tried your fix and confirmed the fix worked well. So, I think we don't need caching anymore.

I know sometime fix in Ruby side is much simpler and cleaner. But, I've tried to avoid since I want to keep Ruby code compatibility with CRuby version. Honestly, I'm not sure what direction we should go.

Also, content method has a problem reported in #521 . This one is hard to fix. Just stripping out all whitespaces causes many failures in other tests.


benlangfeld commented Nov 23, 2012

@yokolet Would you be happy to merge the Java fix, but not 96f91eb? I would really like to get this in a release ASAP; it's the last thing in Nokogiri blocking for some very anxious clients.


jvshahid commented Nov 23, 2012

Thanks @yokolet for the review. I'll let you decide which version to merge on master. I don't really care.

Regarding #521, it looks like the newlines are actually inserted in the body tag (which is very interesting, it could be a Xerces bug). I've been trying for a while to read about XML whitespace handling since I have no clue what the specs say. May be it's time to do that.

@yokolet yokolet added a commit that referenced this pull request Nov 23, 2012

@yokolet yokolet Merges java version of fix of a pull request #797, which incluse a te…
…st case from pull request #794.

yokolet commented Nov 23, 2012

I merged Java version of fix and pushed to the master.

Sometime around Feb or March this year, I've talked with other Nokogiri committers about JRuby specific Ruby code. The conclusion then was that's ok for test cases, but others needed to think about. I did write JRuby specific Ruby code because implementation only in Java would be so hard and weird.

I think we should bring this topic on the table again.


yokolet commented Nov 23, 2012

I'm going to close this issue.

@yokolet yokolet closed this Nov 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment