You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "JWT007 (via GitHub)" <gi...@apache.org> on 2025/01/08 00:04:17 UTC
[PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
JWT007 opened a new pull request, #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372
In LoggerConfig.RootLogger.Builder:
* Deprecated method with typo in name 'withtFilter'.
* Added correct method 'withFilter'.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#discussion_r1957283823
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java:
##########
@@ -945,11 +944,20 @@ public Filter getFilter() {
return filter;
}
+ /**
+ * @deprecated Use {@link #withFilter(Filter)} instead
+ */
+ @Deprecated
public B withtFilter(final Filter filter) {
this.filter = filter;
return asBuilder();
}
+ public B withFilter(final Filter filter) {
Review Comment:
Since we settled on using "setters" for new builder methods instead of "withers" (see e.g. #1206), could you change it to `setFilter`?
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2978919225
@norrisjeremy,
The overall goal is to transition all withers to setters in version 3.x, as described in #1206. Since the builders are mutable, the purpose is to use a naming pattern more common for mutable objects.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2979985822
Hi @norrisjeremy,
Thanks for your thoughtful and detailed questions—these are all excellent points, and I really appreciate you taking the time to raise them.
> 1. So why weren't all the other wither methods replaced with setters and marked as deprecated too?
You're absolutely right to highlight the inconsistency. The focus of the PR was to correct a specific issue with `withtFilter` (with an additional `t`), and the introduction of `setFilter` was a side-effect driven by the fact that `withFilter` is already deprecated in `3.x`. So it made sense to align it preemptively here.
Replacing all wither methods in the same Log4j Core plugin—or across all plugins—would indeed be more consistent. But that would have significantly expanded the scope of this PR. Since there are around 300 plugins, we felt it was better to handle that broader change separately. To address this, I’ve opened issue #3750 to track the work of adding setters and deprecating all remaining withers. If you're interested, contributions are always welcome!
> 2. And why wasn't [issue #1206](https://github.com/apache/logging-log4j2/issues/1206) referenced in the PR?**
It was actually mentioned in a resolved conversation here: https://github.com/apache/logging-log4j2/pull/3372#discussion_r1957283823
> 3. Why wasn’t this change documented in the `Deprecated` section of the Release Notes?
That’s a fair point. When preparing release notes, we tend to prioritize high-impact changes for the majority of users, most of whom use Log4j via configuration files. The Log4j Core internal API is mostly used by Log4j Plugin developers and integrators. As a result, API-level changes like this one can get less visibility, especially if they're part of a narrowly scoped fix.
That said, you're absolutely right: this deprecation should have been classified as `deprecated`, not just `changed`. Feel free to submit a PR correcting that in the release notes—those are always welcome, even after the release, and can help future users.
> From a user's perspective, this change seems poorly implemented and documented, which is extremely frustrating.
I hear you, and I’m sorry the process came across that way. We always aim to provide a stable and predictable experience, and clearly we can improve how we communicate these kinds of changes.
Out of curiosity—what kind of impact did this deprecation have on your application? Was it mainly the compiler warnings, or something else? Understanding that might help us improve how we handle and communicate such changes going forward.
Thanks again for your feedback—it’s valuable and helps us improve the project for everyone.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2577163897
@JWT007,
Can you add a changelog entry (of type `added`)?
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "JWT007 (via GitHub)" <gi...@apache.org>.
JWT007 commented on code in PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#discussion_r1957397458
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java:
##########
@@ -945,11 +944,20 @@ public Filter getFilter() {
return filter;
}
+ /**
+ * @deprecated Use {@link #withFilter(Filter)} instead
+ */
+ @Deprecated
public B withtFilter(final Filter filter) {
this.filter = filter;
return asBuilder();
}
+ public B withFilter(final Filter filter) {
Review Comment:
@ppkarwasz
I changed the LoggerConfig.RootLogger.Builder#withFilter to #setFilter.
The change of LoggerConfig.Builder#withFilter was not part of my original change, but I have now deprecated that too and added a 'setFilter' there as well. I couldn't change the name of the existing #withFilter there without triggering a major baseline change.
I updated the tests using the #createLoggerConfig. Fixed other code in same test methods to use builders as well (i.e. FileAppender).
NOTE I did not update test 'JiraLog4j2_2134Test' since it seems to *explicitly* be testing deprecated methods.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2577172337
<table>
<tr>
<th>Job</th>
<th>Requested goals</th>
<th>Build Tool Version</th>
<th>Build Outcome</th>
<th>Build Scan®</th>
</tr>
<tr>
<td>build-macos-latest</td>
<td>clean install</td>
<td align='center'>3.9.8</td>
<td align='center'>:x:</td>
<td><a href="https://ge.apache.org/s/7oi2ykri5rzca" rel="nofollow"><img src="https://img.shields.io/badge/Build%20Scan%C2%AE-PUBLISHED-06A0CE?logo=Gradle" alt="Build Scan PUBLISHED" /></a></td>
</tr>
<tr>
<td>build-ubuntu-latest</td>
<td>clean install</td>
<td align='center'>3.9.8</td>
<td align='center'>:x:</td>
<td><a href="https://ge.apache.org/s/iiujdmsikzor6" rel="nofollow"><img src="https://img.shields.io/badge/Build%20Scan%C2%AE-PUBLISHED-06A0CE?logo=Gradle" alt="Build Scan PUBLISHED" /></a></td>
</tr>
<tr>
<td>build-windows-latest</td>
<td>clean install</td>
<td align='center'>3.9.8</td>
<td align='center'>:x:</td>
<td><a href="https://ge.apache.org/s/qaxxccp6qw3zk" rel="nofollow"><img src="https://img.shields.io/badge/Build%20Scan%C2%AE-PUBLISHED-06A0CE?logo=Gradle" alt="Build Scan PUBLISHED" /></a></td>
</tr>
</table>
###### Generated by gradle/develocity-actions
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "JWT007 (via GitHub)" <gi...@apache.org>.
JWT007 commented on code in PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#discussion_r1957369751
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java:
##########
@@ -945,11 +944,20 @@ public Filter getFilter() {
return filter;
}
+ /**
+ * @deprecated Use {@link #withFilter(Filter)} instead
+ */
+ @Deprecated
public B withtFilter(final Filter filter) {
this.filter = filter;
return asBuilder();
}
+ public B withFilter(final Filter filter) {
Review Comment:
@ppkarwasz all the other methods are withers which is why I stuck with it. Should I add only the one setter or all?
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "norrisjeremy (via GitHub)" <gi...@apache.org>.
norrisjeremy commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2978292816
> We should use `setFilter` instead of `withFilter`.
>
> I also noticed that **none** of our tests use the `LoggerConfig` builder and they call the deprecated `LoggerConfig#createLogger` method instead. Could you fix this?
@ppkarwasz
What is the justification for deprecating `withFilter()`?
Nothing else in this builder class is configured with a setter, but rather they are all withers, so this seems to introduce an API inconsistency.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2984789617
Hi @norrisjeremy,
> To answer your question about the impact: we deliberately run compiler warnings & deprecations set to errors for our internal apps, so that they immediately cause a compilation failure, in order for our team to notice and deal with API deprecations immediately and avoid having them pile up over time.
That’s a great policy—seriously! 👏 It’s not something we see often; more typically, teams tend to tolerate deprecated methods and configuration properties until something breaks. Because of that, we don't usually factor the impact of deprecations into our decision-making. So, thank you for calling attention to your approach—it’s helpful context for us going forward.
> And then when trying to load up the latest Javadocs, noticed that they had not yet been published on the website. (And in fact updated Javadocs are still not published, as all the pages linked [here](https://logging.apache.org/log4j/2.x/javadoc.html) lead to 404 page not found errors...)
That was ironically caused by a recent change in the Maven Javadoc Plugin (apache/maven-javadoc-plugin#1163). While it improved consistency, it also broke our site build :wink:. This has now been fixed—thanks for bringing it to our attention!
> On top of this, this is the second time in past year that this API method has been touched. In the 2.24.0 release, the original `withtFilter()` was marked as deprecated in lieu of the new `withFilter()` method (see #788).
Yes, good catch. That change was made in 21ad7be2f7f08118cc5022c8e6ce2e4555b008d4, back when we still allowed Commit-then-Review for “trivial” changes. That likely contributed to us forgetting about the "review" part of the process. We've since moved to a strict Review-then-Commit policy for all changes, which should help avoid similar oversights in the future.
> So all told, this is just frustrating: it results in lost time & productivity for what is amounting to whack-a-mole operation, chasing the whims of upstream library developers: deprecating and changing withers to setters in a Builder class just doesn't seem to offer tangible benefit to any existing users.
Totally understandable. This change reflects an effort to standardize the API and address inconsistencies that date back to the early development of Log4j 2. That said, I agree that the change has only an aesthetic effect. I don't want to leave things in the middle, so I’ve scheduled #3750 for the `2.26.0` milestone, but we will deprecate all remaining "withers" in a single go.
As mentioned, we assume most users rely on configuration files—which are unaffected by these changes. But for those constructing configurations programmatically, we recommend using [`ConfigurationBuilder`](https://logging.apache.org/log4j/2.x/manual/customconfig.html#ConfigurationBuilder). It's a more stable and future-proof approach, unlike directly using plugin builders, which can be more volatile.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#discussion_r1959377828
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java:
##########
@@ -945,11 +944,20 @@ public Filter getFilter() {
return filter;
}
+ /**
+ * @deprecated Use {@link #withFilter(Filter)} instead
+ */
+ @Deprecated
public B withtFilter(final Filter filter) {
this.filter = filter;
return asBuilder();
}
+ public B withFilter(final Filter filter) {
Review Comment:
> Should I add only the one setter or all?
As you prefer, you can replace all "withers" in `LoggerConfig.Builder` with "setters".
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "norrisjeremy (via GitHub)" <gi...@apache.org>.
norrisjeremy commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2980153817
Hi @ppkarwasz,
Thank you for your detailed response.
To answer your question about the impact: we deliberately run compiler warnings & deprecations set to errors for our internal apps, so that they immediately cause a compilation failure, in order for our team to notice and deal with API deprecations immediately and avoid having them pile up over time.
So we immediately noticed the deprecation when a Dependabot PR for one of our internal apps failed to build yesterday.
Upon seeing the deprecation warning, we first turned to the release notes for the new 2.25.0 release, but did not note any mention of the `withFilter()` method being deprecated.
And then when trying to load up the latest Javadocs, noticed that they had not yet been published on the website.
(And in fact updated Javadocs are still not published, as all the pages linked [here](https://logging.apache.org/log4j/2.x/javadoc.html) lead to 404 page not found errors...)
This then led us to having to go dumpster diving through Github, to determine how to handle the deprecation, as well as why it had been occurred.
On top of this, this is the second time in past year that this API method has been touched.
In the 2.24.0 release, the original `withtFilter()` was marked as deprecated in lieu of the new `withFilter()` method (see #788).
So all told, this is just frustrating: it results in lost time & productivity for what is amounting to whack-a-mole operation, chasing the whims of upstream library developers: deprecating and changing withers to setters in a Builder class just doesn't seem to offer tangible benefit to any existing users.
Thanks,
Jeremy
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "norrisjeremy (via GitHub)" <gi...@apache.org>.
norrisjeremy commented on PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#issuecomment-2978994220
> @norrisjeremy,
>
> The overall goal is to transition all withers to setters in version 3.x, as described in #1206. Since the builders are mutable, the purpose is to use a naming pattern more common for mutable objects.
@ppkarwasz,
1. So why weren't all the other wither methods replaced with setters and marked as deprecated too?
2. And why wasn't #1206 mentioned at all in this PR in the first place?
3. And why wasn't this change documented in the `Deprecated` section of the [Release Notes](https://logging.apache.org/log4j/2.x/release-notes.html)?
From a user's perspective, this change seems poorly implemented and documented, which is extremely frustrating.
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) [logging-log4j2]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz merged PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372
--
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
Re: [PR] Fix typo in LoggerConfig.RootLogger.Builder#withtFilter (#3369) (logging-log4j2)
Posted by "JWT007 (via GitHub)" <gi...@apache.org>.
JWT007 commented on code in PR #3372:
URL: https://github.com/apache/logging-log4j2/pull/3372#discussion_r1957369285
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/LoggerConfig.java:
##########
@@ -945,11 +944,20 @@ public Filter getFilter() {
return filter;
}
+ /**
+ * @deprecated Use {@link #withFilter(Filter)} instead
+ */
+ @Deprecated
public B withtFilter(final Filter filter) {
this.filter = filter;
return asBuilder();
}
+ public B withFilter(final Filter filter) {
Review Comment:
sure... will do
--
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