Page MenuHomePhabricator

Add missing tests for nsText property of title objects, and set it to nil for interwiki links
Open, Needs TriagePublic

Description

Dedicated ticket for missing tests I noticed while having another peek back at T180911 and T369784:

On a related note, in TitleLibraryTests.lua there are tests for subjectNsText, and now for talkNsText as well, but there are no tests for nsText.

These tests have been missing from the beginning and I'm not sure why, it was probably just an oversight.

Event Timeline

Change #1092899 had a related patch set uploaded (by Gerrit Patch Uploader; author: Anne Haunime):

[mediawiki/extensions/Scribunto@master] Add missing tests for nsText property of title objects

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

Fixed tests: gerrit 1092899/1..2.

That was precisely because of the issue detailed in T369784 (which is pending since July…). After the patch at hand is merged, gerrit 1053882 will need to be rebased (to update the tests added here)

Notice how nsText is, currently, diverging from subjectNsText and talkNsText.
We could (should) state that nsText is either subjectNsText or talkNsText… but currently there is the difference about the underscores.

Tests are still failing, and it's quite interesting.

Provided the object created by mw.title.new( 'interwikiprefix:Module:TestFramework' ):

  • Its subjectNsText and talkNsText properties are nil.
  • So naturally, I was expecting nsText to be nil as well. But currently, its value is '' (empty string)…

Related: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/983410

I would suggest to be bold, and complete/correct gerrit 983410 by adding nsText to the list of properties that are set to nil. Considering how interwiki links are such a special case, I can't think of situations where this nsText change would break something.

I don't think that amendment is worth creating a dedicated Phabricator task (but correct me if I'm wrong).

Od1n renamed this task from Add missing tests for nsText property of title objects to Add missing tests for nsText property of title objects, and set it to nil for interwiki links.Jan 1 2025, 8:34 PM

Uploaded patchset 3. It now accomplishes two things:

  • Sets nsText to nil for interwiki links, as pointed out above.
  • Adds the missing unit tests, as originally intended.

Currently encountering an error during the unit tests, at this line: mw.title.lua#L91.
That is because of an attempt to concatenate the new value nil.

This can be fixed, however:

  • We should set the interwiki property to nil as well, for consistency, and because we don't know the value of this property. (edit 2025-05-21: Actually, we do know the value of this property, don’t we?)

    ⚠️🤔 There is something I don't understand:
    • The current value of this property is '' (empty string): as obtained with mw.title.new( 'interwikiprefix:Module:TestFramework' ).interwiki
    • However, the relevant unit test expects the value 'interwikiprefix': TitleLibraryTests.lua#L296.
  • We should also set the namespace property to nil. Currently it contains 0, which is erroneous.

I would need to understand the situation regarding the interwiki property. As for the namespace property, we can postpone changing it to a later stage.

⚠️🤔 There is something I don't understand:

  • The current value of this property is '' (empty string): as obtained with mw.title.new( 'interwikiprefix:Module:TestFramework' ).interwiki
  • However, the relevant unit test expects the value 'interwikiprefix': TitleLibraryTests.lua#L296.

That's because MediaWiki checks whether the interwiki is a recognized one. If it's not recognized, it is basically handled as part of a local title.

See TitleParser.php#L235, and in particular TitleParser.php#L264:

} elseif ( $this->interwikiLookup->isValidInterwiki( $p ) ) {

So, there should simply be a difference between WMF production wikis and the test environment; the former doesn't recognize it, whereas the latter does, likely due to some override that I haven't yet localized.

Anyway, setting additional properties to nil is not as straightforward as I expected, so I'm considering resetting the patch to its initial task: just adding the missing unit tests. Additional work later would be better suited to a new ticket.