Skip to content
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

LOG4J2-2091: Log4j respects the configured "log4j2.is.webapp" property #120

Closed
wants to merge 1 commit into from

Conversation

carterkozak
Copy link
Contributor

Some applications run embedded servlet containers sharing
a single logging context from the root application.

@jvz
Copy link
Member

jvz commented Oct 26, 2017

This PR looks good, though could you associate this with a JIRA ticket? Also, a unit test would be great to help avoid this bug from resurfacing in the future. Thanks!

@carterkozak carterkozak changed the title Log4j respects the configured "log4j2.is.webapp" property LOG4J2-2091: Log4j respects the configured "log4j2.is.webapp" property Oct 26, 2017
@carterkozak
Copy link
Contributor Author

Hi @jvz, thanks for the quick reply!

I've added a jira ticket, but I'm not sure the best way to add real tests for this given how this is configured as static state. Any chance you could point me in the right direction?

@jvz
Copy link
Member

jvz commented Oct 26, 2017

I think you could add a simple test in log4j-web that asserts Constants.IS_WEB_APP if there isn't one already.

Some applications run embedded servlet containers sharing
a single logging context from the root application.
@carterkozak
Copy link
Contributor Author

Updated, thanks for the feedback!

@jvz
Copy link
Member

jvz commented Oct 26, 2017

Looks great, thanks! Anyone else on the team is welcome to merge this as I won't be around an appropriate computer until tomorrow at the earliest.

@asfgit asfgit closed this in fee2043 Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants