r106752 MediaWiki - Code Review archive

Test Result (4 failures / +4)

   ApiUploadTest.testUpload
   ApiUploadTest.testUploadSameFileName
   ApiUploadTest.testUploadSameContent
   ApiUploadTest.testUploadStash

...looks like these were always skipped on my test box.

Running tests again, it looks like they were *not* skipped on my box...they just work. Looks like there is some inconsistency then.

On my local installation running trunk:

Notice: Undefined index: directory in D:\F_Programmierung\xampp\htdocs\wiki2\includes\Setup.php on line 222

Which repo global?

from my LocalSettings.php:

$wgForeignFileRepos[] = array(
	'class'                  => 'ForeignAPIRepo',
	'name'                   => 'shared',
	'apibase'                => '[http://translatewiki.net/w/api.php', http://translatewiki.net/w/api.php',]
	'fetchDescription'       => true, // Optional
	'descriptionCacheExpiry' => 3600,
);

$wgForeignFileRepos[] = array(
	'class'                  => 'ForeignAPIRepo',
	'name'                   => 'shared',
	'apibase'                => '[http://commons.wikimedia.org/w/api.php', http://commons.wikimedia.org/w/api.php',]
	'fetchDescription'       => true, // Optional
	'descriptionCacheExpiry' => 3600,
);

Fixed with r106764.

Double redirect subtitle after file move: https://translatewiki.net/wiki/File:Museum_Kunstpalast_-_Eingang_Ostfl%C3%BCgel_und_Brunnen_%28test%29.jpg?uselang=en

Are you sure that wasn't there before? I don't see how this is likely to affect that code.

Maybe before. To be fair: I have not looked through your code, just testing.

I think this is more likely to be r102417's fault

You added a few message groups in MessagesEn.php. Those headers need to be added at the bottom of messages.inc, too, in $wgBlockComments. If that is not done, those comment blocks are not added when the MessagesXx.php files are rebuilt.

r106796

The documentation of $wgLocalFileRepo needs to be updated.

I'd like to see less code in Setup.php. The filerepo setup has grown to 143 lines, or 25% of the file. Calling FileBackendGroup/LockManagerGroup::singleton() from Setup.php seems slow and unnecessary. For many requests, a backend object will never be needed. You could remove the register() functions and configure the group from singleton() instead, like in RepoGroup::singleton(). The configuration backwards compatibility code could be moved to either singleton() or the group constructor.

// File where directory should be
$op = array( 'op' => 'delete', 'src' => $thumbDir );
$this->repo->getBackend()->doOperation( $op );

I'm sure this felt really elegant at the time, but I think at some point you're going to realise how much typing it is and regret doing it. Or if you don't regret it, other developers will just silently resent you forever. I suggest adding a few convenience wrappers for common operations, so that code like this will be shorter. You already used the short names such as FileBackend::delete() for internal interfaces, which makes things a bit awkward. It probably makes sense to rename the internal interfaces, allowing you to use the shortest possible names for the interface with FileRepo. I am thinking:

$this->repo->getBackend()->delete( array( 'src' => $thumbDir ) );

Or maybe even:

// File where directory should be
$this->repo->getBackend()->delete( $thumbDir );

Running test Link with double quotes in title part (literal) and alternate part (interpreted)... PHP Notice:  Undefined index: backend in /var/www/thedarkcitadel.com/w/includes/filerepo/FileRepo.php on line 45
PHP Notice:  Undefined index: backend in /var/www/thedarkcitadel.com/w/includes/filerepo/FileRepo.php on line 48
No backend defined with the name ``.
Backtrace:
#0 /var/www/thedarkcitadel.com/w/includes/filerepo/FileRepo.php(48): FileBackendGroup->get(NULL)
#1 /var/www/thedarkcitadel.com/w/includes/filerepo/RepoGroup.php(341): FileRepo->__construct(Array)
#2 /var/www/thedarkcitadel.com/w/includes/filerepo/RepoGroup.php(329): RepoGroup->newRepo(Array)
#3 /var/www/thedarkcitadel.com/w/includes/filerepo/RepoGroup.php(199): RepoGroup->initialiseRepos()
#4 /var/www/thedarkcitadel.com/w/includes/ImageFunctions.php(27): RepoGroup->checkRedirect(Object(Title))
#5 /var/www/thedarkcitadel.com/w/includes/parser/Parser.php(1890): wfIsBadImage('Denys_Savchenko...', Object(Title))
#6 /var/www/thedarkcitadel.com/w/includes/parser/Parser.php(1646): Parser->replaceInternalLinks2(false)
#7 /var/www/thedarkcitadel.com/w/includes/parser/Parser.php(1090): Parser->replaceInternalLinks('[[File:Denys Sa...')
#8 /var/www/thedarkcitadel.com/w/includes/parser/Parser.php(345): Parser->internalParse('[[File:Denys Sa...')
#9 /var/www/thedarkcitadel.com/w/tests/parser/parserTest.inc(485): Parser->parse('[[File:Denys Sa...', Object(Title), Object(ParserOptions), true, true, 1337)
#10 /var/www/thedarkcitadel.com/w/tests/parser/parserTest.inc(399): ParserTest->runTest('Link with doubl...', '[[File:Denys Sa...', '<p><a href="https://www.mediawiki.org/in...', '', '')
#11 /var/www/thedarkcitadel.com/w/tests/parser/parserTest.inc(381): ParserTest->runTests(Object(TestFileIterator))
#12 /var/www/thedarkcitadel.com/w/tests/parserTests.php(90): ParserTest->runTestsFromFiles(Array)
#13 {main}

