SebTardif commented on code in PR #4172:
URL: https://github.com/apache/logging-log4j2/pull/4172#discussion_r3615060111
##########
log4j-1.2-api/src/test/java/org/apache/log4j/config/XmlConfigurationTest.java:
##########
@@ -99,6 +101,23 @@ void testXML() throws Exception {
assertTrue(file.length() > 0, "File A2 is empty");
}
+ /**
Review Comment:
@ramanathan1504
> here too and rest of the file also
Done. Removed the test javadoc and inline comments on
`testInvalidAppenderDoesNotNpe`.
##########
log4j-1.2-api/src/main/java/org/apache/log4j/xml/XmlConfiguration.java:
##########
@@ -774,7 +774,23 @@ private void parse(Element element) {
break;
case APPENDER_TAG:
final Appender appender = parseAppender(currentElement);
- appenderMap.put(appender.getName(), appender);
+ if (appender == null) {
+ // parseAppender already logs the creation failure;
warn at this
Review Comment:
@ramanathan1504
> Can you remove these comments?
Done. Removed the explanatory comments around the `appender == null` guard
in `XmlConfiguration`.
##########
src/changelog/.2.x.x/fix_xml_parse_appender_npe.xml:
##########
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="fixed">
+ <issue id="4172" link="https://github.com/apache/logging-log4j2/pull/4172"/>
Review Comment:
@ramanathan1504
> need to add your `PR` number too. please refer other change log
The changelog entry already includes the PR:
```xml
<issue id="4172" link="https://github.com/apache/logging-log4j2/pull/4172"/>
```
That matches other single-PR entries (for example #4125 / #4127). There is
no separate issue number for this change. Happy to adjust the format if you
prefer a different layout.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
