Page MenuHomePhabricator

Lexeme pages do not show labels of redirected Items (unlike Item pages)
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

List of steps to reproduce (step by step, including full links if applicable):

  • Create a new Lexeme or edit an existing one to change the lex. category, language or grammatical feature (both linking to Items)
  • Use an Item ID that is a redirect

What happens?:

  • The label of the redirect target Item is not shown but instead only the Item ID of the redirected Item
  • Sometimes a reload is needed to show this

What should have happened instead?:

  • The label of the redirect target Item is shown.

Notes:

  • This seems to work as expected (i.e. show the label of the redirect target) on Items as well as statements on Lexemes, therefore there is some infrastructure to doing this
  • In the image below, the QID should have been resolved to the label of the item (if it has one)

image.png (168×308 px, 15 KB)

Event Timeline

Lucas_Werkmeister_WMDE renamed this task from Lexeme pages do not show lables of redirected Items (unlike Item pages) to Lexeme pages do not show labels of redirected Items (unlike Item pages).Mar 30 2022, 10:54 AM

I looked into this a bit, and I think it’s an unfortunate effect of some confusing class functionality in Wikibase. There are two relevant FallbackLabelDescriptionLookup implementations: LanguageFallbackLabelDescriptionLookup, which looks up labels and descriptions using an inner TermLookup while applying language fallbacks, and CachingFallbackLabelDescriptionLookup, which wraps another FallbackLabelDescriptionLookup and adds caching to it.

One thing CachingFallbackLabelDescriptionLookup does that isn’t exactly obvious from its name is that it also resolves redirects. The code to do this (based on EntityRevisionLookup) used to be directly in the class, until it was extracted into a separate RedirectResolvingLatestRevisionLookup class for T196882.

Both of these FallbackLabelDescriptionLookup implementations are instantiated in various places. Some places wrap a CachingFallbackLabelDescriptionLookup around a LanguageFallbackLabelDescriptionLookup, others return a LanguageFallbackLabelDescriptionLookup directly. Because only CachingFallbackLabelDescriptionLookup resolves redirects, this means that only the former places will show the labels of redirect targets – more or less arbitrarily, I think.

Places that create a wrapping CachingFallbackLabelDescriptionLookup (good):

  • FormatterLabelDescriptionLookupFactory, used to format entity ID values (I think)
  • WikibaseValueFormatterBuilders likewise(?)

Places that directly return a LanguageFallbackLabelDescriptionLookup (bad):

  • LanguageFallbackLabelDescriptionLookupFactory (I suppose that’s to be expected), which in turn is used in many places
  • WikibaseLexeme.datatypes.php PT:wikibase-lexeme formatter-factory-callback (has a TODO to use LanguageFallbackLabelDescriptionLookupFactory which wouldn’t really help at the moment, per above), used to format the language and lexical category in the tooltip (title) of Lexeme values (I think)
    • (the PT:wikibase-form formatter-factory-callback uses LanguageFallbackLabelDescriptionLookupFactory, though)
  • WikibaseRepo.entitytypes.php item ENTITY_ID_HTML_LINK_FORMATTER_CALLBACK – I think this is the one that’s actually used by LexemeView to render the language and lexical category
  • WikibaseLexeme.entitytypes.repo.php lexeme (and form) ENTITY_ID_HTML_LINK_FORMATTER_CALLBACK
  • WikibaseClient.ServiceWiring SidebarLinkBadgeDisplay, probably unrelated

I don’t know if there’s actually any good reason why these shouldn’t use CachingFallbackLabelDescriptionLookup as well.

@Lydia_Pintscher are there any places where we generally want to show item labels, but not for redirects? (E.g. Special:AllPages, perhaps?) Or do we always want to resolve redirects?

Slight correction to the above: FormatterLabelDescriptionLookupFactory only returns a CachingFallbackLabelDescriptionLookup wrapping a LanguageFallbackLabelDescriptionLookup when it’s been given a language fallback chain in its formatter options. If it’s only been given a single language, then it returns a LanguageLabelDescriptionLookup, which doesn’t do language fallbacks (sensible) and also doesn’t resolve redirects (probably not intentional).

What a mess.

This patch (in Wikibase, not WikibaseLexeme!) makes the labels show up – it’s certainly not an ideal implementation yet, and it remains to be investigated which other places are affected by it (and whether we want those other places to resolve redirects as well), but it’s a good starting point, I think:

diff --git a/repo/WikibaseRepo.entitytypes.php b/repo/WikibaseRepo.entitytypes.php
index 63ab398b8e..7f5d11ca23 100644
--- a/repo/WikibaseRepo.entitytypes.php
+++ b/repo/WikibaseRepo.entitytypes.php
@@ -18,6 +18,7 @@
  */
 
 use MediaWiki\MediaWikiServices;
