r82577 MediaWiki - Code Review archive

followed up by r82580 which fix hardcoded path

Few minor typos:

+* NS_MEDIA and NS_SPECIAL do not have talk pages. MediaWiki raise an exception for them.
+throw new MWException( "$method does not make any sens for given namespace $index" );
+* for a given namespace where it does not make sens.

Typos fixed in 82692 Thanks to both of you.

What's the usecase for MWNamespace::getAssociated() ? Where would it be useful?

And as Nikerabbit mentioned, you have a couple of typos: Trow -> Throw, Sens -> sense (twice)

The new getAssociated method is needed by a feature I am working on actually.

Fair enough.

Trying to access /wiki/Media:Wiki.png:

#0 /www/w/includes/Namespace.php(110): MWNamespace::isMethodValidFor(-2, 'MWNamespace::ge...')
#1 /www/w/includes/Title.php(2783): MWNamespace::getTalk(-2)
#2 /www/w/includes/Skin.php(246): Title->getTalkPage()
#3 /www/w/includes/Skin.php(226): Skin->preloadExistence()
#4 /www/w/skins/Vector.php(31): Skin->initPage(Object(OutputPage))
#5 /www/w/includes/SkinTemplate.php(155): SkinVector->initPage(Object(OutputPage))
#6 /www/w/includes/OutputPage.php(1857): SkinTemplate->outputPage(Object(OutputPage))
#7 /www/w/includes/Wiki.php(399): OutputPage->output()
#8 /www/w/index.php(105): MediaWiki->finalCleanup(Object(OutputPage))
#9 {main}

This is exactly why I made it throw an extension: bad handling in MediaWiki :p

In this case, we have never properly handled the [[Media:file.ext]] web requests. The Wiki::articleFromTitle() function has a FIXME:

 if( NS_MEDIA == $title->getNamespace() ) {
      // FIXME: where should this go?
      $title = Title::makeTitle( NS_FILE, $title->getDBkey() );
  }

We should probably hack Wiki::handleSpecialCases() and redirect the user to the File: page.

I have played a bit with NS_MEDIA but haven't found a proper way to handle a clean redirection to NS_FILE equivalent. If you have any idea I am all for it!

This change also breaks the DumpHTML extension:

Special:Categories...MWNamespace::getTalk does not make any sense for given namespace -1
Backtrace:
#0 /srv/www/mediawiki/code/includes/Namespace.php(110): MWNamespace::isMethodValidFor(-1, 'MWNamespace::ge...')
#1 /srv/www/mediawiki/code/includes/Title.php(2783): MWNamespace::getTalk(-1)
#2 /srv/www/mediawiki/extensions/DumpHTML/SkinOffline.php(61): Title->getTalkPage()
#3 /srv/www/mediawiki/code/includes/SkinTemplate.php(500): SkinOffline->buildContentActionUrls(Array)
#4 /srv/www/mediawiki/extensions/DumpHTML/dumpHTML.inc(817): SkinTemplate->outputPage(Object(OutputPage))
#5 /srv/www/mediawiki/extensions/DumpHTML/dumpHTML.inc(481): DumpHTML->getArticleHTML(Object(Title))
#6 /srv/www/mediawiki/extensions/DumpHTML/dumpHTML.inc(221): DumpHTML->doArticle(Object(Title))
#7 /srv/www/mediawiki/extensions/DumpHTML/dumpHTML.inc(151): DumpHTML->doSpecials()
#8 /srv/www/mediawiki/extensions/DumpHTML/dumpHTML.php(151): DumpHTML->doEverything()
#9 {main}

In SkinOffline the buildContentActionUrls() seems to add a talk page to any namespace. Could be skipped when the page is a special one, for example:

       if( $this->mTitle->isTalkPage() ) {  // only add the talk action if the namespace has a talk page
       $content_actions['talk'] = $this->tabAction(
           $this->mTitle->getTalkPage(),
           'talk',
           $this->mTitle->isTalkPage(),
           ,
           true);
       }

I think you want $this->mTitle->canTalk(), not $this->mTitle->isTalkPage()

Thanks for the suggestions. I tested this and committed the change in r83643.

Note, testHasSubpages fails for me (Since I have subpages enabled in main namespace on my test wiki). I don't know if unittests are supposed to care if users have non-default stuff in LocalSettings.php, so I don't know if that's an issue or not.

I have added a workaround to testHasSubpages in r92234. Test will simply be skipped if you have that setting.

resetting to new. Looks fine in both 1.18 and trunk

+		$this->object = new MWNamespace;

This is useless (MWNamespace only has static methods) and unused.

+		$this->assertEquals( MWNamespace::getTalk( NS_MAIN), NS_TALK );

It's assertEquals( $expected, $actual ) , so you've got the order wrong. This appears to apply to all calls in this test from a quick glance.

+	public function testGetTalkExceptions() {
+		$this->assertNull( MWNamespace::getAssociated( NS_MEDIA ) );
+		$this->assertNull( MWNamespace::getAssociated( NS_SPECIAL ) );
+	}

This doesn't work the way you think it does. Because an exception stops the execution of the function, this doesn't test that both calls throw an exception, just that one of them does. Because the NS_MEDIA call throws an exception, the NS_SPECIAL call is never executed, so we'd never notice if that were to stop throwing an exception. Conversely, if NS_MEDIA were to stop throwing an exception but NS_SPECIAL still threw one, we'd never notice either. You would need to either split this up into two tests or use a data provider, I guess. You did do this properly in testGetAssociatedExceptionsForNsMedia().

+	public function testIsContent() {

Since the set of content namespaces is configurable, this test breaks on setups with funky configs. If you add e.g. $wgContentNamespaces[] = NS_USER; to your config, this test will fail. To test this properly you will have to exercise full control over $wgContentNamespaces for every assertion.

+	public function testHasSubpages() {

Same thing; this was fixed properly though.

The first 3 items have been dealt with. The last 2 still need dealing with...