I moved discussion here to avoid notify other Apache committers. I think you should close the PR and open a new one :)
2018-02-19 21:58 GMT+01:00 Yasser Zamani <notificati...@github.com>: > 4b89034 > <https://github.com/apache/struts/commit/4b890345e8b7a7f760b170672782296e9113d445> > has deleted if (isReloadingConfigs()) { from all implemented > FileManager#monitorFile methods. This causes Struts to monitor all loaded > files always, regardless of if the value of STRUTS_CONFIGURATION_XML_RELOAD > = "struts.configuration.xml.reload" is true or false. i.e. Struts may > reload a file even when that constant is set to false because if > (revision == null) { is false and Struts will use monitored revision in > FileManager#fileNeedsReloading. As FileManager interface documents: > > /** * Adds file to list of monitored files if {@link > #setReloadingConfigs(boolean)} is true * * @param fileUrl {@link URL} > to file to be monitored */ > void monitorFile(URL fileUrl); > > I think we must revert this change (other changes of this commit looks > good). I'm just worry about: > > //watch class file > if (isReloadEnabled()) { > URL classFile = > actionClass.getResource(actionClass.getSimpleName() + ".class"); > fileManager.monitorFile(classFile); > loadedFileUrls.add(classFile.toString()); > } > > in \convention\PackageBasedActionConfigBuilder.java. This piece of code > seems expects monitorFile method always monitor but I think if it wants to > monitor .class files, then it must use it's own internal mechanism > because as documents say, the monitorFile method will monitor if > STRUTS_CONFIGURATION_XML_RELOAD = "struts.configuration.xml.reload" is > set to true by user. > > Any way I feel OK to revert back that if statement and adding more heavy > test covering all possible situations i.e. when that constant is set to > true or false (two states) and when file is modified or not (two states) so > total four states. @lukaszlenart <https://github.com/lukaszlenart> , > @apache/apache-committers > <https://github.com/orgs/apache/teams/apache-committers> ❓ > > This is not needed as the reload check was moved into FileManager#fileNeedsReloading methods - see usage of those methods. Regards -- Łukasz + 48 606 323 122 http://www.lenart.org.pl/
