Should Discourse's two BBCode rewriting approaches share a core API?

Development

AI-generated summary

The discussion explores whether two distinct BBCode rewriting approaches in open PRs should share a core API. Ethsim2 highlights that #43187 introduces BbcodeRewriter for automatic Event rollover, while #42932 uses a token-based PrettyText.update_bbcode_attributes for workflow actions. Although Martin suggested extracting generic functionality from the Event-specific service, Ethsim2 removed overlapping token-based code from #43187 to avoid duplication, noting the token API remains useful for #42932.

The post compares the implementations: BbcodeRewriter offers stronger runtime checks and identity validation suitable for automatic operations where errors could persist across rollovers, whereas the token-based approach selects the first matching block, which is sufficient for explicit workflow requests. Both share underlying infrastructure like parseBBCodeTag. Ethsim2 proposes a potential shared abstraction separating tag finding, attribute rewriting, and optional validation to allow simpler workflows to use straightforward selection while sensitive operations request stronger safeguards. The author asks maintainers if these APIs should remain separate due to differing target-selection requirements or converge into a shared lower-level rewriter with configurable validation, aiming to prevent unnecessary duplication in Discourse’s core BBCode infrastructure.

1

Two open PRs introduce different approaches to editing BBCode attributes in post raw:

During review of #43187, Martin suggested extracting the generic BBCode manipulation functionality from the Event-specific service.

I initially included some related token-based functionality from the other PR, but this introduced two rewriting mechanisms within #43187. Since the token-based API had no production caller in that branch, I removed it in commit 6d13371.

The token-based API remains independently useful to #42932.

I wondered whether these two approaches should eventually converge into one reusable core API, or whether their different requirements justify retaining both.

Comparison of the current implementations

The table describes what the code currently implements, rather than suggesting that either approach is inherently incorrect.

Capability BbcodeRewriter (#43187) Token-based API (#42932) Meaning and importance
1. Uses Discourse’s BBCode parser Scans candidate opening tags using parseBBCodeTag, the same opening-tag grammar used by the Markdown engine. Uses DiscourseMarkdownIt.parse to obtain Markdown tokens and find a matching BBCode block. Both approaches understand BBCode syntax, rather than relying solely on an independent regular expression. They differ in how they identify the tag to edit.
2. Selects the target BBCode block Temporarily marks candidate opening tags, renders the post, and identifies the candidate corresponding to a selected HTML element, such as div.discourse-post-event. Selects the first parsed BBCode token whose meta.bbcode matches the requested tag name. Target identity matters. #43187 needs to associate raw BBCode with the Event that Discourse actually rendered. #42932’s workflow uses the first matching parsed Event block.
3. Handles missing or ambiguous targets Raises TargetNotFound, AmbiguousTarget, or UnsupportedRenderer when a target cannot be identified reliably. Returns null if no matching parsed block exists; otherwise selects the first match. Failure behaviour differs. #43187 deliberately refuses ambiguous automatic edits. Selecting the first parsed block may be sufficient when the caller expects only one relevant block.
4. Verifies the raw-to-rendered mapping Renders marked content, checks the marker on the intended HTML element, and confirms that inserting and removing the marker leaves the rendered HTML unchanged. Uses the Markdown parser’s token metadata to locate the opening tag without an additional rendered-HTML identity check. This guards against editing the wrong rendered target. It is particularly relevant when a background job automatically archives an existing Event, but involves additional rendering and complexity.
5. Checks the selected opening tag Before rewriting, rescans the supplied raw and checks that the selected candidate still matches its expected opening-tag text. Locates the matching opening tag during the update call without a separate expected-opening comparison. This is an internal consistency check. It helps avoid rewriting a candidate that no longer matches the earlier selection. It is not, by itself, protection against concurrent database edits.
6. Handles attribute names and existing values Rewrites selected attributes using the parser’s attribute boundaries. It also normalises equivalent rendered attribute names, allowing variants such as Recurrence and recurrence to be removed consistently. Uses attribute-token matches to replace or remove requested names and the shared BBCode attribute serializer to insert values. Both aim to preserve unrelated attributes. Normalising equivalent names is especially important when archiving an Event, because leaving a recurrence alias behind could cause the completed Event to remain recurring.
7. Validates the rewritten tag Parses the rewritten opening tag again and checks that its attributes match the intended values. An invalid round-trip raises an error. Produces the rewritten opening tag directly, without an equivalent final parse-and-compare step. Round-trip validation catches serialization problems, such as changed attribute values or malformed boundaries. The token-based approach is simpler but relies on its rewriting logic and tests rather than this additional runtime check.
8. Preserves unrelated post content Changes the selected opening tag while retaining the surrounding raw content. It also checks that unrelated attribute values survive the rewrite. Replaces the selected opening-tag substring while retaining the surrounding raw content and attributes not requested for modification. Both need to avoid unintended post changes. An Event-related edit should not silently modify the surrounding discussion or unrelated BBCode attributes.
9. Implementation cost Requires candidate scanning, temporary markers, post rendering, HTML inspection, attribute rewriting and validation. Parses Markdown tokens, selects the first matching opening tag and rewrites it directly. Stronger runtime checks have a cost. The question is whether the extra safeguards should be available to all callers, optional, or implemented only where the application requires them.

Why the requirements differ

For #43187, successor rollover runs automatically when an occurrence finishes. The original Event is archived as a non-recurring occurrence, and a new Topic is created for the next occurrence.

If the wrong [event] block is modified, or the rewritten attributes no longer represent the intended Event, the error could persist across automatic rollover attempts.

For #42932, the workflow action explicitly requests that an Event be opened or closed. Its current token-based implementation modifies the first parsed matching Event block.

The token-based approach is therefore not necessarily unsuitable for that use case. It simply provides different selection and validation guarantees.

Both approaches already share some underlying infrastructure, including parseBBCodeTag and serializeBBCodeAttr.

Possible direction for a shared abstraction

One possibility might be to separate three responsibilities:

  1. Finding a BBCode opening tag — using parser tokens, explicit candidate selection or a rendered-element mapping.

  2. Rewriting its attributes — using one shared implementation for attribute boundaries, serialization and preservation of unrelated values.

  3. Validating the result — optionally enforcing target identity, ambiguity detection, rendered-content equivalence or attribute round-trip checks, depending on the caller.

That could allow simpler workflows to use straightforward token selection while more sensitive automatic editing operations request stronger safeguards.

However, it could also introduce unnecessary complexity compared with maintaining two smaller APIs.

Question for maintainers

Would it be preferable for the two PRs to retain their separate rewriting APIs, given their different target-selection requirements?

Alternatively, would a shared lower-level attribute rewriter with configurable target selection and validation be a better long-term direction?

I’m particularly interested in whether the additional safety checks currently implemented by BbcodeRewriter are considered generally useful core functionality, or whether they should remain specific to use cases that need stronger guarantees.

I’m not proposing that either PR needs to block the other; the aim is to avoid unnecessary duplication in Discourse’s core BBCode infrastructure as both features develop.

1 Like