Skip to content

[Misc] Fix logging best practices violations in the rendering modules - #397

Merged
vmassol merged 1 commit into
masterfrom
misc-fix-logging-best-practices
Aug 1, 2026
Merged

[Misc] Fix logging best practices violations in the rendering modules#397
vmassol merged 1 commit into
masterfrom
misc-fix-logging-best-practices

Conversation

@vmassol

@vmassol vmassol commented Aug 1, 2026

Copy link
Copy Markdown
Member

Jira URL

N/A — [Misc] commit, mechanical code-quality fixes that do not warrant a JIRA issue.

Changes

Description

Audit of every logging call site under src/main/java (33 of them) against the
Logging Best Practices,
and fix the violations found.

  • Do not print a stack trace in a warning. RenderingContextStore was passing the exception as
    the trailing SLF4J argument, which SLF4J interprets as a throwable and logs as a full stack trace.
    Stack traces are reserved for error; log ExceptionUtils.getRootCauseMessage() instead.

  • Use the parameterized SLF4J signature instead of string concatenation. IconTransformation
    was building its message with +, and used e.getMessage() rather than the root cause message.

  • Surround parameters with []. The trailing placeholder of the MacroTransformation debug
    message was bare (: {}). Reformatting it also brought that line back under 120 characters (it
    was 126).

  • Log as much information as possible. Four error calls logged only a bare sentence plus a
    stack trace, with no way to tell which macro / syntax / resource type failed:

    Location Parameter added
    MacroTransformation#prepare(Block) syntax
    MacroTransformation#prepare(Block, Syntax) macro id + syntax
    DefaultResourceReferenceParser#parse resource type prefix
    DefaultXHTMLLinkRenderer#getXHTMLLinkTypeRenderer resource type scheme

Clarifications

  • In MacroTransformation#prepare(Block) the syntax is logged rather than the block itself: that
    catch handles a StackOverflowError, and AbstractBlock#toString() recurses through all
    children, so rendering the block could overflow the stack again while building the message.
  • No test or resource file asserts on any of the changed strings, so no test updates are needed.
  • Verified clean, no change needed: no placeholder/argument-count mismatches at any call site; all
    components inject Logger and non-components use private static final Logger LOGGER; no
    double-bracket cases (the Block parameters in PutFootnotesMacro render as CompositeBlock:[…],
    so the leading [ is not doubled); the four
    BlockStateChainingListener debug(msg, new IllegalStateException()) calls are already guarded by
    isDebugEnabled().
  • Left alone deliberately, out of scope for this PR:
    • XWikiSerializer2 uses java.util.logging.Logger instead of SLF4J. The field is never actually
      used to log, but it is exposed through public getLogger()/setLogger(), so replacing it is a
      breaking API change needing deprecation, a legacy re-add and a Revapi ignore.
    • "Failed to initialize the default WikiModel implementation" (BlockNoteRenderer,
      AttachmentXHTMLImageTypeRenderer) is inaccurate — nothing is initialized, it is a
      ComponentLookupException from looking up whichever WikiModel is registered, and there is no
      "default implementation". The identical wording exists in xwiki-platform, so changing it here
      only would desynchronize the two.
    • The cause suffix is inconsistent repo-wide (Cause: [{}] in most places, Cause [{}]. in
      ListenerRegistry, Root reason [{}] in DefaultTransformationManager). Purely cosmetic and
      not a rule breach.

Screenshots & Video

N/A — no UI change.

Executed Tests

mvn -B -ntp install \
  -pl xwiki-rendering-api,\
xwiki-rendering-transformations/xwiki-rendering-transformation-icon,\
xwiki-rendering-transformations/xwiki-rendering-transformation-macro,\
xwiki-rendering-syntaxes/xwiki-rendering-syntax-xhtml

All four modules SUCCESS — tests, Checkstyle and JaCoCo all green.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None — cosmetic logging cleanup on master only.

🤖 Generated with Claude Code

* Do not print a stack trace in a warning: RenderingContextStore was passing
  the exception as the trailing SLF4J argument, which logs a full stack trace.
  Log the root cause message instead.
* Use parameterized logging instead of string concatenation in
  IconTransformation, and log the root cause rather than the direct message.
* Surround log parameters with brackets: the trailing placeholder of the
  MacroTransformation debug message was not wrapped.
* Log the identifying parameter (macro id, syntax, resource type) in the error
  messages that previously logged only a bare sentence plus a stack trace.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vmassol vmassol self-assigned this Aug 1, 2026
@vmassol
vmassol merged commit 989303b into master Aug 1, 2026
2 checks passed
@vmassol
vmassol deleted the misc-fix-logging-best-practices branch August 1, 2026 09:58
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.

1 participant