-
Notifications
You must be signed in to change notification settings - Fork 96
[MWAR-450] ISO-8859-1 properties files get changed into UTF-8 when fi… #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Move maven to proper scope, update dependencies, remove unused ones.
src/main/java/org/apache/maven/plugins/war/packaging/AbstractWarPackagingTask.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/war/packaging/AbstractWarPackagingTask.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/war/packaging/AbstractWarPackagingTask.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review Michael! I have made changes based on your suggestions. |
| <resource> | ||
| <targetPath>WEB-INF/classes</targetPath> | ||
| <filtering>true</filtering> | ||
| <directory>src/main/webapp/WEB-INF/classes</directory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't they in src/main/resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they don't in our use case. We like to separate our config files from our resources. This way the config files end up readable in the exploded war, instead of being packaged in the jar file of the project.
| } | ||
| FileInputStream fis = new FileInputStream ( log4jxml ); | ||
| String paramContent = IOUtil.toString ( fis, "UTF-8" ); | ||
| System.out.println( "content='" + paramContent + "'" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This output will be mangled if run on non-UTF-8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tried this on both Windows (CP-1252) and Linux (UTF-8), and the output from the integration test looks fine for us, without mangled characters. Which platform did you have problems on?
| properties.load( fis ); | ||
| fis.close(); | ||
| String property = properties.get( "my.property" ); | ||
| System.out.println( "my.property='" + property + "'" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| } | ||
| FileInputStream fis = new FileInputStream ( myfile ); | ||
| String paramContent = IOUtil.toString ( fis, "UTF-8" ); | ||
| System.out.println( "content='" + paramContent + "'" ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
| System.err.println( "my.properties is missing or is a directory." ); | ||
| return false; | ||
| } | ||
| Properties properties = new Properties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In jdk9 encoding of properties was changed.
https://docs.oracle.com/javase/9/intl/internationalization-enhancements-jdk-9.htm
Maybe good assertions will be length of file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true that changes were made in Java 9. From what I understand the changes were made with regards to internationalization in PropertyResourceBundle, but not to regular Properties. https://docs.oracle.com/javase/9/docs/api/java/util/Properties.html
The assertions that are in place should work fine regardless of Java version since we specify the encoding to use when reading the files, in the plugin's code. The only exception som that rule is when reading the properties file in the verify.bsh file in the integration test.
|
Oh c'mon, really why didn't you rebase first? Now we have those useless merge commits on master. |
|
Sorry, I messed up. I wanted to apply the pull requests in order, but there was a conflict between the PR for MWAR-444 and MWAR-450 that I was unable to resolve at github. Also I wanted to make sure that the failed checks for MWAR-444 had been resolved in master, so I decided to merge the PR for MWAR-444 into our fork. That was apparently one too many remote references for me to handle. I really hate those merge commits... |
|
Resolve #528 |
1 similar comment
|
Resolve #528 |
…ltered.
https://issues.apache.org/jira/browse/MWAR-450
The fix for this issue is similar to MRESOURCES-171. We have added a configuration parameter called propertiesEncoding to the Mojo. If it is not specified it will not be used. Under the hood maven-filtering has been updated to 3.2.0 to be able to use the propertiesEncoding feature in it. maven-shared-utils is also updated to match the version in maven-filtering.
An integration test MWAR-450 has been added to cover the use case described in JIRA. There is already another integration test called web-resources-filtering that covers the behavior prior to this patch.
[ X ] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
[ X ] I am a commiter for the Apache Software Foundation