Page MenuHomePhabricator

hywiki: some doi links in refs render with https with legacy parser and http with Parsoid
Closed, ResolvedPublicBUG REPORT

Description

Check Parsoid vs legacy. The first link in the first ref which is an extlink has a http:// protocol in Parsoid HTML and https:// protocol in legacy HTML.

I haven't investigated the reason - but likely either a difference in the output of the Cite journal template OR some functionality in Cite extension that is different in Parsoid. It is likely the template.

Event Timeline

I would guess https://www.mediawiki.org/wiki/Extension:SecureLinkFixer , which is deployed on WMF wikis, doesn't work with Parsoid.

@Legoktm says: "it's using whatever Linker hook for changing external URLs. presumably parsoid should call that or SLF could just do a post-processing DOM pass I guess.". One of us on CTT will poke around .. should be a straightforward fix.

I don't have a strong opinion on how this is implemented (really I haven't thought about it), I do want to flag one potential area for improvement though. The signature for the hook is:

public static function onLinkerMakeExternalLink( &$url, &$text, &$link, &$attribs, $linktype ) { ... }

where $url is a string, which means callers often need to parse the URL, do something with it, and then re-assemble it. On Wikimedia wikis this happens twice, as both SecureLinkFixer and Wikibase use the hook and call parse_url.

If this ends up being reimplemented as a new hook, I would suggest having the hook provide a pre-parsed version of the URL so it can be examined/manipulated in structured form and then re-assembled after the hooks are done.

Parsoid doesn't use the Linker at all, so this is likely to be implemented as a postprocessing pass; as such there's no good way to avoid re-parsing the href from the <a> tag each time. I probably need the new hook that Isabelle is going to add to the OutputTransform pipeline for DiscussionTools, though, so I'm waiting for that to land.

(New hook: Add OutputTransform{Early,Late}StageHook (1243919) )

There is a DOM postprocessing pass supported by the Parsoid Extension API and this should be implemented as part of that, not the OutputTransformPipeline because we want canonical HTML served by the REST API to have https, not just read views.

There is a DOM postprocessing pass supported by the Parsoid Extension API and this should be implemented as part of that, not the OutputTransformPipeline because we want canonical HTML served by the REST API to have https, not just read views.

I'm not sure I agree: if the canonical HTML has the altered URLs, it will break round-tripping and it would be impossible for an editor to actually *fix* the link to have the https URL in VE. (The closest would be to do a null edit somewhere else in the paragraph, which would trigger a dirty diff to actually fix the https url.)

There's a related feature being asked for at the moment to sanitize links to archive.is, and similar considerations apply: in order for the *editable* version of the page to be able to be fixed, we need to keep the canonical HTML as an accurate representation of the editors wikitext.

What this points out is that various downstream clients may want several output transform passes applied to the canonical HTML -- in fact we already know that: even VisualEditor doesn't actually want canonical HTML, they want canonical HTML with localization performed and sections stripped. So at some point we should provide a way to run portions of the output pipeline, probably controlled by setting specific parser options. But that's a separate task.

I think the consideration that will dictate this is: "Do we want to serve https:// fixed urls in read views, REST API, Enterprise, etc.?". If the answer is yes, we do it in the Parsoid Extension API. And, we have sufficient tools in our toolbox to prevent dirty diffs. That is an easily solved problem.

Change #1243953 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/extensions/SecureLinkFixer@master] Convert SecureLinkFixer to an OutputTransformStage

https://gerrit.wikimedia.org/r/1243953

I think the consideration that will dictate this is: "Do we want to serve https:// fixed urls in read views, REST API, Enterprise, etc.?". If the answer is yes, we do it in the Parsoid Extension API. And, we have sufficient tools in our toolbox to prevent dirty diffs. That is an easily solved problem.

The only use of canonical HTML for read views is PCS (mobile apps), and it is likely that we will need a solution for that -- but we also need to pipe that output via the OutputTransform pipeline to make language conversion work for apps (T380517).

My main intent behind SecureLinkFixer was to avoid having bots constantly needing to edit basically every single page a bunch of times as websites gradually added support for HTTPS. So that's how I approach it, as much as reasonably possible it should be as if the actual wikitext URL was HTTPS. In practice the Linker hook already existed so that's what I used and ideally we'd be able to preserve that behavior (I'm a little out of date and don't know what OutputTransform applies to or doesn't apply to). I would expect that e.g. my bot that fetches Parsoid HTML would see URLs already modified to HTTPS.

For roundtripping, I would think that we can just mark up the link to indicate that when converting back it needs to be modified? e.g. <a typeof="mw:Extension/securelinkfixer" data-mw='{"original-protocol":"http://"}' rel="mw:ExtLink"> (not sure if that's the right use of typeof but you get the point)

Change #1260069 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] Use DataAccess::getExternalUrlInfo() for attributes of external links

https://gerrit.wikimedia.org/r/1260069

Change #1254256 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] Add DataAccess::getExternalUrlInfo to support external link rewriting/blocking

https://gerrit.wikimedia.org/r/1254256

Change #1243953 abandoned by C. Scott Ananian:

[mediawiki/extensions/SecureLinkFixer@master] Convert SecureLinkFixer to an OutputTransformStage

Reason:

Abandoned in favor of Icf61267be418a3341df913acd990f445ac36dbbb

https://gerrit.wikimedia.org/r/1243953

Change #1254256 merged by jenkins-bot:

[mediawiki/core@master] Add DataAccess::getExternalUrlInfo to support external link rewriting/blocking

https://gerrit.wikimedia.org/r/1254256

Change #1260069 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Use DataAccess::getExternalUrlInfo() for attributes of external links

https://gerrit.wikimedia.org/r/1260069

Change #1264672 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.23.0-a24

https://gerrit.wikimedia.org/r/1264672

Change #1264672 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.23.0-a24

https://gerrit.wikimedia.org/r/1264672

ssastry assigned this task to cscott.
ssastry triaged this task as Medium priority.

Change #1243953 restored by C. Scott Ananian:

[mediawiki/extensions/SecureLinkFixer@master] Convert SecureLinkFixer to an OutputTransformStage

https://gerrit.wikimedia.org/r/1243953