From 07afdba500b3709b9d84c71d55a4025226cb6bb4 Mon Sep 17 00:00:00 2001 From: daniel Date: Thu, 13 Oct 2022 12:05:13 +0200 Subject: [PATCH] Don't try to contact RESTbase directly when in PHP direct mode. If VE is configured to call parsoid directly in PHP, the client should not try to fetch HTML from restbase. Doing so will lead to inconsistencies, and may cause edits to fail. Bug: T320704 Bug: T320703 Change-Id: I98bfdd305dcd188d91db6a8fe5885156cdeca7db --- includes/Hooks.php | 8 +++-- includes/VisualEditorParsoidClientFactory.php | 32 +++++++++++-------- .../VisualEditorParsoidClientFactoryTest.php | 16 ++++++---- 3 files changed, 34 insertions(+), 22 deletions(-) diff --git a/includes/Hooks.php b/includes/Hooks.php index 6ab19112a9..ddc17368c7 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -1077,6 +1077,10 @@ class Hooks { ) ); + $parsoidClientFactory = + MediaWikiServices::getInstance()->getService( VisualEditorParsoidClientFactory::SERVICE_NAME ); + $useRestbase = $parsoidClientFactory->useRestbase(); + $namespacesWithSubpages = $coreConfig->get( 'NamespacesWithSubpages' ); // Export as a list of namespaces where subpages are enabled instead of an object // mapping namespaces to if subpages are enabled or not, so filter out disabled @@ -1124,8 +1128,8 @@ class Hooks { 'namespacesWithSubpages' => $namespacesWithSubpagesEnabled, 'specialBooksources' => urldecode( SpecialPage::getTitleFor( 'Booksources' )->getPrefixedURL() ), 'rebaserUrl' => $coreConfig->get( 'VisualEditorRebaserURL' ), - 'restbaseUrl' => $coreConfig->get( 'VisualEditorRestbaseURL' ), - 'fullRestbaseUrl' => $coreConfig->get( 'VisualEditorFullRestbaseURL' ), + 'restbaseUrl' => $useRestbase ? $coreConfig->get( 'VisualEditorRestbaseURL' ) : false, + 'fullRestbaseUrl' => $useRestbase ? $coreConfig->get( 'VisualEditorFullRestbaseURL' ) : false, 'allowLossySwitching' => $coreConfig->get( 'VisualEditorAllowLossySwitching' ), 'feedbackApiUrl' => $veConfig->get( 'VisualEditorFeedbackAPIURL' ), 'feedbackTitle' => $veConfig->get( 'VisualEditorFeedbackTitle' ), diff --git a/includes/VisualEditorParsoidClientFactory.php b/includes/VisualEditorParsoidClientFactory.php index 412a3b2a88..3b78fc6673 100644 --- a/includes/VisualEditorParsoidClientFactory.php +++ b/includes/VisualEditorParsoidClientFactory.php @@ -111,28 +111,20 @@ class VisualEditorParsoidClientFactory { if ( $performer === null ) { $performer = RequestContext::getMain()->getAuthority(); } - // Default to using the direct client. - $client = $this->createDirectClient( $performer ); - if ( !$client ) { - // Default to using the direct client. + if ( $this->useRestbase() ) { $client = new VRSParsoidClient( $this->getVRSClient( $cookiesToForward ), $this->logger ); + } else { + $client = $this->createDirectClient( $performer ); } return $client; } - /** - * Create a ParsoidClient for accessing Parsoid. - * - * @param Authority $performer - * - * @return ?ParsoidClient - */ - private function createDirectClient( Authority $performer ): ?ParsoidClient { + public function useRestbase(): bool { // We haven't checked configuration yet. // Check to see if any of the restbase-related configuration // variables are set, and bail if so: @@ -141,14 +133,26 @@ class VisualEditorParsoidClientFactory { ( isset( $vrs['modules']['restbase'] ) || isset( $vrs['modules']['parsoid'] ) ) ) { - return null; + return true; } + // Eventually we'll do something fancy, but I'm hacking here... if ( !$this->options->get( self::USE_AUTO_CONFIG ) ) { // explicit opt out - return null; + return true; } + return false; + } + + /** + * Create a ParsoidClient for accessing Parsoid. + * + * @param Authority $performer + * + * @return ParsoidClient + */ + private function createDirectClient( Authority $performer ): ParsoidClient { return new DirectParsoidClient( $this->parsoidOutputStash, $this->statsDataFactory, diff --git a/tests/phpunit/integration/VisualEditorParsoidClientFactoryTest.php b/tests/phpunit/integration/VisualEditorParsoidClientFactoryTest.php index 057b288395..6d3d352e1e 100644 --- a/tests/phpunit/integration/VisualEditorParsoidClientFactoryTest.php +++ b/tests/phpunit/integration/VisualEditorParsoidClientFactoryTest.php @@ -62,7 +62,7 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false, VisualEditorParsoidClientFactory::USE_AUTO_CONFIG => true ], - DirectParsoidClient::class + false ]; yield [ @@ -72,7 +72,7 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false, VisualEditorParsoidClientFactory::USE_AUTO_CONFIG => true ], - DirectParsoidClient::class + false ]; yield [ @@ -84,7 +84,7 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false, VisualEditorParsoidClientFactory::USE_AUTO_CONFIG => true ], - VRSParsoidClient::class + true ]; yield [ @@ -96,7 +96,7 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false, VisualEditorParsoidClientFactory::USE_AUTO_CONFIG => true ], - VRSParsoidClient::class + true ]; } @@ -104,9 +104,13 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase * @dataProvider provideGetClient * @covers ::createParsoidClient */ - public function testGetClient( $optionValues, $expectedType ) { - $client = $this->newClientFactory( $optionValues )->createParsoidClient( false ); + public function testGetClient( $optionValues, $useRestbase ) { + $expectedType = $useRestbase ? VRSParsoidClient::class : DirectParsoidClient::class; + $factory = $this->newClientFactory( $optionValues ); + $this->assertSame( $useRestbase, $factory->useRestbase() ); + + $client = $factory->createParsoidClient( false ); $this->assertInstanceOf( $expectedType, $client ); }