Page MenuHomePhabricator

Only format entity links with labels in edit summaries and on special pages
Closed, ResolvedPublic

Description

Wikibase has a useful feature where plain wikilinks in edit summaries are formatted with the label of the entity, which is very useful when looking e.g. at page histories:

[[Property:P735]]: [[Q463035]]

given name (P735): Douglas (Q463035)

Screenshot 2021-09-30 at 15-45-26 Revision history of Douglas Adams (Q42) - Wikidata.png (263×1 px, 135 KB) Screenshot 2021-09-30 at 15-45-51 Difference between revisions of Douglas Adams (Q42) - Wikidata.png (121×835 px, 21 KB)

However, the way this is implemented actually affects all links on diff and history URLs – specifically, this includes links in page content shown on diffs. This causes performance issues (T140879: WMFTimeoutException when loading some diffs on Wikidata [Timebox 8h]), but it’s also undesirable regardless of that – the content of a page looks different in diff and regular views, which is strange. For example:

Here’s the original appearance of my sandbox:

Screenshot 2021-09-30 at 16-09-37 User Lucas Werkmeister (WMDE) sandbox - Wikidata.png (249×560 px, 34 KB)

Then, I purged it without loading the result: curl --data-binary @- 'https://www.wikidata.org/w/index.php?title=User:Lucas_Werkmeister_(WMDE)/sandbox&diff=1505856440&oldid=1463378660&diffmode=source&action=purge' < /dev/null

And then loaded the diff: https://www.wikidata.org/w/index.php?title=User:Lucas_Werkmeister_(WMDE)/sandbox&diff=1505856440&oldid=1463378660&diffmode=source

And not only are there entity labels in the diff view – because this ends up in the parser cache, those labels will also be shown on subsequent non-diff views:

Screenshot 2021-09-30 at 16-15-14 User Lucas Werkmeister (WMDE) sandbox - Wikidata.png (249×560 px, 35 KB)

We should fix this, and only apply this special formatting to links that are actually in edit summaries. (Edited to add: and on special pages, such as Special:AllPages.)

Event Timeline

Super hacky sketch of an improvement:

MediaWiki core
diff --git a/includes/Linker.php b/includes/Linker.php
index 8ebbd1c19e..33c5e9432a 100644
--- a/includes/Linker.php
+++ b/includes/Linker.php
@@ -120,7 +120,15 @@ public static function link(
 			return $linkRenderer->makePreloadedLink( $target, $text, '', $customAttribs, $query );
 		}
 
-		return $linkRenderer->makeLink( $target, $text, $customAttribs, $query );
+		if ( in_array( 'comment', $options, true ) ) {
+			$linkRenderer->renderingComment = true;
+		}
+
+		try {
+			return $linkRenderer->makeLink( $target, $text, $customAttribs, $query );
+		} finally {
+			$linkRenderer->renderingComment = false;
+		}
 	}
 
 	/**
@@ -1639,6 +1647,7 @@ public static function makeCommentLink(
 				/* escape = */ false // Already escaped
 			);
 		} else {
+			$options[] = 'comment';
 			$link = self::link( $linkTarget, $text, [], [], $options );
 		}
 
diff --git a/includes/linker/LinkRenderer.php b/includes/linker/LinkRenderer.php
index 18df8cfbdd..65a1c02baf 100644
--- a/includes/linker/LinkRenderer.php
+++ b/includes/linker/LinkRenderer.php
@@ -82,6 +82,8 @@ class LinkRenderer {
 	 */
 	private $specialPageFactory;
 
+	public $renderingComment = false;
+
 	/**
 	 * @internal For use by LinkRendererFactory
 	 * @param TitleFormatter $titleFormatter
Wikibase
diff --git a/repo/includes/Hooks/HtmlPageLinkRendererEndHookHandler.php b/repo/includes/Hooks/HtmlPageLinkRendererEndHookHandler.php
index 9718c45db1..0967aa7004 100644
--- a/repo/includes/Hooks/HtmlPageLinkRendererEndHookHandler.php
+++ b/repo/includes/Hooks/HtmlPageLinkRendererEndHookHandler.php
@@ -169,6 +169,10 @@ public function onHtmlPageLinkRendererEnd(
 			return true;
 		}
 
+		if ( !$linkRenderer->renderingComment ) {
+			return true;
+		}
+
 		return $this->doHtmlPageLinkRendererEnd(
 			$linkRenderer,
 			Title::newFromLinkTarget( $target ),

This seems to work locally. A non-hacky version will look somewhat different, of course, but I think it might not be totally separate either. Looking at the many methods in Linker/LinkRenderer, and how edit summary links and regular links share so much code, I feel like a separate hook handler (which would be a very clean solution from Wikibase’s point of view) might not be feasible; some kind of flag on the link renderer, where the hook handler can look up what the link renderer is doing, could be a relatively economical way to transfer this information between Linker::makeCommentLink() and the hook handler. (The flag wouldn’t necessarily need to be mutable: Linker::link() already creates different LinkRenderer instances for different $options, so there could also be “a link renderer for comment links”, and the flag of that instance would never change during its lifetime.)

It turns out the implementation of comment link formatting was recently changed, and Linker::makeCommentLink() is gone (T285917), but I think I still found a solution that’s not too ugly.

Change 726638 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/core@master] Add comment flag to LinkRenderer

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

Change 726639 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Only add labels to entity links in comments and special pages

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

Lucas_Werkmeister_WMDE renamed this task from Only format entity links with labels in edit summaries to Only format entity links with labels in edit summaries and on special pages.Oct 5 2021, 4:12 PM
Lucas_Werkmeister_WMDE updated the task description. (Show Details)

Change 726638 merged by jenkins-bot:

[mediawiki/core@master] Add comment flag to LinkRenderer

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

Change 726639 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Only add labels to entity links in comments and special pages

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

No train this week (because of veterans’ day, I assume?), so we won’t be able to verify it until the end of next week. Do we want to try backporting to wmf.7? (But maybe not quite yet, T293948 is in a bit of a precarious state if I understand correctly.)

No train this week (because of veterans’ day, I assume?), so we won’t be able to verify it until the end of next week. Do we want to try backporting to wmf.7? (But maybe not quite yet, T293948 is in a bit of a precarious state if I understand correctly.)

If T293948 gets better and an opportunity to backport presents itself, then let's try backporting. But we don't have to force it. This was broken for so long, waiting a week longer won't make that much of a difference either.