[WIP] Replace log4j with slf4j

[hprange]

As promised, I'm replacing the log4j dependency with slf4j in Wonder. It's still a work in progress. Anyway, I'd like to share my findings and hear from the community.

1. What has been done so far?

• 1.1) Most references to org.apache.log4j.Logger were replaced by org.slf4j.Logger.
• 1.2) All fatal log messages were replaced by error.
• 1.3) The AjaxRemoteLogging component doesn't support the log level fatal anymore. In contrast, trace was added as an option.
• 1.4) The log level configuration was removed from theWOHelperFunctionDeclarationParser, WOHelperFunctionHTMLParser and WOHelperFunctionTagRegistry classes. slf4j doesn't support configuring the log level in runtime.
• 1.5) For the same reason, I removed the ability to change the log level in runtime from the ERD2WDebugFlags.toggleD2WInfo and ERDDebuggingHelp.toggleRuleTracing methods. I'm not entirely satisfied with this change.

2. Open Issues

• 2.1) ERXApplication re-inits the log4j console appenders, so we get logging into WOOutputPath again. Does anybody remember what issue this code fixes? ✅
• 2.2) Wonder has an ERXLogger class that works as a facade. I'm still investigating if we can replace it with the slf4j Logger class. ✅
• 2.3) Wonder has an ERXNSLogLog4jBridge class that works as a bridge between NSLog and log4j. We can create an ERXNSLogSlf4jBridge class to make the bridge between NSLog and slf4j. ✅
• 2.4) The ERD2WPage component on ERDirectToWeb uses the org.apache.log4j.NDC class. We should migrate it to use the org.slf4j.MDC class instead. ✅
• 2.5) The ImageMagickCommandlineMetadataParser class on ERAttachment uses org.apache.log4j.lf5.util.StreamUtils. We can replace it by a more general dependency (commons-io?). ✅
• 2.6) ERXDirectAction.log4jAction is an action that opens the ERXLog4JConfiguration component and allows to change log4j configuration in runtime. I'm thinking of leaving it as it is. The application will throw a ClassNotFoundException if somebody uses it and the log4j library is not in the classpath. Any thoughts?
• 2.7) The StandaloneRunner class of the ERSelenium uses log4j extensively to configure the logging behavior of the selenium runner. I'm thinking of leaving it as it is.
• 2.8) The following classes extend a class from log4j1. We can leave it as it is.
  • ERXThreadStorageAppender extends org.apache.log4j.AppenderSkeleton.
  • ERXPatternLayout extends org.apache.log4j.PatternLayout.
  • ERXMailAppender extends org.apache.log4j.AppenderSkeleton.
  • ERXConsoleAppender extends org.apache.log4j.ConsoleAppender.
  • ERXEOFAppender extends org.apache.log4j.AppenderSkeleton.
• 2.9) Copy the WOCGIFormValues class into the ERExtensions framework and change it to reference the org.slf4j.Logger interface. ✅
• 2.10) Applications like wotaskd and JavaMonitor still use log4j. We could replace it with log4j2. ✅

3. Open Questions

• 3.1) Should we include sample/default log configuration files for the most common log libraries? I'm considering log4j2 and logback in addition to the existing log4j1 configuration.

Considerations

• log4j is now an optional dependency. Users must explicitly declare it on their poms to keep using log4j1.
• Besides that, these changes shouldn't affect projects using log4j1.
• I'm working only on frameworks built by Maven. I didn't touch projects in the following folders:
  • Applications
  • Archives
  • Tests

I would appreciate it if you could look at the proposed changes and help me with the open issues. Your thoughts are always welcome!

[hprange]

BTW, I'm focusing on issues 2.2 and 2.3 now.

[lbane]

Regarding points 1.4 and 1.5 I wonder if it would be useful to have an abstraction with different implementations for supported loggers, maybe even as different frameworks. I would then move the remaining log4j depended classes to the corresponding framework.

[hprange]

Regarding points 1.4 and 1.5 I wonder if it would be useful to have an abstraction with different implementations for supported loggers, maybe even as different frameworks. I would then move the remaining log4j depended classes to the corresponding framework.