I can't really reproduce this and it's not on jenkins. I'm currently working on other stuff atm rather than spending more hours debugging our shitty test framework. I'll get around to poke some more later.

Fixed this one in r107724.

OverlordQ: could you file a bug on this problem? There's much more review that's needed on this revision, and given that this problem works on our Jenkins server (from what Aaron tells me), this seems like a failure of the test framework rather than the core of Aaron's code. I imagine that this will be a bug that we assign to someone else besides Aaron. Thanks!

PHP Fatal error: Call to undefined method FSFile::sha1Base36() in LocalFile.php:1374 Parser.php:3554 Parser.php:4925 Parser.php:1906 Parser.php:1646 Parser.php:1090 Parser.php:345 WikiPage.php:2032 WikiPage.php:1136 parserTest.inc:1179 NewParserTest.php:711 NewParserTest.php:332 NewParserTest.php:480 NewParserTest.php:480 TestCase.php:738 TestCase.php:628 TestResult.php:666 TestCase.php:576 MediaWikiTestCase.php:66 TestSuite.php:757 TestSuite.php:733 TestSuite.php:693 TestSuite.php:693 TestSuite.php:693 TestSuite.php:693 TestRunner.php:305 Command.php:188 MediaWikiPHPUnitCommand.php:44 phpunit.php:60 phase3/includes/filerepo/file/LocalFile.php on line 1374

I think you wanted getSha1Base36?

Not so easy, getSha1Base36() is not a static function like the old one.

Breaks a maintenance script:

$ php maintenance/rebuildImages.php --missing
PHP Fatal error:  Call to undefined method LocalRepo::enumFilesInFS() in maintenance/rebuildImages.php on line 168
PHP Stack trace:
PHP   1. {main}() maintenance/rebuildImages.php:0
PHP   2. require_once() maintenance/rebuildImages.php:225
PHP   3. ImageBuilder->execute() maintenance/doMaintenance.php:105
PHP   4. ImageBuilder->crawlMissing() maintenance/rebuildImages.php:64

This was fixed in r107991.

Seems to be sending incorrect HTCP purge packets (Not sure if this revision is at fault, but I figure its a likely possibility). From my debug log:

Purging URL [http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F120px-Handsprecan.jpg http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F120px-Handsprecan.jpg] via HTCP
Purging URL [http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F180px-Handsprecan.jpg http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F180px-Handsprecan.jpg] via HTCP
Purging URL [http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F.. http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F..] via HTCP
Purging URL [http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F. http://localhost/w/phase3/images/thumb/0/00/Handsprecan.jpg/%2Fvar%2Fwww%2Fw%2Fphase3%2Fimages%2Fthumb%2F0%2F00%2FHandsprecan.jpg%2F.] via HTCP
Purging URL [http://localhost/w/phase3/images/0/00/Handsprecan.jpg http://localhost/w/phase3/images/0/00/Handsprecan.jpg] via HTCP

(I also checked the actual packets sent, and the wrong urls appear there too)

Note the [ and ] didn't appear in the debug log, that's just code review being stupid...

Are you talking about the LocalFile purgeThumbList() functions, maybe you can var_dump() the list of paths to see what it gives at that point. If its OK there, then it's probably not this commit.

here's the var_dump from purgeThumbList():

array(9) {
  [0]=>
  object(SplFileInfo)#137 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(71) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/320px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(20) "320px-DubocePark.jpg"
  }
  [1]=>
  object(SplFileInfo)#138 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(71) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/180px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(20) "180px-DubocePark.jpg"
  }
  [2]=>
  object(SplFileInfo)#135 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(71) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/800px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(20) "800px-DubocePark.jpg"
  }
  [3]=>
  object(SplFileInfo)#192 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(72) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/1280px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(21) "1280px-DubocePark.jpg"
  }
  [4]=>
  object(SplFileInfo)#193 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(53) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/.."
    ["fileName":"SplFileInfo":private]=>
    string(2) ".."
  }
  [5]=>
  object(SplFileInfo)#194 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(52) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/."
    ["fileName":"SplFileInfo":private]=>
    string(1) "."
  }
  [6]=>
  object(SplFileInfo)#195 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(72) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/1024px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(21) "1024px-DubocePark.jpg"
  }
  [7]=>
  object(SplFileInfo)#196 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(71) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/120px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(20) "120px-DubocePark.jpg"
  }
  [8]=>
  object(SplFileInfo)#197 (2) {
    ["pathName":"SplFileInfo":private]=>
    string(71) "/var/www/w/phase3/images/thumb/6/62/DubocePark.jpg/640px-DubocePark.jpg"
    ["fileName":"SplFileInfo":private]=>
    string(20) "640px-DubocePark.jpg"
  }
}

It looks like the issue relates to calling getThumbUrl on some new fancy object, where before it was called on just a string containing the file name.

Unfortunately, DumpHTML is broken with the new FileBackend code (see bug 33878). Can someone who knows the new code please adapt this extension?