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)

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 48fcbaf..d793008 100644 (file)
--- 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.
 * 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
 * $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 7e4e53e..6fb37c1 100644 (file)
--- 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() {
         *
         * @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';
                        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 9e18eb0..9b32e9e 100644 (file)
--- 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 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 );
        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 e5c23d1..6836845 100644 (file)
--- 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 );
        }
                $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 c1bb1a0..46eaaea 100644 (file)
--- 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 );
 
                $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 ) );
                $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() {
        }
 
        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";
                $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() {
        }
 
        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 );
 
                $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
        ) {
        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() );
                }
                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 (file)
index 0000000..a211001
--- /dev/null
+++ b/tests/phpunit/integration/includes/diff/DifferenceEngineSlotDiffRendererIntegrationTest.php
@@ -0,0 +1,19 @@
+<?php
+
+/**
+ * @group small
+ */
+class DifferenceEngineSlotDiffRendererIntegrationTest extends \MediaWikiIntegrationTestCase {
+
+       /**
+        * @covers DifferenceEngineSlotDiffRenderer::getExtraCacheKeys
+        */
+       public function testGetExtraCacheKeys_noExternalDiffEngineConfigured() {
+               $this->setMwGlobals( [ 'wgExternalDiffEngine' => null ] );
+
+               $differenceEngine = new CustomDifferenceEngine();
+               $slotDiffRenderer = new DifferenceEngineSlotDiffRenderer( $differenceEngine );
+               $extraCacheKeys = $slotDiffRenderer->getExtraCacheKeys();
+               $this->assertSame( [ 'foo' ], $extraCacheKeys );
+       }
+}