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

Improve jruby support of ComplexFields #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scottmtp
Copy link

@scottmtp scottmtp commented Oct 22, 2022

Due to underlying differences in the jruby and MRI xml libraries in nokogiri, sablon doesn't currently work well in jruby. This PR improves the situation by fixing the ComplexField class in mail_merge.rb.

Currently the ComplexField class contains xpath searches that use namespace prefixed attributes in the search expression, which appears to not work in all cases with jruby/nokogiri. Using local-name and ignoring the attribute namespace prefix appears to work well for both jruby and MRI.

With this change, many of the test documents in 'test/sandbox/' appear to be generated correctly, even though tests continue to fail due to minor differences in xml serialization.

@stadelmanma
Copy link
Collaborator

stadelmanma commented Oct 24, 2022

Hi @scottmtp this is an interesting fix, so basically the change ignores the particular namespace and something like “z:fldChar” would also match? I don’t know how much overlap there is on element names between namespaces but I’m not a huge fan of matching more than we should because it allows sneaky edge cases into the code that would be very very difficult to debug due to the complexity and opaqueness of the docx file type.

Maybe we can verify the namespace separately from the xpath query?

You also mentioned tests failing, can you check if the tests pass when run with regular Ruby?

I’m still not certain we can ever say jruby is 100% supported but I’m not at all opposed to improving support where possible.

@scottmtp
Copy link
Author

This change only relaxes the namespace check on attributes, not element names, so <z:fldChar w:fldCharType="end"/> would not match, but <w:fldChar z:fldCharType="end"/> would.

The test suite passes when run with cruby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants