I would add a comment to explain that dom4j Node doesn't have a replaceChild method (like org.w3c.dom.Node) so we're forced to replace the element by index. Moreover, dom4j Element doesn't have a method to get all child nodes (including text or comment nodes) so the index we use must be from the list of child elements.

Thank you Marius. Done in below commit (6bd2104).

I would name the file *.xml since it contains XML.

Also would be good to add a license header (we need to add that to other files too).

Ok for the license header.
These files end-up into the XAR... (it could or should be avoided) thus they become pages if they are named .xml which would fail since there is no xwikidoc element. Ok to keep .xwikiclass?
(there's a lot more XML files out there which are not called .xml, e.g. the .plist on macs, or the rss files).

I don't remember the test but then it looks like these resource files are placed in the wrong location if they end up in the XAR when they shouldn't. Using a different suffix is not the right way IMO. In XWiki we use xml for xml files (it helps in the IDE, it helps visually know what the file is about, etc). I don't see any valid argument for not doing that honestly, especially since this file is a xwiki XML page which is supposed to end in .xml.

I've now checked the code and the doc at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations. I think the doc is a bit wrong at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations and the files that are used only as intermediary resources that are used for transformations should be excluded. If they're not needed to be processed (velocity doesn't need to run on them) they could also be put in a different directory such as src/main/xar. I think that's a better idea than the exclude.

they could also be put in a different directory such as src/main/xar. I think that's a better idea than the exclude.

Yes, I don't think exclusion is a good idea.

Very cool. Btw, the file name is entirely left to the developer. If you give the XARMojo a JPEG file as XML source for a REPLACE or INSERT_CHILD, it will (try to) parse it.

So we should open a new bug about the XARMojo so that it implicitly adds excludes for files that are not considered as pages, right? (we could even try to parse the document if xml and exclude it if is not with root xwikidoc once we find this method).

So we should open a new bug about the XARMojo so that it implicitly adds excludes for files that are not considered as pages, right? (we could even try to parse the document if xml and exclude it if is not with root xwikidoc once we find this method).

This is not the problem and there's no bug to open on XARMojo. It already only reads xml files. The problem is the usage of the XARMojo (ie the tests you're modifying). They must not mix files that are sources for the XAR from files that are sources for the transformations. It's just bad practice and asking for trouble to mix them. It's easy to see that. Imagine if you had named your file .xml, you'd see the issue immediately.

What is the part that you don't understand or don't agree with? I'm asking because we're turning in circles and I feel like I keep repeating myself over and over.

1.- this discussion should not be here...
2.- I don't agree with the fact that XARMojo should copy vm, html, png, properties or any other file since it does nothing with this
3.- I don't agree with your view that it is best practice to separate the folders between what's the input for the transformation (e.g. properties or vm) and what is not quite the input (e.g. the XML which is, in the absence of transformation, just copied).

Ok so let me redo all the analysis work from scratch (I didn't want to spend so much time on this since it was minor and simple for me):

1. the source for the XML files is target/classes (

   xwiki-commons/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-plugin/src/main/java/org/xwiki/tool/xar/XARMojo.java

   Line 111 in 348e478

   File sourceDir = new File(this.project.getBuild().getOutputDirectory());

   ). And not src/main/resources (except for package.xml but that's potentially an error IMO).
2. The transformation sources are taken from src/main/resources in the existing code and in your PR:

            <transformation>
              <file>Blog/WebHome.xml</file>
              <xpath>/xwikidoc/content</xpath>
              <action>INSERT_TEXT</action>
              <charset>UTF-8</charset>
              <content>src/main/resources/Blog/WebHome.vm</content>
            </transformation>
   

Conclusions:

1. There's a problem since the maven resources plugin will copy all files from src/main/resources to target/classes while the transformation sources are taken from src/main/resources which is useless in this case (and prevents using filtering if needed BTW)
2. There's another problem since the maven resources plugin will copy all files from src/main/resources to target/classes including XML files named *.xml, which will then be included in the XAR by default (

   xwiki-commons/xwiki-commons-tools/xwiki-commons-tool-xar/xwiki-commons-tool-xar-plugin/src/main/java/org/xwiki/tool/xar/AbstractXARMojo.java

   Line 93 in 71bbc48

   private static final String[] DEFAULT_INCLUDES = new String[] {"**/*.xml"};

   ).

Possible solutions:

1. Configure the maven resource plugin to exclude transformation files from being copied to target/classes. Note that this works unless we want to use the filtering feature of the maven resources plugin (not the case in this test).
2. Use a separate source directories for transformation files (src/main/transformations). This will make sure that the maven resource plugin will not copy them and that the XARMojo won't try to include them in the XAR.
3. Change the pom.xml of the test to not use src/main/resources for transformation content and instead fetch them from target/classes (since they've been copied, and they can benefit from filtering too). Configure the XARMojo plugin with includes/excludes to only include what's needed in the generated XAR.

Pros/cons:

• Solution 1 is not very interesting IMO. No filtering and a bit complex since you need to hand pick the excludes.
• Solution 2 is the best to me (unless we want filtering but right now we've never used that so it's pretty safe and even if needed if doesn't prevent using solution 1 or 3 in the rare cases where it'd be needed), because it's very simple and doesn't require dealing with hand-picked includes/excludes. So it's simpler and less prone to errors.
• Solution 3 is good if we need filtering. But same as solution 1, it requires overriding the include/excludes and hand-pick them, which is more complex and error prone.

So I continue to think that solution 2 is the best (@tmortagne seems to think too according to #136 (comment)).

Now you said you don't like it but I don't recall seing any argument from you as to why? Which solution do you prefer and why?

Thanks

Hello @vmassol ,

I don't like your solution 2 because files that constitute other files should be aside of them. This makes sense, for example, when vm should include neighbour pages... It is disputable and the dispute is really a matter of "conceptual cleanliness vs tools current capabilities". That said, I must say that if you insist that copied files inside a xar are a problem, I guess it is indeed (as @tmortagne mentioned) the lesser evil.

I mentioned my wish for a coherent picture in this more generic post. And I really think we should not argue further on this pull-request as it was just trying to fix a bug in less than two lines while the debate is far broader and touches much older things (e.g. the transformation examples or best practices more than the tests).

paul

Indentation has 4 chars too much.

Is it normal to use "//" instead of the "/xwikidoc/..." used in the previous assert? Especially since it's a single node, it's probably more accurate to test for a precise location. WDYT?

"//" instead of the "/xwikidoc/..." used in the previous assert? Especially since it's a single node, it's probably more accurate to test for a precise location. WDYT?

Thus the test-verification is broader: it asserts that no element with that local-name are anywhere in the tree. So it makes it a more sure test!

Indentation has 4 chars too much.

fixed.

Thus the test-verification is broader: it asserts that no element with that local-name are anywhere in the tree. So it makes it a more sure test!

I disagree. Adding insertHereBlogXWikiClass in the wrong part of the XML is not correct and the test should verify that. The test must verify it's added in the right place.

You disagree with the fact that the verification is broader?
Or do you feel the test should verify that the replacement has happened by the existence of a particular element?

Your transformation is defined as:

<transformation> 
   <file>Blog/WebHome.xml</file> 
   <action>REPLACE</action> 
   <xpath>/xwikidoc/object/insertHereBlogXWikiClass</xpath> 
   <xml>src/main/resources/Blog/BlogClass.xwikiclass</xml> 
 </transformation> 

So AFAIU we need to verify that /xwikidoc/object/insertHereBlogXWikiClass is replaced and only that part of the XML. If another part is replaced then the code is clearly wrong.

Well, in the case of this example and example file (a test is defined by all its parts!) both are the same. Why should the test be minimal and ignore the nature of the input-data?

I guess it might be more elegant to write <xpath>//insertHereBlogXWikiClass</xpath> but showing by an automated test that both things are converging is actually showing that the machine works well. Another thing that this tests, is that the element has not been moved somewhere else (e.g. the insertion has taken place but the element is at some other place...).

I've now checked the code and the doc at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations. I think the doc is a bit wrong at https://dev.xwiki.org/xwiki/bin/view/Community/XARPlugin#HTransformations and the files that are used only as intermediary resources that are used for transformations should be excluded. If they're not needed to be processed (velocity doesn't need to run on them) they could also be put in a different directory such as src/main/xar. I think that's a better idea than the exclude.