Hi there @burke @darius,
I am trying to fix our document type definition files (we refer to undeclared elements)
for our modules config.xml that we have uploaded at
http://resources.openmrs.org/doctype/config-1.X.dtd and put them under version
control in openmrs-core. So they are useful to devs when writing config.xml in their IDEs and also so we can potentially also use them for validation in our
ModuleFileParser (better error messages to implementer, admins and devs, less code for us to write).
When creating the DTD for version 1.6 (which has never been uploaded) I run into
an issue due to the
conditionalResources element added in version 1.6 ( https://wiki.openmrs.org/display/docs/Module+Config+File).
Looking at https://wiki.openmrs.org/display/docs/Supporting+different+OpenMRS+versions
and the implementation of the
ModuleFileParser I see that 2 variations have been introduced:
Variation 2 (problematic one):
Variation 2 introduces another
module element but the
module is already defined in the DTD, it is our root element,
which is why we cannot declare it again https://www.w3.org/TR/REC-xml/#EDUnique
We cannot create a valid DTD for 1.6 if we adhere to our wiki’s descriptions.
I was searching for
conditionalResources on all github (not only openmrs org) and the only usages I found were of Variant 1
Would it be possible for us to change the Variant 2?
Hmm, good catch.
I wonder if @dkayiwa, @wyclif, or @raff know of anyplace that variant #2 is used?
If that variant is never used, and it makes our XML invalid, then I guess we should change the naming of that xml element. It’s actually a different use case than variation 1 – I think it lets you load a resource if a given module is loaded – so we should still preserve it. Assuming that’s what it’s for we could give it a very precise name like
that would be great. lets wait for the others to comment
@mksd i remember as if you used variant #2 for conditional loading based on missing or running modules. https://issues.openmrs.org/browse/TRUNK-5213
@dkayiwa thanks for pinging me on this.
I have never used the DTD for conditional resources loading, I always did it through class annotations. So this is no concern for me at this juncture. I have a feeling anyway that this hasn’t been used much for modules as your GitHub search seems to confirm, so it’s probably a good time to streamline it.
I’m saying this because ‘version’ already refers to the Maven version, and it could be that the module ID might in fact just always be the Maven artifact ID.
@mksd good thought. But I wonder if its not better to go with
moduleId since this reflects what we have in our code at https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/module/ModuleConditionalResource.java#L92-L98
which is what this xml represents in the end I guess. I am not sure how much moduleId’s and artifactId’s diverge.
These changes would make it to
If you are ok (@mksd @darius @dkayiwa) with this I’ll create the 1.6 dtd, adapt the code in the ModuleFileParser, the wiki’s https://wiki.openmrs.org/display/docs/Supporting+different+OpenMRS+versions, https://wiki.openmrs.org/display/docs/Module+Config+File
Another search on github for configVersion 1.6 also doesnt yield a match.
Naming the fields consistent with the underlying Java object sounds good to me.
I now also found out that the conditionalResource element has
the child element openmrsVersion as shown in the wiki but it also has kind of an “alias” element named openmrsPlatformVersion although the wiki doesnt mention this. In openmrs-core the ModuleFileParser treats it the same as openmrsVersion and takes either or to set the fieldfield openmrsPlatformVersion of object ModuleConditionalResource. github shows no use of openmrsPlatformVersion instead openmrsVersion is used exclusively. I see 2 options
- keep both elements as possible children to conditionalResource, since removing openmrsVersion as child from conditionalResource would break existing config.xml
- I could remove openmrsPlatformVersion without breaking existing config.xml, but then I would also change ModuleConditionalResource.openmrsPlatformVersion to ModuleConditionalResource.openmrsVersion so our DTD is closest to actual java objects
let me know which approach you favor
The ModuleConditionalResource class is not really part of the public API that clients code against, it’s used internally by the module engine therefore I think it’s fair to rename the
openmrsPlatformVersion field to
openmrsVersion becaise technically it shouldn’t break any client code.
If I had a time machine I would go back and tell us that the correct name for the attribute should be “openmrsCoreVersion”.
I guess that since “openmrsPlatformVersion” is not documented anywhere, and you couldn’t find any uses on github, I would choose #2 and remove it. (This is a weak preference though.)
@wyclif and @darius I researched more and found https://issues.openmrs.org/browse/TRUNK-4578
where we renamed the annotations
openmrsPlatformVersion, so it seems as if we kept the
openmrsVersion element in config.xml to not break compatibility but
OpenmrsProfile now only uses
Therefore I don’t think I should rename
ModuleConditionalResource.openmrsPlatformVersion and also allow the element
openmrsPlatformVersion as alias to
openmrsVersion in the config.xml. Since thats what is also implemented in the
My team has picked up the ticket https://issues.openmrs.org/browse/TRUNK-5403 recently, and we are not sure what there is left to do besides add openmrsPlatformVersion as a possible child of conditionalResource and create a pull request for the new dtd. Is there anything else that needs to be done besides that?
@teleivo do you happen to have any comments to the above?
My team wants to understand more about what these config files are used for. Could someone give a brief explanation of the purpose of these config files?
Furthermore, we tested config_1.5 and found that there was a syntax error in the file. Which I believe we plan on fixing before releasing a config_1.6. @dkayiwa @teleivo @darius @wyclif
Our team has made changes, and fixed syntax errors in config_1.5. We submitted our pull request https://github.com/openmrs/openmrs-core/pull/2875 and also added test cases for the config files.
I put a comment for you on github.