DEV: Quote attribution fields, bare/relative URLs, tag override fall-through#62
Merged
Merged
Conversation
gschlager
force-pushed
the
improve-quote-url-and-tag-fallback
branch
from
b74541c to
e1cfae5
Compare
AST::Quote called both attribution numbers "IDs", and a consumer shipped a real bug on the strength of that — the Discourse quote format's post: value is a post *number* within its topic, not a post id. The fields are now named for what they hold: - post_number and topic_id replace post and topic (Discourse attribution semantics, parsed by the BBCode handler). - post_id is new and separate: phpBB's post_id (and XenForo-style attributions) carry a database id, which the TextFormatter handler previously funneled into the number slot. It no longer does — a post id in a "post:N" reference links the wrong post, so id-based attributions now render name-only and the ids stay on the AST for consumers to remap. - user_id is new, for sources that attribute quotes by id (phpBB's user_id attribute); usernames may be unknown or stale at parse time. - All number/id fields coerce to Integer at the parser boundary, with an explicit base 10: without it, Integer() reads a leading zero as octal — "post:099" raised straight through the public parse API — and decodes prefix forms like "0x1A" to surprise values. Non-numeric values become nil instead of leaking garbage strings into attributions. The coerced values are unbounded Ruby Integers — a runaway digit run parses to a bignum, so consumers binding into int64 storage need their own bounds check (documented on the helpers). - QuoteTag falls back to username for name-only attributions when author is missing, so id-attributed quotes keep their name. The TextFormatter handler keeps the bare topic= fallback into topic_id (a topic reference is an id in every dialect we know) but deliberately does not map a bare post= attribute: without knowing the exporting platform it is undecidable whether it holds a post id or a post number, and guessing wrong produces attributions that link the wrong post. The BBCode handler documents the XenForo caveat: its attributions also match the post: pattern but carry ids.
Two lossy behaviors, both reported by a consumer: - A bare URL (text equal to the href, or no text at all) rendered as [url](url) — in Discourse that is a real difference, because a bare URL on its own line oneboxes and a Markdown link does not. Bare links now render as the plain href. Bareness is detected on the AST (single Text child equal to href), not on the rendered label, so Markdown escaping in the label can't break the detection. - Relative hrefs ([url=/t/5]here[/url], MediaWiki [[Page Name|x]]) were silently dropped because the scheme allowlist only admitted absolute URLs. Scheme-less hrefs now pass through; anything that looks like an unknown scheme (javascript:, data:) is still dropped, keeping the XSS posture from commit c994efa. Destinations containing whitespace get the <...> CommonMark form so wiki page names produce valid links.
Registering a custom Tag replaced the library entry with no way back: an override that only wants to intercept some nodes had to instantiate the stock tag itself and delegate manually. Tags now get interface.render_default(node), which renders through a pristine default library while children keep resolving against the renderer's (overridden) library — so an override can intercept exactly the nodes it cares about. Also turns the failure mode for a misbehaving override into a real diagnostic: a tag returning nil used to surface as an inscrutable TypeError inside string concatenation, deep in render_children. The renderer now raises immediately, naming the tag class, the node class, the returned value, and pointing at render_default for the fall-through case. Renderer#render_default joins #render on the mutant ignore list — it shares the identical @interface_cache housekeeping whose mutations are memory-only and unobservable; its rendering behavior is pinned by specs.
Review feedback: consumers that intercept Url nodes need the same bare-or-not decision the renderer makes (a deferred-link handler must record nil text for a bare URL, or the importer rebuilds [url](url) and re-breaks oneboxing). Keeping it private in UrlTag would force a second copy that can drift. Url#bare? judges the AST: no children, or a single Text child equal to the href. UrlTag delegates to it and keeps one render-time extra — a label that renders to nothing (e.g. an empty formatting child) also falls back to the bare href, since [](url) shows nothing.
gschlager
force-pushed
the
improve-quote-url-and-tag-fallback
branch
from
e1cfae5 to
2c32c30
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This addresses feedback from a consumer of Markbridge (they shipped a real bug on the strength of our docs, see below). Based on main, independent of the perf PR stack — the only overlapping files are Renderer/RenderingInterface and the conflicts are trivial in either merge order.
Quote attribution modeling (
FIX):AST::Quotecalled both attribution numbers "IDs", but in the Discourse quote formatpost:is a post number within the topic — a consumer stored it as a post id. The fields are nowpost_number/topic_id, plus two new ones:post_idfor id-based dialects (phpBB'spost_id, which the TextFormatter handler previously funneled into the number slot — building apost:Nreference from it links the wrong post, so id-based attributions now render name-only and keep the ids on the AST), anduser_id(phpBB attributes quotes by user id; usernames can be stale or unknown). All numbers/ids coerce to Integer at the parser boundary. The XenForopost:-is-an-id caveat is documented on the BBCode handler.Bare and relative URLs (
FEATURE): a bare URL rendered as[url](url), which kills oneboxing in Discourse — it now renders as the plain href (bareness detected on the AST, so Markdown escaping in the label can't confuse it). Relative hrefs ([url=/t/5], MediaWiki page names) were silently dropped by the scheme allowlist; scheme-less hrefs now pass through while unknown schemes (javascript:,data:) stay blocked. Whitespace-containing destinations get the<...>CommonMark form.Tag override fall-through (
FEATURE): custom tags getinterface.render_default(node)to delegate to the stock rendering for nodes they don't want to intercept — children still resolve against the overridden library. And a tag returning nil now raises immediately with the tag class, node class and value named, instead of an inscrutable TypeError deep inside string concatenation.Mutation coverage stays at 100% on all touched subjects. mutant simplified the Integer coercions (
value &&is redundant withexception: false) and forced a spec for the multi-child bare-URL edge;Renderer#render_defaultjoins#renderon the ignore list for the identical@interface_cachehousekeeping mutations.