Page MenuHomePhabricator

Make Title::getWikiId() work correctly
Open, Needs TriagePublic

Description

Title::getWikiId() (code) is implemented as:

public function getWikiId() {
	return self::LOCAL;
}

Which is incorrect if the title is an interwiki link. The docblock notes:

@note The behavior of this method if considered undefined for interwiki links.
	 * At the moment, this method always returns false. But this may change in the future.

This violates the contract of WikiAwareEntity::getWikiId, and should be changed. If that's not currently possible, better would be to throw an exception if the Title is an interwiki, so the LSP violation would be more explicit and there'd be no risk of random malfunctioning if something relies on getWikiId() (perhaps because it's typehinting against a base interface like PageReference, and not Title).

It seems to me that the correct implementation would have to copy that of Title::getTransWikiID(). I should note that the method is not guaranteed to work because the iw_wikiid is not mandatory even for other wikis in the same wikifarm, but this has nothing to do with Title. I also don't know if there's another way to get a wiki ID from an interwiki prefix.

Event Timeline

Relatedly, Title::toPageIdentity() is also hardcoding LOCAL as the wiki ID, which is wrong for the same reasons as above. Note, this is just a secondary problem, since it should just use getWikiId(), so it will be fixed once getWikiId() is fixed.

Change 785345 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Make Title::toPageIdentity use getWikiId()

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

Relatedly, Title::toPageIdentity() is also hardcoding LOCAL as the wiki ID, which is wrong for the same reasons as above. Note, this is just a secondary problem, since it should just use getWikiId(), so it will be fixed once getWikiId() is fixed.

Title and WikiPage are both guaranteed to represent local pages (or links). If they were to become corss-wiki capable, that would be a breaking change and would violate LSP.

This violates the contract of WikiAwareEntity::getWikiId, and should be changed. If that's not currently possible, better would be to throw an exception if the Title is an interwiki, so the LSP violation would be more explicit and there'd be no risk of random malfunctioning if something relies on getWikiId() (perhaps because it's typehinting against a base interface like PageReference, and not Title).

How does it violate the contract? Title is always local, even if it has an interwiki prefix.

If LinkTarget was to become interwiki-aware, then the wiki-id and the interwiki-prefix would not be equivalent, they would be orthogonal. For example:

  • wiki-id "frwiki" and interwiki prefix "it" is an interwiki link on French Wikipedia to Italian Wikipedia.
  • wiki-id "frwiktionary" and interwiki prefix "it" is an interwiki link on French Wiktionary to Italian Wiktionary.

We discussed this long and hard in the context of the question what the relationship is between LinkTarget and PageReference. The interwiki prefix does not make a Title cross-wiki. It's still to be interpreted in the context of the local wiki - in particular, it needs to use the local wiki's interwiki prefixes.

It seems to me that the correct implementation would have to copy that of Title::getTransWikiID(). I should note that the method is not guaranteed to work because the iw_wikiid is not mandatory even for other wikis in the same wikifarm, but this has nothing to do with Title. I also don't know if there's another way to get a wiki ID from an interwiki prefix.

The fact that iw_wikiid is omitted in production is a performance optimization: the interwiki map is big, and it is loaded on every request. See T113034 for thoughts and discussions. I have some old experimental patches lying around for this...

Change 785345 merged by jenkins-bot:

[mediawiki/core@master] Make Title::toPageIdentity use getWikiId()

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

@daniel Ahhh I see... I admit that it's not super-obvious; I was thinking for a longer-term plan (but shorter term than removing Title) that Title and WikiPage wouldn't have to know about interwiki at all. They would just have a wiki ID; any conversion between interwiki and wiki ID (in the context of the current wiki) would happen upfront in some other factory/lookup service. But perhaps this is already how PageIdentity is supposed to be used, except for the Title and WikiPage implementation. Otherwise, it feels confusing and possibly redundant to have both interwiki and wiki ID.

The fact that iw_wikiid is omitted in production is a performance optimization: the interwiki map is big, and it is loaded on every request. See T113034 for thoughts and discussions. I have some old experimental patches lying around for this...

Good to know, thank you. I assume this will have to be fixed somehow though, right? Having no way to convert interwiki to wiki IDs seems problematic, especially if we want to implement something like the plan I just proposed.

it feels confusing and possibly redundant to have both interwiki and wiki ID.

Yes, it's confusing... but I don't think they can be consolidated. They do different things.