That's certainly an option worth pursuing. I can add support for log4j1 to keep the same behavior, and we may add implementation for other frameworks in the future.

[hprange]

I found two unmodifiable classes that require log4j.

The com.webobjects.appserver._private.WOCGIFormValues.Encoder class of the JavaWebObjects framework creates a org.apache.log4j.Logger. Here is a list of classes depending on WOCGIFormValues:

• com.webobjects.appserver.WOApplication.responseForDirectActionWithNameAndClass
• com.webobjects.appserver.WOContext._directActionURL
• com.webobjects.appserver.WORequest.getFormValuesFromURLEncoding
• com.webobjects.appserver._private.WOHyperlink._appendQueryStringToResponse
• er.extensions.components.ERXLinkButton._appendQueryStringToResponse
• er.extensions.components.ERXLinkButton5._appendQueryStringToResponse
• er.extensions.foundation.ERXHyperlinkResource._appendQueryStringToResponse

Since we can't modify some of these classes, I'm willing to copy the WOCGIFormValues class into the ERExtensions framework and change it to reference the org.slf4j.Logger interface. I've added item 2.9 to tackle it.

The com.webobjects.foundation._NSFileUtilities class of the ERFoundation framework also creates a org.apache.log4j.Logger. However, I couldn't find any references in Wonder or WebObjects. Until proven otherwise, we can leave it alone.

[lbane]

I found two unmodifiable classes that require log4j.

Doesn't slf4j have stub implementations for those cases (e.g. log4j-over-slf4j.jar)? If the really used logging framework is anything other than log4j (or reload4j), than putting log4j-over-slf4.jar in the path should solve the problem.

[hprange]

Doesn't slf4j have stub implementations for those cases (e.g. log4j-over-slf4j.jar)? If the really used logging framework is anything other than log4j (or reload4j), than putting log4j-over-slf4.jar in the path should solve the problem.

I did not know about the log4j-over-slf4j bridge. Thanks for sharing. It's an interesting band-aid. Here is my take after reading the docs. It may solve the problem of WOCGIFormValues referencing org.apache.log4j.Logger without requiring any funky code decompilation—which is a good thing. However, it doesn't solve Wonder's entire "migrate log4j to slf4j" issue and introduces another level of complexity. Wonder users must become familiar with using the log4j-over-slf4j bridge. They can't, for instance, use it with log4j-slf4j-impl.jar or slf4j-reload4j.jar.

Since we're talking about only one class, I still prefer the decompilation course of action (even though it hurts my soul profoundly). It's one less thing to explain/document/break and one less step in the migration path. Anyway, I'd like to hear from others about their take on this subject.

[paulhoadley]

Since we're talking about only one class, I still prefer the decompilation course of action (even though it hurts my soul profoundly). It's one less thing to explain/document/break and one less step in the migration path. Anyway, I'd like to hear from others about their take on this subject.

I feel your pain, but I support this approach. It's not like this is the first time Wonder will be decompiling a legacy class—we're already doing this in several other places.

Meanwhile, I had almost forgotten an issue that @renebock just brought up in #971: JavaXML.framework, among other travesties of software engineering, has classes baked in from an ancient version of Log4j 1.x. We're going to have to address this somehow as well, aren't we?

[hprange]

Meanwhile, I had almost forgotten an issue that @renebock just brought up in #971: JavaXML.framework, among other travesties of software engineering, has classes baked in from an ancient version of Log4j 1.x. We're going to have to address this somehow as well, aren't we?

Yes, you're correct. It's not an easy fix, though. JavaXML is a transitive dependency of the JavaWebObjects library (as you can see in the pom.xml). We can change it and replace the JavaXML dependency with a set of genuine dependencies: axis, log4j, etc. However, it's hard to propagate this change using the same version (5.4.3). Anybody that has downloaded the JavaWebObjects jar at some point would have to remove the dependency from their local repository and download it again. The same applies to repositories used by CI/CD pipelines or proxy repositories.

To be honest, I haven't given this subject much thought. I never bothered about the JavaXML dependency because I've been excluding it from my projects for a long time. I even use the maven-enforcer-plugin to entirely ban the JavaXML dependency (the build fails if I forget to exclude it).

