Page MenuHomePhabricator

Drop requirement to define a talk namespace for every subject namespace
Open, MediumPublic

Description

Currently, MW core requires a talk namespace to be defined for every subject namespace. However, in some cases, having associated talk pages is unnecessary or even unwanted, for instance for Flow's topic namespace. Extensions like flow try to work around this by not defining the talk namespace, and trying to hack around the fallout this causes in core code. Other extensions may define the unwanted namespace, but hide links and prevent editing. Both should not be necessary.

Current Situation

The assumption of having a talk namespace associated is manifest in the MWNamespace::getTalk() and Title::getTalkNsText() methods. More importantly, the assumption is spread throughout the code base by calls to Title::getTalkPage(), which is guaranteed to always return a Title. The requirement is also documented in https://www.mediawiki.org/wiki/Manual:Using_custom_namespaces#Creating_a_custom_namespace.

The behavior of Title::getTalkPage() is scary in the case no talk namespace is defined: The Title object returned by Title::getTalkPage() will eventually rely on Title::prefix() to add the namespace prefix. Title::prefix() will take the return value of Title::getNsText() unseen, and prepend it to the title text. However, Title::getNsText() will return false if the namespace is undefined! So the resulting title has an empty namespace prefix, effectively causing it to refer to the main namespace, by virtue of the fact that turning the boolean false into a string in PHP results in an empty string.

This behavior causes wrong links to be shown in the "talk" tab on top of the page, but surprisingly few other places, even though Title::getTalkPage is called a lot by core, and various extensions. Most such calls are for user talk pages. Some classes that use getTalkPage for all kinds of pages are SpecialEditWatchlist and code that constructs FeedItems. The $comments parameter of a FeedItem is typically set to the URL of the page's talk page. However, FeedItem comments are currently not used in Autom output (the respective line in AtomFeed::outItem is commented out), and RSS feeds are disabled on the live site.

So it seems the fallout of ignoring the requirement of defining a talk namespace is currently limited, but only by sheer luck. It would be much better to drop the requirement from core.

Proposed Solution

UPDATED after discussion on IRC on June 7 2017.

Change the behavior of relevant methods in Title as follows:

  • MWNamespace::canTalk() should check whether the talk NS is actually defined. This would also change the behavior of Title::canTalk().
    • Title::canTalk is a strange name. Perhaps introduce Title::hasTalkPage to replace it.
  • Title::getTalkPage() should throw an exception if Title::hasTalkPage returns false.
    • Callers should check canTalk() first, unless the title is known to be on in a well known namespace with a guaranteed talk namespace associated, as is the case when calling getTalkPage on a user page title, for example.
  • Title::getTalkNSText() should declare that it may return false. This is already the case, but not documented.
  • Title::prefix() should check whether getNsText() returned false. In that case, it should prefix the title with Special:BadTitle/NS<number>:<title>. This behavior will be inherited by getPrefixedText() and friends.
    • Note that using Special:BadTitle here doesn't allow for a successful round trip. But neither did the old behavior, which caused the text form to refer to the main namespace.
  • Title::makeTitle must continue to return Title objects when called with a bad namespace ID, so we can still render e.g. log entries about pages in namespaces that were removed.
    • We could introduce a sensitive version, Title::makeTitleThrow, but Title::makeTitleSafe can already be used for this. Title::makeTitleThrow could behave just like Title::makeTitleSafe, except for throwing an exception instead of returning null for bad namespaces and malformed titles.
  • Introduce Title::isValid(), that applies the same checks as makeTitleSafe.
    • Title::canExist() should probably check Title::isValid().

All code that calls Title::getTalkPage() must be changed to check Title::canTalk() first.

Event Timeline

daniel created this task.May 12 2017, 11:33 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 12 2017, 11:33 AM
Anomie added a subscriber: Anomie.May 12 2017, 3:14 PM

This somewhat reminds me of T487: RfC: Associated namespaces.

Perhaps we could have Special:ShowMessage/message-key-here.

Or we could just use Special:BadTitle, unless displaying a custom message is important.

Title::prefix() should check whether getNsText() returned false. In that case, some string derived from the namespace ID should be used as a "fake" namespace name. Perhaps NS1234:Foo would work.

