From 865ac3b3553751dd72cf805d9933639b129f36e2 Mon Sep 17 00:00:00 2001 From: "James D. Forrester" Date: Tue, 2 Jul 2019 14:41:41 -0700 Subject: [PATCH] Drop strings for wgExternalDiffEngine, deprecated in 1.27 and 1.32 Also move the 'unit' test into integration, given it tests code using globals. Change-Id: Ie039cae9b5d2870c18a6deefec9a73de522dd847 (cherry picked from commit 5b3bbd5adea327912694745db9c53d5d39de3315) --- RELEASE-NOTES-1.34 | 3 ++ includes/diff/DifferenceEngine.php | 41 +++++++++---------- .../includes/api/ApiComparePagesTest.php | 10 ----- .../DifferenceEngineSlotDiffRendererTest.php | 8 ---- .../includes/diff/DifferenceEngineTest.php | 24 ----------- ...eEngineSlotDiffRendererIntegrationTest.php | 19 +++++++++ 6 files changed, 41 insertions(+), 64 deletions(-) create mode 100644 tests/phpunit/integration/includes/diff/DifferenceEngineSlotDiffRendererIntegrationTest.php diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 48fcbafb97..d7930085b3 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -63,6 +63,9 @@ $wgPasswordPolicy['policies']['default']['PasswordNotInLargeBlacklist'] = false; * Introduced $wgVerifyMimeTypeIE to allow disabling the MSIE 6/7 file type detection heuristic on upload, which is more conservative than the checks that were changed above. +* $wgExternalDiffEngine — Setting this to a string value of 'wikidiff', + 'wikidiff2', or 'wikidiff3' will no longer work. This legacy behaviour was + deprecated in MediaWiki 1.27, 1.32, and 1.27, respectively. * $wgSkipSkin — Setting this instead of $wgSkipSkins, deprecated in 1.23, is now hard-deprecated. * $wgLocalInterwiki — Setting this instead of $wgLocalInterwikis, deprecated in diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php index 7e4e53e2b6..6fb37c126f 100644 --- a/includes/diff/DifferenceEngine.php +++ b/includes/diff/DifferenceEngine.php @@ -1288,35 +1288,32 @@ class DifferenceEngine extends ContextSource { } /** - * Process $wgExternalDiffEngine and get a sane, usable engine + * Process ExternalDiffEngine config and get a sane, usable engine * * @return bool|string 'wikidiff2', path to an executable, or false * @internal For use by this class and TextSlotDiffRenderer only. */ public static function getEngine() { - global $wgExternalDiffEngine; - // We use the global here instead of Config because we write to the value, - // and Config is not mutable. - if ( $wgExternalDiffEngine == 'wikidiff' || $wgExternalDiffEngine == 'wikidiff3' ) { - wfDeprecated( "\$wgExternalDiffEngine = '{$wgExternalDiffEngine}'", '1.27' ); - $wgExternalDiffEngine = false; - } elseif ( $wgExternalDiffEngine == 'wikidiff2' ) { - wfDeprecated( "\$wgExternalDiffEngine = '{$wgExternalDiffEngine}'", '1.32' ); - $wgExternalDiffEngine = false; - } elseif ( !is_string( $wgExternalDiffEngine ) && $wgExternalDiffEngine !== false ) { - // And prevent people from shooting themselves in the foot... - wfWarn( '$wgExternalDiffEngine is set to a non-string value, forcing it to false' ); - $wgExternalDiffEngine = false; - } - - if ( is_string( $wgExternalDiffEngine ) && is_executable( $wgExternalDiffEngine ) ) { - return $wgExternalDiffEngine; - } elseif ( $wgExternalDiffEngine === false && function_exists( 'wikidiff2_do_diff' ) ) { + $externalDiffEngine = MediaWikiServices::getInstance()->getMainConfig() + ->get( 'ExternalDiffEngine' ); + + if ( $externalDiffEngine ) { + if ( is_string( $externalDiffEngine ) ) { + if ( is_executable( $externalDiffEngine ) ) { + return $externalDiffEngine; + } + wfDebug( 'ExternalDiffEngine config points to a non-executable, ignoring' ); + } else { + wfWarn( 'ExternalDiffEngine config is set to a non-string value, ignoring' ); + } + } + + if ( function_exists( 'wikidiff2_do_diff' ) ) { return 'wikidiff2'; - } else { - // Native PHP - return false; } + + // Native PHP + return false; } /** diff --git a/tests/phpunit/includes/api/ApiComparePagesTest.php b/tests/phpunit/includes/api/ApiComparePagesTest.php index 9e18eb0985..9b32e9e5d1 100644 --- a/tests/phpunit/includes/api/ApiComparePagesTest.php +++ b/tests/phpunit/includes/api/ApiComparePagesTest.php @@ -10,16 +10,6 @@ class ApiComparePagesTest extends ApiTestCase { protected static $repl = []; - protected function setUp() { - parent::setUp(); - - // Set $wgExternalDiffEngine to something bogus to try to force use of - // the PHP engine rather than wikidiff2. - $this->setMwGlobals( [ - 'wgExternalDiffEngine' => '/dev/null', - ] ); - } - protected function addPage( $page, $text, $model = CONTENT_MODEL_WIKITEXT ) { $title = Title::newFromText( 'ApiComparePagesTest ' . $page ); $content = ContentHandler::makeContent( $text, $title, $model ); diff --git a/tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php b/tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php index e5c23d172b..6836845f62 100644 --- a/tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php +++ b/tests/phpunit/includes/diff/DifferenceEngineSlotDiffRendererTest.php @@ -33,12 +33,4 @@ class DifferenceEngineSlotDiffRendererTest extends MediaWikiIntegrationTestCase $slotDiffRenderer = new DifferenceEngineSlotDiffRenderer( $differenceEngine ); $slotDiffRenderer->addModules( $output ); } - - public function testGetExtraCacheKeys() { - $differenceEngine = new CustomDifferenceEngine(); - $slotDiffRenderer = new DifferenceEngineSlotDiffRenderer( $differenceEngine ); - $extraCacheKeys = $slotDiffRenderer->getExtraCacheKeys(); - $this->assertSame( [ 'foo' ], $extraCacheKeys ); - } - } diff --git a/tests/phpunit/includes/diff/DifferenceEngineTest.php b/tests/phpunit/includes/diff/DifferenceEngineTest.php index c1bb1a0d38..46eaaeaa9d 100644 --- a/tests/phpunit/includes/diff/DifferenceEngineTest.php +++ b/tests/phpunit/includes/diff/DifferenceEngineTest.php @@ -165,12 +165,6 @@ class DifferenceEngineTest extends MediaWikiTestCase { $oldContent = ContentHandler::makeContent( ...$oldContentArgs ); $newContent = ContentHandler::makeContent( ...$newContentArgs ); - // Set $wgExternalDiffEngine to something bogus to try to force use of - // the PHP engine rather than wikidiff2. - $this->setMwGlobals( [ - 'wgExternalDiffEngine' => '/dev/null', - ] ); - $differenceEngine = new DifferenceEngine(); $diff = $differenceEngine->generateContentDiffBody( $oldContent, $newContent ); $this->assertSame( $expectedDiff, $this->getPlainDiff( $diff ) ); @@ -187,12 +181,6 @@ class DifferenceEngineTest extends MediaWikiTestCase { } public function testGenerateTextDiffBody() { - // Set $wgExternalDiffEngine to something bogus to try to force use of - // the PHP engine rather than wikidiff2. - $this->setMwGlobals( [ - 'wgExternalDiffEngine' => '/dev/null', - ] ); - $oldText = "aaa\nbbb\nccc"; $newText = "aaa\nxxx\nccc"; $expectedDiff = " aaa aaa\n-bbb+xxx\n ccc ccc"; @@ -203,12 +191,6 @@ class DifferenceEngineTest extends MediaWikiTestCase { } public function testSetContent() { - // Set $wgExternalDiffEngine to something bogus to try to force use of - // the PHP engine rather than wikidiff2. - $this->setMwGlobals( [ - 'wgExternalDiffEngine' => '/dev/null', - ] ); - $oldContent = ContentHandler::makeContent( 'xxx', null, CONTENT_MODEL_TEXT ); $newContent = ContentHandler::makeContent( 'yyy', null, CONTENT_MODEL_TEXT ); @@ -243,12 +225,6 @@ class DifferenceEngineTest extends MediaWikiTestCase { public function testGetDiffBody( RevisionRecord $oldRevision = null, RevisionRecord $newRevision = null, $expectedDiff ) { - // Set $wgExternalDiffEngine to something bogus to try to force use of - // the PHP engine rather than wikidiff2. - $this->setMwGlobals( [ - 'wgExternalDiffEngine' => '/dev/null', - ] ); - if ( $expectedDiff instanceof Exception ) { $this->setExpectedException( get_class( $expectedDiff ), $expectedDiff->getMessage() ); } diff --git a/tests/phpunit/integration/includes/diff/DifferenceEngineSlotDiffRendererIntegrationTest.php b/tests/phpunit/integration/includes/diff/DifferenceEngineSlotDiffRendererIntegrationTest.php new file mode 100644 index 0000000000..a211001238 --- /dev/null +++ b/tests/phpunit/integration/includes/diff/DifferenceEngineSlotDiffRendererIntegrationTest.php @@ -0,0 +1,19 @@ +setMwGlobals( [ 'wgExternalDiffEngine' => null ] ); + + $differenceEngine = new CustomDifferenceEngine(); + $slotDiffRenderer = new DifferenceEngineSlotDiffRenderer( $differenceEngine ); + $extraCacheKeys = $slotDiffRenderer->getExtraCacheKeys(); + $this->assertSame( [ 'foo' ], $extraCacheKeys ); + } +} -- 2.20.1