Updated Log4J from version 1.2.17 to version 2.24.3 #385
Conversation
GeorgeBlackwell
requested review from
chrisbloe-nhse and
shul1
and removed request for
chrisbloe-nhse
|
Presuming the update and change of packaging to log4j-core , is like going from .net Framework -> .Net core ? hence the namespacing changes within some of the logging calls ? |
@grant-dot-dev I've got very limited Java knowledge but yes I think that's correct. It looks like they made the move a while back. |
| @Override | ||
| public BaseServerResponseException preProcessOutgoingException(RequestDetails theRequestDetails, | ||
| Throwable theException, HttpServletRequest theServletRequest) throws ServletException { | ||
| Throwable theException, HttpServletRequest theServletRequest) throws ServletException { |
There was a problem hiding this comment.
I'd suggest formatting the arguments like this once they start to get too long. Not enough for me to reject the PR but one to be aware of.
public BaseServerResponseException preProcessOutgoingException(
RequestDetails theRequestDetails,
Throwable theException,
HttpServletRequest theServletRequest
) throws ServletException {
There was a problem hiding this comment.
Just one comment about argument structuring but otherwise I'm happy with it. It's probably too big of a change for this codebase, but I'd suggest moving to Spring's @Slf4j or even @Log4j2 annotations rather than creating a LogManager object everywhere it's needed, but it's not a dealbreaker.
I mirrored the changes by Lillie in this PR -> #383