Page MenuHomePhabricator

Edit summary should include the label of foreign entities
Closed, ResolvedPublic

Description

As shown on the screenshot below, page title of the foreign entity is used in the edit summary on the diff page, instead of showing foreign property label.

Entity title lookup apparently picks the right page title, just label of the foreign entity is not used in this case.

Patch-For-Review:

Details

Related Gerrit Patches:

Related Objects

StatusAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
StalledNone
ResolvedLydia_Pintscher
ResolvedLydia_Pintscher
ResolvedLydia_Pintscher
ResolvedWMDE-leszek
ResolvedWMDE-leszek

Event Timeline

WMDE-leszek updated the task description. (Show Details)
daniel added a comment.Feb 6 2017, 6:16 PM

The issue here is how SummaryFormatter and LinkBeginHookHandler interact:

  • SummaryFormatter uses a EntityIdPlainLinkFormatter to generate wiki-links for each entity.
  • LinkBeginHookHandler recognized links to entity pages, and automatically looks up the entity's label and uses it as the link text.

For foreign entities however, EntityIdPLainLinkFormatter would generate a link of the form [[foo:Special:EntityPage/P17]], which LinkBeginHookHandler does not recognize, since it's not a link to a local entity namespace.

After some discussion with @WMDE-leszek, I see two options to address this:

  1. teach LinkBeginHookHandler to parse links of the form [[foo:Special:EntityPage/P17]]. The pattern is easy enough, we can check whether foo is a known repo, and we can construct the EntityId foo:P17 unambiguously (using for now the assumption that repo names and interwiki prefixes are the same).
  2. make EntityIdPlainLinkFormatter (or a simmilar class) emit links of the form [[foo:Special:EntityPage/P17|foo:P17]], using the serialized ID as the link text (at least if it's different from the link target). This would give LinkBeginHookHandler more information to work with, but it also breaks some assumptions, and requires some refactoring. In particular:
  • LinkBeginHookHandler would need to be migrated from the deprecated LinkBegin hook to the new HtmlPageLinkRendererBegin hook. Care must be taken to correctly convert from the old $html to the new $text parameter.
  • The handler function should look at $text, and if it is a string and can be parsed as an EntityId, it looks up the label and replaced $text accordingly (with an HtmlArmor object). If EntityId is null or empty, the old code for parsing the link target should be used. This needs to be supported indefinitely, so we can still render old edit summaries correctly.
  • This breaks the assumption that the LinkBeginHookHandler magic that localized entity links will only kick in if no link text was explicitly given. There might be some code that explicitly sets the link text to the ID, in the expectation that it will not be replaced by the label.
daniel added a comment.Feb 6 2017, 6:18 PM

Thinking about it now, I think I prefer option 1 as a first step, even though it's kind of dirty (parsing links, eww). We can look into option 2 later, but it seems a bit involved and brittle.

daniel moved this task from Inbox to Push on the User-Daniel board.Feb 6 2017, 6:20 PM

Change 336623 had a related patch set uploaded (by WMDE-leszek):
Parse foreign entity page links in LinkBeginHookHandler

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

Change 339654 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Refactor foreign EntityId parsing in LinkBeginHookHandler

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

Change 339655 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
Move check checks up in LinkBeginHookHandler::doOnLinkBegin

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

Change 336623 merged by jenkins-bot:
Parse foreign entity page links in LinkBeginHookHandler

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

Change 340523 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
[mediawiki/extensions/Wikibase] Refactor foreign EntityId parsing in LinkBeginHookHandler, step 1

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

Change 340524 had a related patch set uploaded (by Thiemo Mättig (WMDE)):
[mediawiki/extensions/Wikibase] Refactor foreign EntityId parsing in LinkBeginHookHandler, step 2

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

Change 340524 abandoned by Thiemo Mättig (WMDE):
Refactor foreign EntityId parsing in LinkBeginHookHandler, step 2

Reason:
I did not assigned anybody on purpose because I wanted to see if this fails. It does not, which leaves me puzzled.

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

WMDE-leszek closed this task as Resolved.Mar 7 2017, 1:46 PM
thiemowmde reopened this task as Open.Apr 13 2017, 1:02 PM
thiemowmde moved this task from Done to Review on the Wikidata-Former-Sprint-Board board.
thiemowmde added a subscriber: thiemowmde.
thiemowmde updated the task description. (Show Details)Apr 13 2017, 1:04 PM
WMDE-leszek lowered the priority of this task from High to Normal.Apr 18 2017, 12:11 PM

Lowering the priority as the original task has been completed a while a ago. This ticket is now reused for the refactoring patch, which is not critical.

Oh, thanks. I forgot to look at the priority field.

Change 340523 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Minor refactoring of foreign EntityId parsing in LinkBeginHookHandler

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

WMDE-leszek closed this task as Resolved.Apr 20 2017, 8:27 AM

Change 339654 merged by WMDE-leszek:
[mediawiki/extensions/Wikibase@master] Refactor foreign EntityId parsing in LinkBeginHookHandler

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

Change 339655 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Move cheap checks up in LinkBeginHookHandler::doOnLinkBegin

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