Merge "Pick ParsoidClient implementation based on etag."

extension.json

@@ -111,6 +111,10 @@
 			"value": true,
 			"description": "Deprecated since MW 1.40. Unused."
 		},
+		"VisualEditorDefaultParsoidClient": {
+			"value": "vrs",
+			"description": "Sets the mechanism used by the VE API to talk to Parsoid. This should be removed as soon as the direct mode is stable."
+		},
 		"VisualEditorParsoidSettings": {
 			"value": []
 		},

includes/DualParsoidClient.php

@@ -0,0 +1,173 @@
+<?php
+
+namespace MediaWiki\Extension\VisualEditor;
+
+use Language;
+use MediaWiki\Page\PageIdentity;
+use MediaWiki\Permissions\Authority;
+use MediaWiki\Revision\RevisionRecord;
+
+/**
+ * A decorator implementation of ParsoidClient that will delegate to the appropriate
+ * implementation of ParsoidClient based on the incoming ETag.
+ *
+ * The purpose of this decorator is to ensure that VE sessions that loaded HTML from
+ * one ParsoidClient implementation will use the same implementation when saving the HTML,
+ * even when the preferred implementation was changed on the server while the editor was open.
+ *
+ * This avoids users losing edits at the time of the config change: if the HTML the user
+ * submits when saving the page doesn't get handled by the same implementation that originally
+ * provided the HTML for editing, the ETag will mismatch and the edit will fail.
+ */
+class DualParsoidClient implements ParsoidClient {
+
+	/** @var VisualEditorParsoidClientFactory */
+	private VisualEditorParsoidClientFactory $factory;
+
+	/** @var string|string[]|null */
+	private $cookiesToForward;
+
+	/** @var Authority */
+	private Authority $authority;
+
+	/**
+	 * @note Called by DiscussionTools, keep compatible!
+	 *
+	 * @param VisualEditorParsoidClientFactory $factory
+	 * @param string|string[]|false $cookiesToForward
+	 * @param Authority $authority
+	 */
+	public function __construct(
+		VisualEditorParsoidClientFactory $factory,
+		$cookiesToForward,
+		Authority $authority
+	) {
+		$this->factory = $factory;
+		$this->cookiesToForward = $cookiesToForward;
+		$this->authority = $authority;
+	}
+
+	/**
+	 * Detect the mode to use based on the given ETag
+	 *
+	 * @param string $etag
+	 *
+	 * @return string|null
+	 */
+	private static function detectMode( string $etag ): ?string {
+		// Extract the mode from between the double-quote and the colon
+		if ( preg_match( '/^(W\/)?"(\w+):/', $etag, $matches ) ) {
+			return $matches[2];
+		}
+
+		return null;
+	}
+
+	/**
+	 * Inject information about what ParsoidClient implementation was used
+	 * into the ETag header.
+	 *
+	 * @param array &$result
+	 * @param ParsoidClient $client
+	 */
+	private static function injectMode( array &$result, ParsoidClient $client ) {
+		$mode = $client instanceof VRSParsoidClient ? 'vrs' : 'direct';
+
+		if ( isset( $result['headers']['etag'] ) ) {
+			$etag = $result['headers']['etag'];
+
+			// Inject $mode after double-quote
+			$result['headers']['etag'] = preg_replace( '/^(W\/)?"(.*)"$/', '$1"' . $mode . ':$2"', $etag );
+		}
+	}
+
+	/**
+	 * Strip information about what ParsoidClient implementation to use from the ETag,
+	 * restoring it to the original ETag originally emitted by that ParsoidClient.
+	 *
+	 * @param string $etag
+	 *
+	 * @return string
+	 */
+	private static function stripMode( string $etag ): string {
+		// Remove any prefix between double-quote and colon
+		return preg_replace( '/"(\w+):/', '"', $etag );
+	}
+
+	/**
+	 * Create a ParsoidClient based on information embedded in the given ETag.
+	 *
+	 * @param string|null $etag
+	 *
+	 * @return ParsoidClient
+	 */
+	private function createParsoidClient( ?string $etag = null ): ParsoidClient {
+		$shouldUseVRS = null;
+
+		if ( $etag ) {
+			$mode = self::detectMode( $etag );
+			if ( $mode === 'vrs' ) {
+				$shouldUseVRS = true;
+			} elseif ( $mode === 'direct' ) {
+				$shouldUseVRS = false;
+			}
+		}
+
+		return $this->factory->createParsoidClientInternal(
+			$this->cookiesToForward,
+			$this->authority,
+			[ 'ShouldUseVRS' => $shouldUseVRS, 'NoDualClient' => true ]
+		);
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function getPageHtml( RevisionRecord $revision, ?Language $targetLanguage ): array {
+		$client = $this->createParsoidClient();
+		$result = $client->getPageHtml( $revision, $targetLanguage );
+
+		self::injectMode( $result, $client );
+		return $result;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function transformHTML(
+		PageIdentity $page,
+		Language $targetLanguage,
+		string $html,
+		?int $oldid,
+		?string $etag
+	): array {
+		$client = $this->createParsoidClient( $etag );
+
+		if ( $etag ) {
+			$etag = self::stripMode( $etag );
+		}
+
+		$result = $client->transformHTML( $page, $targetLanguage, $html, $oldid, $etag );
+
+		self::injectMode( $result, $client );
+		return $result;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function transformWikitext(
+		PageIdentity $page,
+		Language $targetLanguage,
+		string $wikitext,
+		bool $bodyOnly,
+		?int $oldid,
+		bool $stash
+	): array {
+		$client = $this->createParsoidClient();
+		$result = $client->transformWikitext( $page, $targetLanguage, $wikitext, $bodyOnly, $oldid, $stash );
+
+		self::injectMode( $result, $client );
+		return $result;
+	}
+}

includes/VisualEditorParsoidClientFactory.php

@@ -39,9 +39,13 @@ class VisualEditorParsoidClientFactory {
 	 */
 	public const CONSTRUCTOR_OPTIONS = [
 		MainConfigNames::VirtualRestConfig,
-		self::ENABLE_COOKIE_FORWARDING
+		self::ENABLE_COOKIE_FORWARDING,
+		self::DEFAULT_PARSOID_CLIENT_SETTING,
 	];
 
+	/** @var string */
+	public const DEFAULT_PARSOID_CLIENT_SETTING = 'VisualEditorDefaultParsoidClient';
+
 	/** @var HttpRequestFactory */
 	private $httpRequestFactory;
 
@@ -100,15 +104,47 @@ class VisualEditorParsoidClientFactory {
 	 *
 	 * @param string|string[]|false $cookiesToForward
 	 * @param Authority|null $performer
+	 * @param array $hints An associative array of hints for client creation.
 	 *
 	 * @return ParsoidClient
 	 */
-	public function createParsoidClient( $cookiesToForward, ?Authority $performer = null ): ParsoidClient {
+	public function createParsoidClient(
+		$cookiesToForward,
+		?Authority $performer = null,
+		array $hints = []
+	): ParsoidClient {
 		if ( $performer === null ) {
 			$performer = RequestContext::getMain()->getAuthority();
 		}
 
-		if ( $this->useParsoidOverHTTP() ) {
+		if ( empty( $hints['NoDualClient'] ) ) {
+			return new DualParsoidClient( $this, $cookiesToForward, $performer );
+		}
+	}
+
+	/**
+	 * Create a ParsoidClient for accessing Parsoid.
+	 *
+	 * @internal For use by DualParsoidClient only.
+	 *
+	 * @param string|string[]|false $cookiesToForward
+	 * @param Authority $performer
+	 * @param array $hints An associative array of hints for client creation.
+	 *
+	 * @return ParsoidClient
+	 */
+	public function createParsoidClientInternal(
+		$cookiesToForward,
+		Authority $performer,
+		array $hints = []
+	): ParsoidClient {
+		// TODO: Delete when we no longer support VRS
+		$shouldUseVRS = $hints['ShouldUseVRS'] ?? null;
+		if ( $shouldUseVRS === null ) {
+			$shouldUseVRS = ( $this->options->get( self::DEFAULT_PARSOID_CLIENT_SETTING ) === 'vrs' );
+		}
+
+		if ( $shouldUseVRS && $this->canUseParsoidOverHTTP() ) {
 			$client = new VRSParsoidClient(
 				$this->getVRSClient( $cookiesToForward ),
 				$this->logger
@@ -120,7 +156,23 @@ class VisualEditorParsoidClientFactory {
 		return $client;
 	}
 
+	/**
+	 * Whether Parsoid should be used over HTTP, according to the configuration.
+	 * Note that we may still end up using direct mode, depending on information
+	 * from the request.
+	 *
+	 * @return bool
+	 */
 	public function useParsoidOverHTTP(): bool {
+		$shouldUseVRS = ( $this->options->get( self::DEFAULT_PARSOID_CLIENT_SETTING ) === 'vrs' );
+		return $this->canUseParsoidOverHTTP() && $shouldUseVRS;
+	}
+
+	/**
+	 * Whether Parsoid could be used over HTTP, based on the configuration provided.
+	 * @return bool
+	 */
+	private function canUseParsoidOverHTTP(): bool {
 		// If we have VRS modules configured, use them
 		$vrs = $this->options->get( MainConfigNames::VirtualRestConfig );
 		if ( isset( $vrs['modules'] ) &&

tests/api-testing/edit.js

@@ -34,7 +34,7 @@ describe( 'Visual Editor API', function () {
 		assert.nestedProperty( result.visualeditor, 'checkboxesMessages' );
 
 		assert.nestedProperty( result.visualeditor, 'etag' );
-		assert.match( result.visualeditor.etag, /^(W\/)?"\d+\// );
+		assert.match( result.visualeditor.etag, /^(W\/)?".*\d+\// );
 
 		assert.nestedProperty( result.visualeditor, 'oldid' );
 		assert.equal( result.visualeditor.oldid, pageInfo.newrevid );

tests/phpunit/integration/DualParsoidClientTest.php

@@ -0,0 +1,200 @@
+<?php
+
+namespace MediaWiki\Extension\VisualEditor\Tests;
+
+use Language;
+use MediaWiki\Extension\VisualEditor\DirectParsoidClient;
+use MediaWiki\Extension\VisualEditor\DualParsoidClient;
+use MediaWiki\Extension\VisualEditor\ParsoidClient;
+use MediaWiki\Extension\VisualEditor\VisualEditorParsoidClientFactory;
+use MediaWiki\Extension\VisualEditor\VRSParsoidClient;
+use MediaWiki\Page\PageIdentity;
+use MediaWiki\Page\PageIdentityValue;
+use MediaWiki\Permissions\Authority;
+use MediaWiki\Revision\RevisionRecord;
+use MediaWikiIntegrationTestCase;
+
+/**
+ * @covers \MediaWiki\Extension\VisualEditor\DualParsoidClient
+ * @group Database
+ */
+class DualParsoidClientTest extends MediaWikiIntegrationTestCase {
+
+	/**
+	 * @param array $hints
+	 * @param string $default
+	 *
+	 * @return ParsoidClient
+	 */
+	public function createMockClient( array $hints, string $default ) {
+		$vrs = $hints[ 'ShouldUseVRS' ] ?? ( $default === 'vrs' );
+		$class = $vrs ? VRSParsoidClient::class : DirectParsoidClient::class;
+
+		$client = $this->createMock( $class );
+
+		$client->method( 'getPageHtml' )->willReturnCallback(
+			static function () use ( $vrs ) {
+				return [
+					'body' => $vrs ? 'mode:vrs' : 'mode:direct',
+					'headers' => [
+						'etag' => '"abcdef1234"',
+					]
+				];
+			}
+		);
+
+		$client->method( 'transformWikitext' )->willReturnCallback(
+			static function () use ( $vrs ) {
+				return [
+					'body' => $vrs ? 'mode:vrs' : 'mode:direct',
+					'headers' => [
+						'etag' => '"abcdef1234"',
+					]
+				];
+			}
+		);
+
+		$client->method( 'transformHTML' )->willReturnCallback(
+			static function (
+				PageIdentity $page,
+				Language $targetLanguage,
+				string $html,
+				?int $oldid,
+				?string $etag
+			) use ( $vrs ) {
+				return [
+					'body' => ( $vrs ? 'mode:vrs' : 'mode:direct' ) . '; etag:' . $etag,
+					'headers' => []
+				];
+			}
+		);
+
+		return $client;
+	}
+
+	/**
+	 * @param string $defaultMode
+	 *
+	 * @return VisualEditorParsoidClientFactory
+	 */
+	private function createClientFactory( string $defaultMode ) {
+		$factory = $this->createMock( VisualEditorParsoidClientFactory::class );
+
+		$factory->method( 'createParsoidClientInternal' )->willReturnCallback(
+			function ( $cookiesToForward, ?Authority $performer = null, array $hints = [] ) use ( $defaultMode ) {
+				return $this->createMockClient( $hints, $defaultMode );
+			}
+		);
+
+		return $factory;
+	}
+
+	/**
+	 * @param string $defaultMode
+	 *
+	 * @return DualParsoidClient
+	 */
+	private function createDualClient( string $defaultMode ): DualParsoidClient {
+		$directClient = new DualParsoidClient(
+			$this->createClientFactory( $defaultMode ),
+			false,
+			$this->createNoOpMock( Authority::class )
+		);
+
+		return $directClient;
+	}
+
+	public function provideDefaultModes() {
+		yield 'direct' => [ 'direct' ];
+		yield 'vrs' => [ 'vrs' ];
+	}
+
+	/**
+	 * @dataProvider provideDefaultModes
+	 */
+	public function testGetPageHTML( $default ) {
+		$client = $this->createDualClient( $default );
+		$result = $client->getPageHtml( $this->createNoOpMock( RevisionRecord::class ), null );
+
+		$this->assertSame( 'mode:' . $default, $result['body'] );
+
+		$etag = $result['headers']['etag'];
+		$this->assertStringContainsString( '"' . $default . ':', $etag );
+
+		// Check round trip using the etag returned by the call above
+		$client = $this->createDualClient( 'xyzzy' );
+		$result = $client->transformHTML(
+			PageIdentityValue::localIdentity( 0, NS_MAIN, 'Dummy' ),
+			$this->createNoOpMock( Language::class ),
+			'input html',
+			null,
+			$etag
+		);
+		$this->assertStringContainsString( 'mode:' . $default, $result['body'] );
+	}
+
+	/**
+	 * @dataProvider provideDefaultModes
+	 */
+	public function testTransformWikitext( $default ) {
+		$client = $this->createDualClient( $default );
+		$result = $client->transformWikitext(
+			PageIdentityValue::localIdentity( 0, NS_MAIN, 'Dummy' ),
+			$this->createNoOpMock( Language::class ),
+			'input wikitext',
+			false,
+			null,
+			false
+		);
+
+		$this->assertSame( 'mode:' . $default, $result['body'] );
+
+		$etag = $result['headers']['etag'];
+		$this->assertStringContainsString( '"' . $default . ':', $etag );
+
+		// Check round trip using the etag returned by the call above
+		$client = $this->createDualClient( 'xyzzy' );
+		$result = $client->transformHTML(
+			PageIdentityValue::localIdentity( 0, NS_MAIN, 'Dummy' ),
+			$this->createNoOpMock( Language::class ),
+			'input html',
+			null,
+			$etag
+		);
+		$this->assertStringContainsString( 'mode:' . $default, $result['body'] );
+	}
+
+	public function provideTransformHTML() {
+		$fallbackMode = 'direct';
+
+		yield 'no etag' => [ null, $fallbackMode ];
+		yield 'etag without prefix' => [ '"abcdef1234"', $fallbackMode ];
+		yield 'etag with bogus prefix' => [ '"bogus:abcdef1234"', $fallbackMode ];
+		yield 'etag with direct prefix' => [ '"direct:abcdef1234"', 'direct' ];
+		yield 'etag with vrs prefix' => [ '"vrs:abcdef1234"', 'vrs' ];
+		yield 'weak etag with vrs prefix' => [ 'W/"vrs:abcdef1234"', 'vrs' ];
+	}
+
+	/**
+	 * @dataProvider provideTransformHTML
+	 */
+	public function testTransformHTML( $etag, $mode ) {
+		$client = $this->createDualClient( 'direct' );
+
+		$result = $client->transformHTML(
+			PageIdentityValue::localIdentity( 0, NS_MAIN, 'Dummy' ),
+			$this->createNoOpMock( Language::class ),
+			'input html',
+			null,
+			$etag
+		);
+
+		$this->assertStringContainsString( "mode:$mode", $result['body'] );
+
+		if ( $etag ) {
+			$this->assertStringContainsString( "abcdef", $result['body'] );
+			$this->assertStringNotContainsString( "etag:\"$mode:", $result['body'] );
+		}
+	}
+
+}

tests/phpunit/integration/HooksTest.php

@@ -32,43 +32,61 @@ class HooksTest extends MediaWikiIntegrationTestCase {
 	public function provideOnResourceLoaderGetConfigVars() {
 		// TODO: test a lot more config!
 
-		yield 'restbaseUrl: No VRS modules' => [
+		yield 'restbaseUrl: No VRS modules, DefaultParsoidClient=vrs' => [
 			[
 				'VirtualRestConfig' => [ 'modules' => [] ],
 				'VisualEditorRestbaseURL' => 'parsoid-url',
 				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'vrs',
 			],
 			[
 				'restbaseUrl' => false,
 				'fullRestbaseUrl' => false,
 			]
 		];
-		yield 'restbaseUrl: VRS modules available' => [
+		yield 'restbaseUrl: VRS modules available, DefaultParsoidClient=vrs' => [
 			[
 				'VirtualRestConfig' => [ 'modules' => [
 					'parsoid' => true,
 				] ],
 				'VisualEditorRestbaseURL' => 'parsoid-url',
 				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'vrs',
 			],
 			[
 				'restbaseUrl' => 'parsoid-url',
 				'fullRestbaseUrl' => 'full-parsoid-url',
 			]
 		];
-		yield 'restbaseUrl: VRS modules available, but no direct access URLs' => [
+		yield 'restbaseUrl: VRS modules available, but no direct access URLs. DefaultParsoidClient=vrs' => [
 			[
 				'VirtualRestConfig' => [ 'modules' => [
 					'parsoid' => true,
 				] ],
 				'VisualEditorRestbaseURL' => 'parsoid-url',
 				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'vrs',
 			],
 			[
 				'restbaseUrl' => 'parsoid-url',
 				'fullRestbaseUrl' => 'full-parsoid-url',
 			]
 		];
+
+		yield 'restbaseUrl: VRS modules available, but DefaultParsoidClient=direct' => [
+			[
+				'VirtualRestConfig' => [ 'modules' => [
+					'parsoid' => true,
+				] ],
+				'VisualEditorRestbaseURL' => 'parsoid-url',
+				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'direct',
+			],
+			[
+				'restbaseUrl' => false,
+				'fullRestbaseUrl' => false,
+			]
+		];
 	}
 
 }

tests/phpunit/integration/VisualEditorParsoidClientFactoryTest.php

@@ -6,12 +6,14 @@ use IBufferingStatsdDataFactory;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Edit\ParsoidOutputStash;
 use MediaWiki\Extension\VisualEditor\DirectParsoidClient;
+use MediaWiki\Extension\VisualEditor\DualParsoidClient;
 use MediaWiki\Extension\VisualEditor\VisualEditorParsoidClientFactory;
 use MediaWiki\Extension\VisualEditor\VRSParsoidClient;
 use MediaWiki\Http\HttpRequestFactory;
 use MediaWiki\MainConfigNames;
 use MediaWiki\Parser\Parsoid\HTMLTransformFactory;
 use MediaWiki\Parser\Parsoid\ParsoidOutputAccess;
+use MediaWiki\Permissions\Authority;
 use MediaWikiIntegrationTestCase;
 use MultiHttpClient;
 use ParsoidVirtualRESTService;
@@ -53,61 +55,123 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase
 	}
 
 	public function provideGetClient() {
-		yield [
+		yield 'Empty VRS modules array, DefaultParsoidClient=vrs, no hints' => [
 			[
 				MainConfigNames::ParsoidSettings => [],
 				MainConfigNames::VirtualRestConfig => [
 					'modules' => []
 				],
 				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
 			],
-			false
+			[],
+			DirectParsoidClient::class
 		];
 
-		yield [
+		yield 'No VRS modules array, DefaultParsoidClient=vrs, no hints' => [
 			[
 				MainConfigNames::ParsoidSettings => [],
 				MainConfigNames::VirtualRestConfig => [],
 				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
 			],
-			false
+			[],
+			DirectParsoidClient::class
 		];
 
-		yield [
+		yield 'restbase module defined, DefaultParsoidClient=vrs, no hints' => [
 			[
 				MainConfigNames::ParsoidSettings => [],
 				MainConfigNames::VirtualRestConfig => [
 					'modules' => [ 'restbase' => [] ]
 				],
 				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
 			],
-			true
+			[],
+			VRSParsoidClient::class
 		];
 
-		yield [
+		yield 'parsoid module defined, DefaultParsoidClient=vrs, no hints' => [
 			[
 				MainConfigNames::ParsoidSettings => [],
 				MainConfigNames::VirtualRestConfig => [
 					'modules' => [ 'parsoid' => [] ]
 				],
 				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
 			],
-			true
+			[],
+			VRSParsoidClient::class
+		];
+
+		yield 'parsoid module defined, DefaultParsoidClient=direct, no hints' => [
+			[
+				MainConfigNames::ParsoidSettings => [],
+				MainConfigNames::VirtualRestConfig => [
+					'modules' => [ 'parsoid' => [] ]
+				],
+				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'direct',
+			],
+			[],
+			DirectParsoidClient::class
+		];
+
+		yield 'parsoid module defined, DefaultParsoidClient=direct, ShouldUseVRS=true' => [
+			[
+				MainConfigNames::ParsoidSettings => [],
+				MainConfigNames::VirtualRestConfig => [
+					'modules' => [ 'parsoid' => [] ]
+				],
+				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'direct',
+			],
+			[ 'ShouldUseVRS' => true ],
+			VRSParsoidClient::class
+		];
+
+		yield 'parsoid module define, ShouldUseVRS = false' => [
+			[
+				MainConfigNames::ParsoidSettings => [],
+				MainConfigNames::VirtualRestConfig => [
+					'modules' => [ 'parsoid' => [] ]
+				],
+				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
+			],
+			[ 'ShouldUseVRS' => false ],
+			DirectParsoidClient::class
+		];
+
+		yield 'No VRS modules array, ShouldUseVRS = true' => [
+			[
+				MainConfigNames::ParsoidSettings => [],
+				MainConfigNames::VirtualRestConfig => [],
+				VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => false,
+				VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
+			],
+			[ 'ShouldUseVRS' => true ],
+			DirectParsoidClient::class
 		];
 	}
 
 	/**
 	 * @dataProvider provideGetClient
+	 * @covers ::createParsoidClientInternal
 	 * @covers ::createParsoidClient
 	 */
-	public function testGetClient( $optionValues, $useRestbase ) {
-		$expectedType = $useRestbase ? VRSParsoidClient::class : DirectParsoidClient::class;
+	public function testGetClient( $optionValues, $hints, $expectedType ) {
+		$authority = $this->createNoOpMock( Authority::class );
 
 		$factory = $this->newClientFactory( $optionValues );
-		$this->assertSame( $useRestbase, $factory->useParsoidOverHTTP() );
 
-		$client = $factory->createParsoidClient( false );
+		$client = $factory->createParsoidClientInternal( false, $authority, $hints );
 		$this->assertInstanceOf( $expectedType, $client );
+
+		// This just checks that nothing explodes.
+		$client = $factory->createParsoidClient( false, $authority );
+		$this->assertInstanceOf( DualParsoidClient::class, $client );
 	}
 
 	public function provideCookieToForward() {
@@ -123,6 +187,8 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase
 	 * @covers ::createParsoidClient
 	 */
 	public function testGetVRSClientForwardedCookies( $cookie, $expectedCookie ) {
+		$authority = $this->createNoOpMock( Authority::class );
+
 		$optionValues = [
 			MainConfigNames::ParsoidSettings => [],
 			MainConfigNames::VirtualRestConfig => [
@@ -133,10 +199,11 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase
 					]
 				]
 			],
-			VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => true
+			VisualEditorParsoidClientFactory::ENABLE_COOKIE_FORWARDING => true,
+			VisualEditorParsoidClientFactory::DEFAULT_PARSOID_CLIENT_SETTING => 'vrs',
 		];
 
-		$parsoidClient = $this->newClientFactory( $optionValues )->createParsoidClient( $cookie );
+		$parsoidClient = $this->newClientFactory( $optionValues )->createParsoidClientInternal( $cookie, $authority );
 		$vrsClient = TestingAccessWrapper::newFromObject( $parsoidClient )->vrsClient;
 
 		$mountAndService = $vrsClient->getMountAndService( '/restbase/' );
@@ -176,4 +243,66 @@ class VisualEditorParsoidClientFactoryTest extends MediaWikiIntegrationTestCase
 		$this->assertArrayHasKey( 'Host', $res[0]['headers'] );
 	}
 
+	/**
+	 * @dataProvider provideUseParsoidOverHTTP
+	 * @covers ::useParsoidOverHTTP
+	 */
+	public function testUseParsoidOverHTTP( array $optionValues, bool $expected ) {
+		$parsoidClient = $this->newClientFactory( $optionValues );
+
+		$this->assertSame( $expected, $parsoidClient->useParsoidOverHTTP() );
+	}
+
+	public function provideUseParsoidOverHTTP() {
+		// TODO: test a lot more config!
+
+		yield 'restbaseUrl: No VRS modules, DefaultParsoidClient=vrs' => [
+			[
+				'VirtualRestConfig' => [ 'modules' => [] ],
+				'VisualEditorRestbaseURL' => 'parsoid-url',
+				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'vrs',
+				'EnableCookieForwarding' => true,
+			],
+			false
+		];
+		yield 'restbaseUrl: VRS modules available, DefaultParsoidClient=vrs' => [
+			[
+				'VirtualRestConfig' => [ 'modules' => [
+					'parsoid' => true,
+				] ],
+				'VisualEditorRestbaseURL' => 'parsoid-url',
+				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'vrs',
+				'EnableCookieForwarding' => true,
+			],
+			true
+		];
+		yield 'restbaseUrl: VRS modules available, but no direct access URLs. DefaultParsoidClient=vrs' => [
+			[
+				'VirtualRestConfig' => [ 'modules' => [
+					'parsoid' => true,
+				] ],
+				'VisualEditorRestbaseURL' => 'parsoid-url',
+				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'vrs',
+				'EnableCookieForwarding' => true,
+			],
+			true
+		];
+
+		yield 'restbaseUrl: VRS modules available, but DefaultParsoidClient=direct' => [
+			[
+				'VirtualRestConfig' => [ 'modules' => [
+					'parsoid' => true,
+				] ],
+				'VisualEditorRestbaseURL' => 'parsoid-url',
+				'VisualEditorFullRestbaseURL' => 'full-parsoid-url',
+				'VisualEditorDefaultParsoidClient' => 'direct',
+				'EnableCookieForwarding' => true,
+			],
+			false
+		];
+	}
+
 }