Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: changes.xml
    • Labels:
      None
    • Environment:
      Maven version: 2.0.10
      Java version: 1.5.0_17
      OS name: "linux" version: "2.6.32-686" arch: "i386" Family: "unix"

      Description

      The fix for MCHANGES-189 does not seem to be correct. A changes.xml file containing HTML-Tags got rendered correctly using version 2.2. Starting with version 2.3, HTML-Tags will be encoded using HTML entities for the '<' and '>' characters leading to the actual tags getting shown in the report. See the attached example changes.xml file containing HTML no longer working with version 2.3.

      1. MCHANGES-190-2.patch
        3 kB
      2. MCHANGES-190.zip
        1 kB
      3. MCHANGES-190.patch
        3 kB
        dennislundberg
      4. changes.xml
        0.7 kB
      5. changes.xml
        0.5 kB
        dennislundberg

        Issue Links

          Activity

          Christian Schulte created issue -
          Hide
          Christian Schulte added a comment -

          Example file containing HTML tags.

          Show
          Christian Schulte added a comment - Example file containing HTML tags.
          schulte2005 made changes -
          Field Original Value New Value
          Attachment changes.xml [ 46378 ]
          Hide
          Dennis Lundberg added a comment -

          Hi Christian

          It was I who made the patch from MCHANGES-189. I was struggling to get issues that contains html-entities to show up correctly in both the announcement and in the changes-report. I'll upload an example shortly showing what I mean.

          Show
          Dennis Lundberg added a comment - Hi Christian It was I who made the patch from MCHANGES-189 . I was struggling to get issues that contains html-entities to show up correctly in both the announcement and in the changes-report. I'll upload an example shortly showing what I mean.
          Hide
          Dennis Lundberg added a comment -

          A file with < and > encoded as html entities. This file works as expected for both the announcement and the changes-report.

          Show
          Dennis Lundberg added a comment - A file with < and > encoded as html entities. This file works as expected for both the announcement and the changes-report.
          Dennis Lundberg made changes -
          Attachment changes.xml [ 46399 ]
          Hide
          Christian Schulte added a comment -

          The changes file using HTML entities for the < and > characters does not work for me also.
          Please unzip MCHANGES-190.zip and execute

          mvn site
          

          and compare the result using the 2.2 version and the 2.3 version. The changes.xml file therein contains two actions using the same HTML. In a cdata section and using HTML entities.

                <action><![CDATA[<b>bold</b> - <i>italic</i>]]></action>
                <action>&lt;b&gt;bold&lt;/b&gt; - &lt;i&gt;italic&lt;/i&gt;</action>
          

          Using version 2.2, the raw HTML is rendered so a browser actually displays bold and italic text. Using version 2.3, the HTML got encoded so a browser displays the tags.

          Show
          Christian Schulte added a comment - The changes file using HTML entities for the < and > characters does not work for me also. Please unzip MCHANGES-190 .zip and execute mvn site and compare the result using the 2.2 version and the 2.3 version. The changes.xml file therein contains two actions using the same HTML. In a cdata section and using HTML entities. <action><![CDATA[<b>bold</b> - <i>italic</i>]]></action> <action>&lt;b&gt;bold&lt;/b&gt; - &lt;i&gt;italic&lt;/i&gt;</action> Using version 2.2, the raw HTML is rendered so a browser actually displays bold and italic text. Using version 2.3, the HTML got encoded so a browser displays the tags.
          schulte2005 made changes -
          Attachment MCHANGES-190.zip [ 46401 ]
          Hide
          Dennis Lundberg added a comment -

          I think we need to have a configuration parameter for this.

          Some people, like me, wants to use the issue text for both the announcement and the report and can't have html code in the changes.xml file. Others, like yourself, wants to enhance their issue text with html code and only use it for the report.

          Now we only need to figure out a characteristic name for the parameter and a good default value. The parameter will go into the changes-report goal. How about allowHtml with a default value of "false"?

          Show
          Dennis Lundberg added a comment - I think we need to have a configuration parameter for this. Some people, like me, wants to use the issue text for both the announcement and the report and can't have html code in the changes.xml file. Others, like yourself, wants to enhance their issue text with html code and only use it for the report. Now we only need to figure out a characteristic name for the parameter and a good default value. The parameter will go into the changes-report goal. How about allowHtml with a default value of "false"?
          Hide
          Lukas Theussl added a comment -

          According to the current changes.xsd schema, the action element in changes.xml has text content only, ie no markup is allowed inside <action>. So I think the current behavior is actually correct. However, wrt to backward compatibility and functionality, this is not very satisfactory.

          Note that in maven 1, the <action> element could contain arbitrary xdoc markup, for the announcement only the text content was extracted.

          I think the proper solution would be to mimic this behavior, ie generalize the xsd schema and process the content of changes.xml according to where the output goes. A configuration parameter seems unnecessary to me, at least you would have to make it consistent with the xsd.

          Show
          Lukas Theussl added a comment - According to the current changes.xsd schema, the action element in changes.xml has text content only, ie no markup is allowed inside <action>. So I think the current behavior is actually correct. However, wrt to backward compatibility and functionality, this is not very satisfactory. Note that in maven 1, the <action> element could contain arbitrary xdoc markup, for the announcement only the text content was extracted. I think the proper solution would be to mimic this behavior, ie generalize the xsd schema and process the content of changes.xml according to where the output goes. A configuration parameter seems unnecessary to me, at least you would have to make it consistent with the xsd.
          Dennis Lundberg made changes -
          Link This issue is duplicated by MCHANGES-193 [ MCHANGES-193 ]
          Hide
          Joerg Schaible added a comment -

          This is really a major regression. I had to detect now that roughly 50 change reports for our last release cycle are simply rubbish! We added HTML for years in CDATA sections without any problems. This was valid XML and conforming to the schema.

          Show
          Joerg Schaible added a comment - This is really a major regression. I had to detect now that roughly 50 change reports for our last release cycle are simply rubbish! We added HTML for years in CDATA sections without any problems. This was valid XML and conforming to the schema.
          Hide
          Christian Schulte added a comment -

          When generalizing the xsd schema, support for action details (in addition to short descriptions) could be added so that current behaviour could be retained. Those details could be ignored by default when generating announcement mails. Seems the intended use of the changes.xml file is to specify a summary of changes rather than specifying a detailed changes report.

          > ...and process the content of changes.xml according to where the output goes.

          I would still say that no processing of the content from the changes.xml file should be performed at all. What if someone decides to send out HTML announcement mails ? Please note that I am currently only using the changes-report goal during site generation so I am very unfamiliar with the announcement mail feature.

          Show
          Christian Schulte added a comment - When generalizing the xsd schema, support for action details (in addition to short descriptions) could be added so that current behaviour could be retained. Those details could be ignored by default when generating announcement mails. Seems the intended use of the changes.xml file is to specify a summary of changes rather than specifying a detailed changes report. > ...and process the content of changes.xml according to where the output goes. I would still say that no processing of the content from the changes.xml file should be performed at all. What if someone decides to send out HTML announcement mails ? Please note that I am currently only using the changes-report goal during site generation so I am very unfamiliar with the announcement mail feature.
          Hide
          Lukas Theussl added a comment -

          no processing of the content from the changes.xml file should be performed at all.

          How do you want to generate anything without processing the content of changes.xml?

          What if someone decides to send out HTML announcement mails ?

          Then the content of changes.xml should be processed to produce an HTML mail (eg via an option in the changes:announcement-mail goal).

          The point of this issue is really just backward compatibility, the current behavior of the plugin is perfectly correct IMO. The fact that in earlier versions you could put arbitrary html inside CDATA is an undocumented (and I would guess unintentional) feature, which should really be considered a bug (it only works for html output, ie site generation, but you can't use it for eg producing a pdf of the changes report via the pdf plugin).

          Show
          Lukas Theussl added a comment - no processing of the content from the changes.xml file should be performed at all. How do you want to generate anything without processing the content of changes.xml? What if someone decides to send out HTML announcement mails ? Then the content of changes.xml should be processed to produce an HTML mail (eg via an option in the changes:announcement-mail goal). The point of this issue is really just backward compatibility, the current behavior of the plugin is perfectly correct IMO. The fact that in earlier versions you could put arbitrary html inside CDATA is an undocumented (and I would guess unintentional) feature, which should really be considered a bug (it only works for html output, ie site generation, but you can't use it for eg producing a pdf of the changes report via the pdf plugin).
          Hide
          Sebb added a comment -

          Note that version 2.0 also allowed HTML markup which was not in a CDATA section.

          This has also been used, e.g. to add links and emphasis, both of which would also be useful in PDF output.

          ==

          As to announcements, these are processed using a Velocity template.
          Velocity is powerful enough to be able to handle markup so surely it ought to be possible to fix this somehow?

          Show
          Sebb added a comment - Note that version 2.0 also allowed HTML markup which was not in a CDATA section. This has also been used, e.g. to add links and emphasis, both of which would also be useful in PDF output. == As to announcements, these are processed using a Velocity template. Velocity is powerful enough to be able to handle markup so surely it ought to be possible to fix this somehow?
          Hide
          Jörg Hohwiller added a comment -

          IMHO adding markup as text in CDATA sections was always a hack.
          The best way to fix this would be to allow xsd:any in action and put the content as is in the result.
          Then we also do not need a trigger as commented in
          "Dennis Lundberg added a comment - 21/Dec/09 2:31 PM"
          because one can still have plain text in CDATA and does not need to care about escaping.

          Show
          Jörg Hohwiller added a comment - IMHO adding markup as text in CDATA sections was always a hack. The best way to fix this would be to allow xsd:any in action and put the content as is in the result. Then we also do not need a trigger as commented in "Dennis Lundberg added a comment - 21/Dec/09 2:31 PM" because one can still have plain text in CDATA and does not need to care about escaping.
          Hide
          Alexander Daniel added a comment -

          > Note that version 2.0 also allowed HTML markup which was not in a CDATA section.

          For me HTML markup only works in a CDATA section even with version 2.0

          Show
          Alexander Daniel added a comment - > Note that version 2.0 also allowed HTML markup which was not in a CDATA section. For me HTML markup only works in a CDATA section even with version 2.0
          Hide
          Dennis Lundberg added a comment -

          Here's an attempt to objectively summarize the comments on this issue so far.

          1. We want it to be possible to somehow have HTML markup in the changes report, at least for backwards compatibility reasons.
          2. The most transparent way is to put HTML markup inside CDATA sections, that way we don't have to change the XSD.
          3. Different mojos have different things they want achieve, and so do different users. We need some means to allow this diversity.

          Please let med know if we agree on these points.

          I'll follow up after that with my suggested way to implement them.

          Show
          Dennis Lundberg added a comment - Here's an attempt to objectively summarize the comments on this issue so far. We want it to be possible to somehow have HTML markup in the changes report, at least for backwards compatibility reasons. The most transparent way is to put HTML markup inside CDATA sections, that way we don't have to change the XSD. Different mojos have different things they want achieve, and so do different users. We need some means to allow this diversity. Please let med know if we agree on these points. I'll follow up after that with my suggested way to implement them.
          Hide
          Lukas Theussl added a comment -

          I don't agree with 2. I think the most transparent way is to change the XSD. However, I would like to see your proposed implementation first, if you can come up with an solution that doesn't need a new xsd and that satisfies everyone, I will happily accept it.

          Show
          Lukas Theussl added a comment - I don't agree with 2. I think the most transparent way is to change the XSD. However, I would like to see your proposed implementation first, if you can come up with an solution that doesn't need a new xsd and that satisfies everyone, I will happily accept it.
          Hide
          Dennis Lundberg added a comment -

          Here is my proposed patch to fix this issue.

          It should satisfy those users who want to use HTML code inside a CDATA section to format the contents of their <action>s in the changes report. They would simply add this to their POM:

                  <configuration>
                    <escapeHTML>false</escapeHTML>
                  </configuration>
          

          Users of the announcement-generate goal are unaffected by this change (and this issue for that matter). If you put HTML code inside your <action>s it will be written to the announcement file.

          If someone later on want to add a new feature to this plugin that would allow sending HTML emails, that would work too.

          Show
          Dennis Lundberg added a comment - Here is my proposed patch to fix this issue. It should satisfy those users who want to use HTML code inside a CDATA section to format the contents of their <action>s in the changes report. They would simply add this to their POM: <configuration> <escapeHTML>false</escapeHTML> </configuration> Users of the announcement-generate goal are unaffected by this change (and this issue for that matter). If you put HTML code inside your <action>s it will be written to the announcement file. If someone later on want to add a new feature to this plugin that would allow sending HTML emails, that would work too.
          Dennis Lundberg made changes -
          Attachment MCHANGES-190.patch [ 52829 ]
          Hide
          Christian Schulte added a comment -

          Setting 'escapeHTML' to 'false' the patch works for me.

          Show
          Christian Schulte added a comment - Setting 'escapeHTML' to 'false' the patch works for me.
          Hide
          Christian Schulte added a comment -

          Although unrelated to this issue, looking at the announcement mail feature, I noticed that the 'announcement-generate' goal supports template encodings but the 'announcement-mail' goal does not. This patch adds a 'templateEncoding' parameter to the 'announcement-mail' goal.

          Show
          Christian Schulte added a comment - Although unrelated to this issue, looking at the announcement mail feature, I noticed that the 'announcement-generate' goal supports template encodings but the 'announcement-mail' goal does not. This patch adds a 'templateEncoding' parameter to the 'announcement-mail' goal.
          schulte2005 made changes -
          Attachment MCHANGES-190-2.patch [ 52845 ]
          Hide
          Lukas Theussl added a comment -

          @Christian: If it's unrelated, please attach it to a new issue.

          @Dennis: as a hack, this is fine, I am just somewhat reluctant as introducing this means that we 'officially' support this feature, which was never the intention. Just making sure you are aware of the implications...

          Show
          Lukas Theussl added a comment - @Christian: If it's unrelated, please attach it to a new issue. @Dennis: as a hack, this is fine, I am just somewhat reluctant as introducing this means that we 'officially' support this feature, which was never the intention. Just making sure you are aware of the implications...
          Lukas Theussl made changes -
          Link This issue depends upon MODELLO-254 [ MODELLO-254 ]
          Hide
          Dennis Lundberg added a comment -

          Right. Do you think I should add a disclaimer in the parameter documentation about this? Something like this:

          "Putting any kind of markup inside a CDATA section might mess up the Changes Report or other generated documents, such as PDFs, that are based on your changes.xml file."

          Show
          Dennis Lundberg added a comment - Right. Do you think I should add a disclaimer in the parameter documentation about this? Something like this: "Putting any kind of markup inside a CDATA section might mess up the Changes Report or other generated documents, such as PDFs, that are based on your changes.xml file."
          Hide
          Dennis Lundberg added a comment -

          Fixed in r1054123.

          Show
          Dennis Lundberg added a comment - Fixed in r1054123 .
          Dennis Lundberg made changes -
          Fix Version/s 2.4 [ 16043 ]
          Assignee Dennis Lundberg [ dennislundberg ]
          Status Open [ 1 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Lukas Theussl made changes -
          Link This issue is related to MCHANGES-216 [ MCHANGES-216 ]
          Mark Thomas made changes -
          Project Import Sun Apr 05 09:14:45 UTC 2015 [ 1428225285377 ]
          Mark Thomas made changes -
          Workflow jira [ 12718024 ] Default workflow, editable Closed status [ 12749330 ]
          Mark Thomas made changes -
          Project Import Sun Apr 05 22:40:15 UTC 2015 [ 1428273615853 ]
          Mark Thomas made changes -
          Workflow jira [ 12955605 ] Default workflow, editable Closed status [ 12992963 ]
          Mark Thomas made changes -
          Assignee dennislundberg [ dennislundberg ] Dennis Lundberg [ dennisl@apache.org ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Closed Closed
          384d 23h 8m 1 Dennis Lundberg 31/Dec/10 08:49

            People

            • Assignee:
              Dennis Lundberg
              Reporter:
              Christian Schulte
            • Votes:
              8 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development