Although "NS1234:Foo" is still a page in the main namespace. Again, we might use Special:BadTitle.

Ha, I forget that Special:BadTitle exists! Sure, we can use that. Would be nice to improve the wording a bit, and perhaps support a single parameter, to be passed using subpage syntax. But that's not even a blocker, we can start using Special:BadTitle as it is now.

Thanks @Anomie!

I note various things already use the Special:BadTitle subpage to indicate where the bad title was generated.

Hm... what should getPrefixedText() return for a title with an invalid namespace? It feels kind of wrong to return "Special:BadTitle/BAD-NS-678:Foo". But then, why not?

daniel added a comment.Jun 1 2017, 3:41 PM

Note to self: Tim said he'd prefer to see getTalkPage() throw an error if canTalk() returns false, rather than returning a link to Special:BadTitle.

ssastry added a subscriber: ssastry.

Note to self: Tim said he'd prefer to see getTalkPage() throw an error if canTalk() returns false, rather than returning a link to Special:BadTitle.

Seems like a cleaner solution.

Anomie added a comment.Jun 1 2017, 5:01 PM

Note to self: Tim said he'd prefer to see getTalkPage() throw an error if canTalk() returns false, rather than returning a link to Special:BadTitle.

Seems like a cleaner solution.

Although in that case you'll want to audit existing callers of getTalkPage() to fix any that are assuming the call would succeed for every positive namespace.

Note to self: Tim said he'd prefer to see getTalkPage() throw an error if canTalk() returns false, rather than returning a link to Special:BadTitle.

Seems like a cleaner solution.

Although in that case you'll want to audit existing callers of getTalkPage() to fix any that are assuming the call would succeed for every positive namespace.

Yup, understood. It seems a good opportunity to remove the assumption about a talk namespace existing always. My preference, wherever possible, is to use a real exception instead of using Special:BadTitle as a faux exception which is potentially fragile.

MaxSem added a subscriber: MaxSem.Jun 5 2017, 5:44 PM
daniel moved this task from Inbox to Request IRC meeting on the TechCom-RFC board.Jun 7 2017, 7:53 PM
daniel updated the task description. (Show Details)Jun 8 2017, 1:59 PM
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)
daniel updated the task description. (Show Details)

Change 358047 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Make Titles with an unknown namespace ID refer to Special:Badtitle.

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

Change 358163 had a related patch set (by Daniel Kinzler) published:
[mediawiki/core@master] Rename canTalk methods

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

Change 358423 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Add Title::isValid method.

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

Change 358164 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Make talk namespace optional [DNM]

https://gerrit.wikimedia.org/r/#/c/358164

Change 358047 merged by jenkins-bot:
[mediawiki/core@master] Make Titles with an unknown namespace ID refer to Special:Badtitle.

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

daniel moved this task from In progress to Last Call on the TechCom-RFC board.Jun 16 2017, 1:55 PM

During the ArchCom meeting on June 14 and the IRC discussion on June 7, it was agreed that this RFC be put on Final Call: if no new pertinent concerns are raised and remain open by June 28, this RFC will be approved for implementation.

Change 358163 merged by jenkins-bot:
[mediawiki/core@master] Rename canTalk methods

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

This RFC has been approved for implementation after the final call period.

Change 369424 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Introduce Title::getTalkPageIfDefined.

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

Change 369424 merged by jenkins-bot:
[mediawiki/core@master] Introduce Title::getTalkPageIfDefined.

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

Change 374620 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] Title: Make getOtherPage() check canHaveTalkPage()

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

Change 374620 merged by jenkins-bot:
[mediawiki/core@master] Title: Make getOtherPage() check canHaveTalkPage()

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

Change 358423 merged by jenkins-bot:
[mediawiki/core@master] Add Title::isValid method

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

Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 12 2018, 12:23 AM
SBisson moved this task from Inbox to External on the Growth-Team board.Jul 17 2018, 8:20 PM

Change 453552 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] TitleFormatter: Implement fbc144965315 and unify prefix logic

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

Change 453552 merged by jenkins-bot:
[mediawiki/core@master] TitleFormatter: Implement fbc144965315 and unify prefix logic

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