Page MenuHomePhabricator

Remove unchecked calls to Title::getTalkPage from extensions
Open, Needs TriagePublic

Description

As per T165149, the contract of Title::getTalkPage changes to throw an exception if called on a page in a namespace that does not have a talk namespace associated.

All calls to Title::getTalkPage must either be changed to getTalkPageIfDefined, or they must first check canHaveTalkPage(). These checks can be omitted only for the User namespace, since user pages can always have talk pages. Note that some of the below may be calls to User::getTalkPage instead; those also don't need checks, since users can always have talk pages.

Offending calls to getTalkPage() should be removed from all extensions. They MUST be removed from all extensions deployed on the Wikimedia cluster before merging Icee208dc.

Here is a list of files with potentially offending calls to getTalkNamespace():

Deployed on Wikimedia cluster:

Not deployed on Wikimedia cluster:

More can be found using https://github.com/search?l=&p=7&q=org%3Awikimedia+getTalkPage+language%3APHP&ref=advsearch&type=Code&utf8=%E2%9C%93

Event Timeline

I can't find getTalkPageIfDefined, is it not in master yet or something?

I can't find getTalkPageIfDefined, is it not in master yet or something?

Oh, I see, it's part of this commit which can't be merged until this task is done. Perhaps getTalkPageIfDefined could be broken out and merged now, so that new code can be written that relies on it?

Change 368943 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/Echo@master] Check canHaveTalkPage() before calling getTalkPage()

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

Removed SiteScout from the list as those 2 cases are false positives (those calls are getting the user talk)

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

Perhaps getTalkPageIfDefined could be broken out and merged now, so that new code can be written that relies on it?

Good idea, done! I6d2613d8f7105048

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

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

Removed SiteScout from the list as those 2 cases are false positives (those calls are getting the user talk)

Is that actually true? With the underlying RFC, couldn't a wiki potentially make NS_USER not have a corresponding talk?

Change 369470 had a related patch set uploaded (by Ladsgroup; owner: Amir Sarabadani):
[mediawiki/extensions/WikibaseQualityConstraints@master] Fix call to Title::getTalkPage

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

Change 369470 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] Fix call to Title::getTalkPage

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

Is that actually true? With the underlying RFC, couldn't a wiki potentially make NS_USER not have a corresponding talk?

Not legally. $wgCanonicalNamespaceNames is private and is not supposed to be changed per install. Anything defined there can be assumed to be the same on all installs.
In fact, we currently guarantee talk namespaces for all default namespaces. But the User namespace is special, in that linking to the user talk page is very common. So it is useful to explicitly guarantee that the User_talk namespace is always defined.

Change 368943 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Check canHaveTalkPage() before calling getTalkPage()

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

Is that actually true? With the underlying RFC, couldn't a wiki potentially make NS_USER not have a corresponding talk?

Not legally. $wgCanonicalNamespaceNames is private and is not supposed to be changed per install. Anything defined there can be assumed to be the same on all installs.
In fact, we currently guarantee talk namespaces for all default namespaces. But the User namespace is special, in that linking to the user talk page is very common. So it is useful to explicitly guarantee that the User_talk namespace is always defined.

I suppose... But for what it's worth we can't enforce that unless we moved it to like static data within MWNamespace.

Soooo, back in like 2008 when I was doing internal MW work for $DAYJOB-2 one of the requests was to get rid of talk pages. I was able to do it, and this included NS_USER_TALK

Anyway, it's mostly me just pontificating over something I don't have a really strong opinion over. Carry on 😃

Change 370283 had a related patch set uploaded (by SrishtiSethi; owner: srish):
[mediawiki/extensions/EducationProgram@master] Check canHaveTalkPage() before calling getTalkPage()

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

Change 370326 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/News@master] Check Title::canHaveTalkPage before calling getTalkPage()

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

Change 370326 merged by jenkins-bot:
[mediawiki/extensions/News@master] Check Title::canHaveTalkPage before calling getTalkPage()

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

Change 370283 abandoned by SrishtiSethi:
Check canHaveTalkPage() before calling getTalkPage()

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

Could this be a good Google-Code-in-2018 task? If so, is there anyone willing to mentor this?

I can mentor this.

Created associated tasks in GCI.

@Mholloway: Thanks! I've tweaked the descriptions a bit on the GCI site and published part of them (more to come)! :)

Change 475584 had a related patch set uploaded (by Arcayn; owner: Arcayn):
[mediawiki/skins/CologneBlue@master] Change check to canHaveTalkPage before calling getTalkPage()

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

Change 475594 had a related patch set uploaded (by Pjht; owner: Pjht):
[mediawiki/extensions/GoogleNewsSitemap@master] Change getTalkPage() to getTalkPageIfDefined()

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

Change 475584 merged by jenkins-bot:
[mediawiki/skins/CologneBlue@master] Change check to canHaveTalkPage before calling getTalkPage()

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

The getTalkPage in the WikiLove extension is to a User talk page and therefore doesn't have to be checked.
I checked it off.

Thanks @Aklapper for the updates, and thanks to Platonides (not subscribed) for review assistance so far!

Change 475594 merged by jenkins-bot:
[mediawiki/extensions/GoogleNewsSitemap@master] Only set $commentsURL in FeedSMIItem.php when there is one for the page

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

Change 476066 had a related patch set uploaded (by Rafidaslam; owner: Rafid Aslam):
[mediawiki/extensions/BlueSpiceFoundation@master] ArticleHelper.class.php: Change getTalkPage() to getTalkPageIfDefined()

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

Change 476066 merged by jenkins-bot:
[mediawiki/extensions/BlueSpiceFoundation@master] ArticleHelper.class.php: Change getTalkPage() to getTalkPageIfDefined()

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

Change 477178 had a related patch set uploaded (by Mogmog123; owner: Mogmog123):
[mediawiki/extensions/EducationProgram@master] Changing Title::getTalkPage to getTalkPageIfDefined.

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

Change 477338 had a related patch set uploaded (by Arcayn; owner: Arcayn):
[mediawiki/extensions/EditSubpages@master] Add canHaveTalkPage() check before calling getTalkPage()

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

Change 477338 merged by jenkins-bot:
[mediawiki/extensions/EditSubpages@master] Add canHaveTalkPage() check before calling getTalkPage()

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

Change 477550 had a related patch set uploaded (by Bjornskjald; owner: Bjornskjald):
[mediawiki/extensions/BlueSpiceBlog@master] Remove unchecked calls to Title::getTalkPage

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

Change 477178 abandoned by Mholloway:
Changing Title::getTalkPage to getTalkPageIfDefined.

Reason:
The author abandoned this task on the GCI site.

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

Change 477550 merged by jenkins-bot:
[mediawiki/extensions/BlueSpiceBlog@master] Remove unchecked calls to Title::getTalkPage

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

Change 477867 had a related patch set uploaded (by Bjornskjald; owner: Bjornskjald):
[mediawiki/extensions/EducationProgram@master] Remove unchecked calls to Title::getTalkPage

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

Change 477867 merged by jenkins-bot:
[mediawiki/extensions/EducationProgram@master] Remove unchecked calls to Title::getTalkPage

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