-
Notifications
You must be signed in to change notification settings - Fork 366
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
Issue #839 JavaLogFactory ConcMod #840
Conversation
Removing support for esapi-java-logging.properties file from baseline. ConfigurationException is thrown if file is found on the path at runtime. Exception message links to a wiki page with instructions on how to configure the application instance.
@jeremiahjstacey - Do you think it's useful to mention the previous defaults that were in the (now removed -- after this PR is merged) esapi-java-logging.properties file that used to be in the ESAPI configuration jar? I thought we may get questions about what properties used to be there for JUL and thought we could drop those into the new wiki page (https://github.com/ESAPI/esapi-java-legacy/wiki/Configuring-the-JavaLogFactory) that you wrote. There's only these 5:
I can add it if you think it would be useful. I just don't want that to come up as a question and the JUL Also, when setting
|
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.
@jeremiahjstacey - Some minor suggestions is all, but I'm okay with this PR as it is.
@xeno6696 - I may try doing a release over Memorial Day weekend (no promise though), so if I will give you until Sat around noon if you want to review this. Ideally, just leave an 'approved' as early as possible so I know you're good with it.
if (stream == null) { | ||
throw new ConfigurationException("Unable to locate resource: esapi-java-logging.properties"); | ||
if (stream != null) { | ||
throw new ConfigurationException("esapi-java-logging.properties is no longer supported. See https://github.com/ESAPI/esapi-java-legacy/wiki/Configuring-the-JavaLogFactory for information on corrective actions."); |
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.
I think it would make more sense to define this exception message as final String
and the reference it both places. That way, if we every decide to change the error message, we won't forget to change one of the instances. Otherwise, I think this is great and hopefully it will head off questions from those who seem to have a habit of never reading the release notes.
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.
Update applied
Removing unused imports. Consolidating String duplication to a class constant.
Removing support for esapi-java-logging.properties file from baseline.
ConfigurationException is thrown if file is found on the path at runtime. Exception message links to a wiki page with instructions on how to configure the application instance.
Closes #839
Also note the wiki page has been created.
https://github.com/ESAPI/esapi-java-legacy/wiki/Configuring-the-JavaLogFactory