What alternatives do we have?

[hprange]

What alternatives do we have?

I'll check if we could use the Bill of Materials (BOM) to group all WebObjects and Wonder dependencies, excluding the JavaXML transitive dependency.

[hprange]

It looks like a wonder-bom may solve the JavaXML dependency issue. 😀 I'll try to elaborate a little bit better tomorrow.

[renebock]

2.5) The ImageMagickCommandlineMetadataParser class on ERAttachment uses org.apache.log4j.lf5.util.StreamUtils. We can replace it by a more general dependency (commons-io?).

I would go for org.apache.commons.io.IOUtils which has a copy function with the same signature.

[hprange]

I pushed a few changes to this branch:

• Added toggle rule tracing back while debugging D2W apps in development mode. It works with log4j 1, log4j 2, and logback.
• log4j dependency has been removed from the ImageMagickCommandlineMetadataParser class according to @renebock recommendation.
• JavaXML has been removed as a direct dependency of all frameworks.
• Added a wonder-bom module to group all WebObjects and Wonder dependencies into one pom.xml. It also excludes JavaXML as a transitive dependency of the JavaWebObjects dependency.

I've deployed version 7.4-SNAPSHOT to the WOCommunity repository containing all the changes from this pull request.

The best way to test it is to import the wonder-bom
as a dependency in the dependencyManagement section of your project's parent pom, as illustrated below:

<dependencyManagement>
    <dependencies>
        <dependency>
            <groupId>wonder</groupId>
            <artifactId>wonder-bom</artifactId>
            <version>7.4-SNAPSHOT</version>
            <type>pom</type>
            <scope>import</scope>
        </dependency>
    </dependencies>
</dependencyManagement>

See this guide for more information.

You can then remove the version of your WebObjects and Wonder dependencies. The wonder-bom will also exclude the JavaXML as a transitive dependency of the JavaWebObjects framework.

What you can expect from this version:

• Your project should log the same messages if you use log4j 1 (you may have to declare the log4j 1 dependency in your pom.xml).
• Your project should log the same messages with logback. Don't forget to add a logback.xml file to the classpath and configure it properly.
• Your project should log the same messages with log4j 2. Don't forget to add a log4j2.properties or log4j2.xml to the classpath and configure it properly.

You must configure your log messages according to the selected logging library since there's no default log configuration for other log libraries besides log4j 1.

I'm still not happy with the resulting wonder-bom because it includes other dependencies in addition to the WebObjects and Wonder dependencies. I'm working on it.

Please, give it a try and let me know your first impressions.

[renebock]

Hi @hprange: are there an plans to merge this PR into master?

[renebock]

@hprange wrote:

I've deployed version 7.4-SNAPSHOT to the WOCommunity repository containing all the changes from this pull request.

I fear that the the resources of the current snapshot don't match the code in the pull request.

e.g the repositories version of ERXGenericRecord imports org.apache.log4j.Logger; where as the version in your PR imports org.slf4j.Logger and org.slf4j.LoggerFactory;

After excluding log4j 1.x in my pom.xml dependencies, I got some very nasty compiler null pointer exceptions :-(

[hprange]

Hi @hprange: are there an plans to merge this PR into master?

@renebock I've been using the changes from this PR in a large-scale app for a while without problems. IMHO, if we put the discussion about dropping the Ant build support aside, this pull request is ready to be merged.

I fear that the the resources of the current snapshot don't match the code in the pull request.

That's correct. GitHub Actions deploy a new SNAPSHOT every time there's a new commit into master.

[renebock]

IMHO, if we put the discussion about dropping the Ant build support aside, this pull request is ready to be merged.

I would really appreciate if the 'dropping ant build support' will be split into an PR dedicated to this task.

[renebock]

Regarding topics 2.6 and 2.8: shouldn't these classes not be moved into separate frameworks (eg. ERXLog4jExtensions, ERXLog4j2Extensions, ERXLogbackLoggingExtensions) in order to keep ERXExtensions clean from dedicated logger implementations?