Cocoon 3
  1. Cocoon 3
  2. COCOON3-51

XIncludeTransformer sends extra startDocument and endDocument events

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha-3
    • Fix Version/s: 3.0.0-alpha-3
    • Component/s: cocoon-sax
    • Labels:
      None

      Description

      The XIncludeTransformer sends extra start and endDocument events causing errors in subsequent handlers.
      1. patch.txt
        1 kB
        Charles Yates
      2. COCOON3-51.txt
        6 kB
        Charles Yates

        Activity

        Hide
        Charles Yates added a comment -
        Thank you Simone, I also concluded keeping track of the document level was the best solution.
        Show
        Charles Yates added a comment - Thank you Simone, I also concluded keeping track of the document level was the best solution.
        Hide
        Simone Tripodi added a comment -
        Patch applied successfully. I investigated a lot about it before applying it and I combined the two patches provided, there's no better way than using the 'documentLevel' variable - see first submitted patch - and tested with test provided in the second patch.
        Thanks Charles :)
        Show
        Simone Tripodi added a comment - Patch applied successfully. I investigated a lot about it before applying it and I combined the two patches provided, there's no better way than using the 'documentLevel' variable - see first submitted patch - and tested with test provided in the second patch. Thanks Charles :)
        Hide
        Simone Tripodi added a comment -
        very very appreciated Charles, thanks, test is meaningful and way of submitting patches is now much better, but please keep in mindfor the future patches you'll submit, when adding new resources, like the XML test, please add the Apache License header.

        I would avoid adding an external dependency in the XIncludeTransformer that's a dependencies-less class. I'm sure there is a bug on the use of 'xIncludeElementLevel' class field, which scope is also avoiding the bug you noticed.

        BTW, well done ;)
        Show
        Simone Tripodi added a comment - very very appreciated Charles, thanks, test is meaningful and way of submitting patches is now much better, but please keep in mindfor the future patches you'll submit, when adding new resources, like the XML test, please add the Apache License header. I would avoid adding an external dependency in the XIncludeTransformer that's a dependencies-less class. I'm sure there is a bug on the use of 'xIncludeElementLevel' class field, which scope is also avoiding the bug you noticed. BTW, well done ;)
        Hide
        Charles Yates added a comment -
        Hi Simone,

        Of course you are right I should include test cases. Also in this patch is a more elegant fix using org.apache.cocoon.xml.sax.EmbeddedSAXPipe to catch the extra document events.
        Show
        Charles Yates added a comment - Hi Simone, Of course you are right I should include test cases. Also in this patch is a more elegant fix using org.apache.cocoon.xml.sax.EmbeddedSAXPipe to catch the extra document events.
        Hide
        Simone Tripodi added a comment -
        Generally speaking, It would be nice also if you could provide testcases to show why the patch has to be applied.
        Thanks in advance!!!
        Show
        Simone Tripodi added a comment - Generally speaking, It would be nice also if you could provide testcases to show why the patch has to be applied. Thanks in advance!!!
        Hide
        Simone Tripodi added a comment -
        Charles,
        instead of declaring yet another variable (the documentLevel) can you help us understanding why with the xIncludeElementLevel doesn't work?
        Yet another suggestion: when naming patches, can you please call patch files with meaningful names?
        Thanks in advance, very appreciated!!!
        Show
        Simone Tripodi added a comment - Charles, instead of declaring yet another variable (the documentLevel) can you help us understanding why with the xIncludeElementLevel doesn't work? Yet another suggestion: when naming patches, can you please call patch files with meaningful names? Thanks in advance, very appreciated!!!
        Hide
        Simone Tripodi added a comment -
        Hi Charles,
        very appreciated, when I implemented the XInclude i was sure it was aware from such problems but I'll apply the patch ASAP (maybe tomorrow)
        Show
        Simone Tripodi added a comment - Hi Charles, very appreciated, when I implemented the XInclude i was sure it was aware from such problems but I'll apply the patch ASAP (maybe tomorrow)

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            Charles Yates
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development