You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/02/18 16:06:53 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new pull request #762: Trim property values
ppkarwasz opened a new pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762
Log4j 1.x almost always trimmed the values found in the configuration, the Log4j 1.x bridge should do the same.
Since `OptionConverter` already does most of the trimming, only class name trimming must be added. The (obsolete?) `Log4j1ConfigurationFactory` also fails to trim spaces in the _"level and appenders"_ specification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#discussion_r810543758
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java
##########
@@ -71,7 +71,7 @@ public Appender parseAppender(String name, String className, String prefix, Stri
String filterPrefix, Properties props, PropertiesConfiguration config) {
Objects.requireNonNull(plugins, "plugins");
Objects.requireNonNull(className, "className");
- PluginType<?> plugin = plugins.get(className.toLowerCase());
+ PluginType<?> plugin = plugins.get(className.toLowerCase().trim());
Review comment:
I am sorry @ppkarwasz I misread the line, I though the trim was applied to the result of the get, my mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#discussion_r810156858
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java
##########
@@ -95,7 +95,8 @@ public Filter parseFilter(String className, Element filterElement, XmlConfigurat
}
public Filter parseFilter(String className, String filterPrefix, Properties props, PropertiesConfiguration config) {
- PluginType<?> plugin = plugins.get(className.toLowerCase());
+ Objects.requireNonNull(className, "className");
+ PluginType<?> plugin = plugins.get(className.toLowerCase().trim());
Review comment:
NPE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#issuecomment-1046122291
Hi @ppkarwasz
I did this a bit differently in release-2.x, please verify your use case.
TY!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#discussion_r810179530
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java
##########
@@ -71,7 +71,7 @@ public Appender parseAppender(String name, String className, String prefix, Stri
String filterPrefix, Properties props, PropertiesConfiguration config) {
Objects.requireNonNull(plugins, "plugins");
Objects.requireNonNull(className, "className");
- PluginType<?> plugin = plugins.get(className.toLowerCase());
+ PluginType<?> plugin = plugins.get(className.toLowerCase().trim());
Review comment:
AFAIK `toLowerCase()` does not return nulls. :-) Besides, `className` is protected by an `Objects.requireNonNull`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#issuecomment-1046188788
@garydgregory: I tested the `Log4j1ConfigurationConverter` with a configuration full of spaces and it worked correctly, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz closed pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
ppkarwasz closed pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#discussion_r810157109
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java
##########
@@ -118,7 +119,8 @@ public Layout parseLayout(String className, Element layoutElement, XmlConfigurat
return null;
}
public Layout parseLayout(String className, String layoutPrefix, Properties props, PropertiesConfiguration config) {
- PluginType<?> plugin = plugins.get(className.toLowerCase());
+ Objects.requireNonNull(className, "className");
+ PluginType<?> plugin = plugins.get(className.toLowerCase().trim());
Review comment:
NPE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#discussion_r810156532
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java
##########
@@ -71,7 +71,7 @@ public Appender parseAppender(String name, String className, String prefix, Stri
String filterPrefix, Properties props, PropertiesConfiguration config) {
Objects.requireNonNull(plugins, "plugins");
Objects.requireNonNull(className, "className");
- PluginType<?> plugin = plugins.get(className.toLowerCase());
+ PluginType<?> plugin = plugins.get(className.toLowerCase().trim());
Review comment:
This could cause an NPE, see the bull check on the line below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #762: Trim property values
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #762:
URL: https://github.com/apache/logging-log4j2/pull/762#discussion_r810181303
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java
##########
@@ -159,7 +159,7 @@ private void warn(final String string) {
}
private void buildAppender(final String appenderName, final String appenderClass) {
- switch (appenderClass) {
+ switch (appenderClass.trim()) {
Review comment:
@garydgregory: I double checked that this private method is never called with a null `appenderClass`, but can you check again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org