This repository has been archived by the owner on Jan 19, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would much rather FuelSDK be updated to require Java 8 (in which case it should be 2.17.0), but if it is still to be Java 6, then from Log4J page: https://logging.apache.org/log4j/2.x/index.html, quote: "Mitigation Upgrade to Log4j 2.3.1 (for Java 6), 2.12.3 (for Java 7), or 2.17.0 (for Java 8 and later)."
If we use 2.3.1 here in order to maintain JDK 6 support, downstream users of this library using JDK 8+ will probably be able to override it to 2.17.x anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmazza Many thanks for your suggestion! I agree, I would also rather FuelSDK be upgraded to a newer Java version, but on the other hand, some people rely on it running on Java 6. Using log4j 2.3.1 for continued compatibility might be the right call here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmazza However, this change alone wouldn't be enough: I introduced the Log4j 1.2 API bridge to keep the amount of required changes low. This package however, is not compatible with Java 6 (afaik).
I see two possible paths here:
I feel like the first option might be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roechi It looks like that JDK 6 declaration was not used in a release version of this application (i.e., 1.5.0). The last patch done in February bumped the release version to 1.5.1 and it was that version that declared JDK 6. It doesn't appear 1.5.1 was ever released.
I put in a support ticket with Marketing Cloud for them to look at this patch but they rather quickly closed it, they're firm that this is a community project and they don't provide official support for it. From what I can see, the last release was back in 2017 and this code has gotten quite old.
Anyway, my fork: https://github.com/gmazza/FuelSDK-Java is a copy of what we're testing at work, it uses the very latest CXF 3.5.0 released this month instead of a 2015 version in the main Salesforce branch, and latest 2.17.1 of Log4J. Code is quite new but initial testing is indicating that for obvious functionality it seems to be doing its job (I expect some tripping up sooner rather than later, however, I'll try to keep my fork updated as I find things.) There are numerous @ignores I needed to add to the test cases however, I suspect the API in Salesforce has changed for several of them (meaning even the main branch would have these failures), also that I might not have provided all needed configuration from the fuelsdk.properties.template file. Feel free to fork, see the fuelsdk.properties.template file on config needed to run tests via mvn clean install, and if you have a local Artifactory repository you can configure the pom.xml for them and do "mvn clean deploy" to install your local version and work with that.