+use ValueFormatters\FormatterOptions;
 use Wikibase\DataAccess\DatabaseEntitySource;
 use Wikibase\DataModel\Entity\EntityDocument;
 use Wikibase\DataModel\Entity\Item;
@@ -27,6 +28,7 @@
 use Wikibase\Lib\DataTypeDefinitions;
 use Wikibase\Lib\EntityTypeDefinitions as Def;
 use Wikibase\Lib\EntityTypeDefinitions;
+use Wikibase\Lib\Formatters\FormatterLabelDescriptionLookupFactory;
 use Wikibase\Lib\Formatters\LabelsProviderEntityIdHtmlLinkFormatter;
 use Wikibase\Lib\Interactors\MatchingTermsLookupSearchInteractor;
 use Wikibase\Lib\SimpleCacheWithBagOStuff;
@@ -245,8 +247,17 @@
 			);
 		},
 		Def::ENTITY_ID_HTML_LINK_FORMATTER_CALLBACK => function( Language $language ) {
-			$languageLabelLookupFactory = WikibaseRepo::getLanguageFallbackLabelDescriptionLookupFactory();
-			$languageLabelLookup = $languageLabelLookupFactory->newLabelDescriptionLookup( $language );
+			$formatterLabelDescriptionLookupFactory = new FormatterLabelDescriptionLookupFactory(
+				WikibaseRepo::getTermLookup(),
+				WikibaseRepo::getTermFallbackCache(),
+				new RedirectResolvingLatestRevisionLookup( WikibaseRepo::getEntityRevisionLookup() )
+			);
+			$languageFallbackChainFactory = WikibaseRepo::getLanguageFallbackChainFactory();
+			$languageFallbackChain = $languageFallbackChainFactory->newFromLanguage( $language );
+			$options = new FormatterOptions( [
+				FormatterLabelDescriptionLookupFactory::OPT_LANGUAGE_FALLBACK_CHAIN => $languageFallbackChain,
+			] );
+			$languageLabelLookup = $formatterLabelDescriptionLookupFactory->getLabelDescriptionLookup( $options );
 			return new LabelsProviderEntityIdHtmlLinkFormatter(
 				$languageLabelLookup,
 				WikibaseRepo::getLanguageNameLookup(),

@Lydia_Pintscher are there any places where we generally want to show item labels, but not for redirects? (E.g. Special:AllPages, perhaps?) Or do we always want to resolve redirects?

Hmmm I would have said no but indeed Special:AllPages currently does not show the label from the redirect target. Example: https://www.wikidata.org/wiki/Q10000117?redirect=no is shown by ID in Special:AllPages even though the target has a label in my interface language.
🤔 I don't have a good reason to go either direction. Thoughts?

🤔 I don't have a good reason to go either direction. Thoughts?

In that case, I think we should try to make the code resolve redirects in more situations, and assume that, when this affects other places besides the Lexeme header, that’s acceptable. I suspect there are lots of small, obscure places that don’t resolve redirects at the moment, where it just hasn’t been noticed so far. For example, in the tooltip of a Lexeme statement value, showing its language and lexical category, redirects aren’t currently resolved either, even with my patch above that shows them in the Lexeme page header:

image.png (548×504 px, 30 KB)

Alright, plan for now (discussed with Itamar):

  • Introduce a new factory, FallbackLabelDescriptionLookupFactory, that only promises to return any FallbackLabelDescriptionLookup (the interface); it actually returns a CachingFallbackLabelDescriptionLookup wrapping a LanguageFallbackLabelDescriptionLookup. (T312222)
  • Migrate other code to use this new factory. This should probably include all callers of the current LanguageFallbackLabelDescriptionLookupFactory (which, both in name and in phpdoc, promises to return specifically a LanguageFallbackLabelDescriptionLookup), but also others. (T312223)
  • Remove the old, more specific factory, LanguageFallbackLabelDescriptionLookupFactory, once it has become unused. (T312224)

This leaves the two lookup implementations as they are: arguably flawed (there’s no good reason why one resolves redirects and the other doesn’t), but I think it’s better not to move that code around at the moment. (Though we can at least add this information to their phpdoc.) We just change how the lookups are created, so that hopefully nobody ends up with an unwrapped LanguageFallbackLabelDescriptionLookupFactory that doesn’t resolve redirects.

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

[mediawiki/extensions/Wikibase@master] Use FallbackLabelDescriptionLookupFactory in entity types

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

Manuel triaged this task as Medium priority.Jul 12 2022, 8:42 AM

Change 812839 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use FallbackLabelDescriptionLookupFactory in item entity types

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