SQ3-410 - fix: enable WordML content reusability with current_fragment helper#11
Closed
leandronsp wants to merge 2 commits into
Closed
Conversation
PR senny#84 introduced a bug where WordML objects could only be used once. The issue occurred because Nokogiri's add_next_sibling() moves nodes rather than copying them, leaving xml.children empty after first use. Changes: - Add current_fragment() helper that creates fresh node instances on demand - Update all_inline?() and add_siblings_to() to use current_fragment - Add test case that reuses same table content twice to prevent regression This fix maintains PR senny#84's architecture (storing DocumentFragment) while solving the node reuse issue by lazy-parsing fresh fragments when needed.
jmnsf
approved these changes
Nov 18, 2025
Author
|
Closing this PR after further investigation. The root cause is in our implementation, not in Sablon upstream. Our code happened to work with the old Sablon version because it masks the bug by constantly creating new fragments. The memoize we introduced recently worked accidentally with the old upstream implementation. However, the new Sablon upstream exposed the bug that our memoize introduced. Combined with Nokogiri's We decided the fix needs to be on our side by removing the problematic memoization, not by patching Sablon. cc @jmnsf |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Payment plan tables were disappearing from generated PDFs. Troubleshooting revealed the root cause was in Sablon PR #84 (2018), which changed WordML to store parsed DocumentFragment objects instead of strings.
The problem: Nokogiri's
add_next_siblingmoves nodes rather than copying them. When a WordML object was used once, its nodes were moved fromxml.childreninto the document. On subsequent reuse,xml.childrenwas empty, causing content to silently disappear.The bug existed since Sablon PR #84 (Jan 2018) but went unnoticed because most code creates fresh WordML objects for each use. Payment plans are the possible unique case that truly reused the same object (to check other scenarios as well).
Changes
Added
current_fragmenthelper method that creates fresh node instances on demand by re-parsing XML. This can be seen as not performant at first glance, but they are parsed only when needed, so I think we won't face performance issues here.Also added a new test case that reuses the same table content twice to prevent regression.