From a565e388f9ec7c00a5566440fcb8572fe457dbca Mon Sep 17 00:00:00 2001 From: "C. Scott Ananian" Date: Wed, 22 May 2024 10:08:45 -0400 Subject: [PATCH] Move ParsoidOutputAccess::supportsContentModel() into Parsoid SiteConfig The `supportsContentModel` method is really querying Parsoid for the set of content models it supports, so it makes sense to put it in the Parsoid-specific SiteConfig service. This is part of the work to deprecate and remove ParsoidOutputAccess. Change-Id: I81eb2df8cef93ede95361a4e03185b3d58e5b84b --- docs/config-schema.yaml | 2 +- includes/MainConfigSchema.php | 3 +- includes/MediaWikiServices.php | 2 +- includes/ServiceWiring.php | 4 +-- includes/config-schema.php | 1 + .../jobqueue/jobs/ParsoidCachePrewarmJob.php | 9 +++-- includes/parser/Parsoid/Config/SiteConfig.php | 35 ++++++++++++++++++ .../parser/Parsoid/ParsoidOutputAccess.php | 31 ++-------------- maintenance/prewarmParsoidParserCache.php | 9 ++++- .../includes/jobqueue/JobFactoryTest.php | 3 +- tests/phpunit/includes/jobqueue/JobTest.php | 3 +- .../jobs/ParsoidCachePrewarmJobTest.php | 6 ++-- tests/phpunit/includes/page/ArticleTest.php | 4 +-- .../Rest/Handler/ParsoidOutputAccessTest.php | 25 ------------- .../parser/Parsoid/Config/SiteConfigTest.php | 36 +++++++++++++++++++ .../parser/Parsoid/Config/SiteConfigTest.php | 2 ++ 16 files changed, 106 insertions(+), 69 deletions(-) create mode 100644 tests/phpunit/integration/includes/parser/Parsoid/Config/SiteConfigTest.php diff --git a/docs/config-schema.yaml b/docs/config-schema.yaml index 2f4f3a8e1f9..ed6f8bbd87b 100644 --- a/docs/config-schema.yaml +++ b/docs/config-schema.yaml @@ -7141,7 +7141,7 @@ config-schema: revertedTagUpdate: RevertedTagUpdateJob 'null': NullJob userEditCountInit: UserEditCountInitJob - parsoidCachePrewarm: { class: ParsoidCachePrewarmJob, services: [ParsoidOutputAccess, PageStore, RevisionLookup], needsPage: false } + parsoidCachePrewarm: { class: ParsoidCachePrewarmJob, services: [ParsoidOutputAccess, PageStore, RevisionLookup, ParsoidSiteConfig], needsPage: false } renameUser: { class: MediaWiki\RenameUser\RenameUserJob, services: [MainConfig, DBLoadBalancerFactory] } type: object description: |- diff --git a/includes/MainConfigSchema.php b/includes/MainConfigSchema.php index ad9bafc82a9..93bc0b38dc8 100644 --- a/includes/MainConfigSchema.php +++ b/includes/MainConfigSchema.php @@ -11445,7 +11445,8 @@ class MainConfigSchema { 'services' => [ 'ParsoidOutputAccess', 'PageStore', - 'RevisionLookup' + 'RevisionLookup', + 'ParsoidSiteConfig', ], // tell the JobFactory not to include the $page parameter in the constructor call 'needsPage' => false diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index 7ec176d3d81..e520dbe4622 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -118,6 +118,7 @@ use MediaWiki\Parser\MagicWordFactory; use MediaWiki\Parser\Parser; use MediaWiki\Parser\ParserCacheFactory; use MediaWiki\Parser\Parsoid\Config\PageConfigFactory; +use MediaWiki\Parser\Parsoid\Config\SiteConfig; use MediaWiki\Parser\Parsoid\HtmlTransformFactory; use MediaWiki\Parser\Parsoid\ParsoidOutputAccess; use MediaWiki\Parser\Parsoid\ParsoidParserFactory; @@ -208,7 +209,6 @@ use Wikimedia\Message\IMessageFormatterFactory; use Wikimedia\NonSerializable\NonSerializableTrait; use Wikimedia\ObjectFactory\ObjectFactory; use Wikimedia\Parsoid\Config\DataAccess; -use Wikimedia\Parsoid\Config\SiteConfig; use Wikimedia\Rdbms\ChronologyProtector; use Wikimedia\Rdbms\ConfiguredReadOnlyMode; use Wikimedia\Rdbms\DatabaseFactory; diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 5e1d77a0ef5..4a37803561a 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -245,7 +245,6 @@ use Wikimedia\EventRelayer\EventRelayerGroup; use Wikimedia\Message\IMessageFormatterFactory; use Wikimedia\ObjectFactory\ObjectFactory; use Wikimedia\Parsoid\Config\DataAccess; -use Wikimedia\Parsoid\Config\SiteConfig; use Wikimedia\Parsoid\Parsoid; use Wikimedia\Rdbms\ChronologyProtector; use Wikimedia\Rdbms\ConfiguredReadOnlyMode; @@ -1653,7 +1652,7 @@ return [ ); }, - 'ParsoidSiteConfig' => static function ( MediaWikiServices $services ): SiteConfig { + 'ParsoidSiteConfig' => static function ( MediaWikiServices $services ): MWSiteConfig { $mainConfig = $services->getMainConfig(); $parsoidSettings = $mainConfig->get( MainConfigNames::ParsoidSettings ); return new MWSiteConfig( @@ -1671,6 +1670,7 @@ return [ $services->getLanguageConverterFactory(), $services->getLanguageNameUtils(), $services->getUrlUtils(), + $services->getContentHandlerFactory(), ExtensionRegistry::getInstance()->getAttribute( 'ParsoidModules' ), // These arguments are temporary and will be removed once // better solutions are found. diff --git a/includes/config-schema.php b/includes/config-schema.php index 3e989472f4f..5b9cd43468b 100644 --- a/includes/config-schema.php +++ b/includes/config-schema.php @@ -2144,6 +2144,7 @@ return [ 'ParsoidOutputAccess', 'PageStore', 'RevisionLookup', + 'ParsoidSiteConfig', ], 'needsPage' => false, ], diff --git a/includes/jobqueue/jobs/ParsoidCachePrewarmJob.php b/includes/jobqueue/jobs/ParsoidCachePrewarmJob.php index 16a75229f3d..fa0d7eb5b29 100644 --- a/includes/jobqueue/jobs/ParsoidCachePrewarmJob.php +++ b/includes/jobqueue/jobs/ParsoidCachePrewarmJob.php @@ -21,6 +21,7 @@ use MediaWiki\Logger\LoggerFactory; use MediaWiki\Page\PageLookup; use MediaWiki\Page\PageRecord; +use MediaWiki\Parser\Parsoid\Config\SiteConfig as ParsoidSiteConfig; use MediaWiki\Parser\Parsoid\ParsoidOutputAccess; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\SlotRecord; @@ -36,18 +37,21 @@ class ParsoidCachePrewarmJob extends Job { private ParsoidOutputAccess $parsoidOutputAccess; private PageLookup $pageLookup; private RevisionLookup $revisionLookup; + private ParsoidSiteConfig $parsoidSiteConfig; /** * @param array $params * @param ParsoidOutputAccess $parsoidOutputAccess * @param PageLookup $pageLookup * @param RevisionLookup $revisionLookup + * @param ParsoidSiteConfig $parsoidSiteConfig */ public function __construct( array $params, ParsoidOutputAccess $parsoidOutputAccess, PageLookup $pageLookup, - RevisionLookup $revisionLookup + RevisionLookup $revisionLookup, + ParsoidSiteConfig $parsoidSiteConfig ) { parent::__construct( 'parsoidCachePrewarm', $params ); @@ -56,6 +60,7 @@ class ParsoidCachePrewarmJob extends Job { $this->parsoidOutputAccess = $parsoidOutputAccess; $this->pageLookup = $pageLookup; $this->revisionLookup = $revisionLookup; + $this->parsoidSiteConfig = $parsoidSiteConfig; } /** @@ -124,7 +129,7 @@ class ParsoidCachePrewarmJob extends Job { $parserOpts->setRenderReason( $renderReason ); $mainSlot = $rev->getSlot( SlotRecord::MAIN ); - if ( !$this->parsoidOutputAccess->supportsContentModel( $mainSlot->getModel() ) ) { + if ( !$this->parsoidSiteConfig->supportsContentModel( $mainSlot->getModel() ) ) { $this->logger->debug( __METHOD__ . ': Parsoid does not support content model ' . $mainSlot->getModel() ); return; } diff --git a/includes/parser/Parsoid/Config/SiteConfig.php b/includes/parser/Parsoid/Config/SiteConfig.php index 6b5ca57e6d7..bdca97446f3 100644 --- a/includes/parser/Parsoid/Config/SiteConfig.php +++ b/includes/parser/Parsoid/Config/SiteConfig.php @@ -28,6 +28,7 @@ use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\Config\Config; use MediaWiki\Config\MutableConfig; use MediaWiki\Config\ServiceOptions; +use MediaWiki\Content\IContentHandlerFactory; use MediaWiki\Interwiki\InterwikiLookup; use MediaWiki\Languages\LanguageConverterFactory; use MediaWiki\Languages\LanguageFactory; @@ -43,6 +44,7 @@ use MediaWiki\Title\Title; use MediaWiki\User\Options\UserOptionsLookup; use MediaWiki\Utils\UrlUtils; use MediaWiki\WikiMap\WikiMap; +use MWUnknownContentModelException; use ParserFactory; use PrefixingStatsdDataFactoryProxy; use Psr\Log\LoggerInterface; @@ -113,6 +115,7 @@ class SiteConfig extends ISiteConfig { private LanguageConverterFactory $languageConverterFactory; private LanguageNameUtils $languageNameUtils; private UrlUtils $urlUtils; + private IContentHandlerFactory $contentHandlerFactory; private ?string $baseUri = null; private ?string $relativeLinkPrefix = null; private ?array $interwikiMap = null; @@ -135,6 +138,7 @@ class SiteConfig extends ISiteConfig { * @param LanguageConverterFactory $languageConverterFactory * @param LanguageNameUtils $languageNameUtils * @param UrlUtils $urlUtils + * @param IContentHandlerFactory $contentHandlerFactory * @param array $extensionParsoidModules * @param ParserFactory $parserFactory * @param Config $mwConfig @@ -155,6 +159,7 @@ class SiteConfig extends ISiteConfig { LanguageConverterFactory $languageConverterFactory, LanguageNameUtils $languageNameUtils, UrlUtils $urlUtils, + IContentHandlerFactory $contentHandlerFactory, array $extensionParsoidModules, // $parserFactory is temporary and may be removed once a better solution is found. ParserFactory $parserFactory, // T268776 @@ -181,6 +186,7 @@ class SiteConfig extends ISiteConfig { $this->languageConverterFactory = $languageConverterFactory; $this->languageNameUtils = $languageNameUtils; $this->urlUtils = $urlUtils; + $this->contentHandlerFactory = $contentHandlerFactory; // Override parent default if ( isset( $this->parsoidSettings['linting'] ) ) { @@ -763,4 +769,33 @@ class SiteConfig extends ISiteConfig { public function getExternalLinkTarget() { return $this->config->get( MainConfigNames::ExternalLinkTarget ); } + + // MW-specific helper + + /** + * Returns true iff Parsoid natively supports the given content model. + * @param string $model content model identifier + * @return bool + */ + public function supportsContentModel( string $model ): bool { + if ( $model === CONTENT_MODEL_WIKITEXT ) { + return true; + } + + // Check if the content model serializes to wikitext. + // NOTE: We could use isSupportedFormat( CONTENT_FORMAT_WIKITEXT ) if PageContent::getContent() + // would specify the format when calling serialize(). + try { + $handler = $this->contentHandlerFactory->getContentHandler( $model ); + if ( $handler->getDefaultFormat() === CONTENT_FORMAT_WIKITEXT ) { + return true; + } + } catch ( MWUnknownContentModelException $ex ) { + // If the content model is not known, it can't be supported. + return false; + } + + return $this->getContentModelHandler( $model ) !== null; + } + } diff --git a/includes/parser/Parsoid/ParsoidOutputAccess.php b/includes/parser/Parsoid/ParsoidOutputAccess.php index e8af3b90236..d1165fa7b87 100644 --- a/includes/parser/Parsoid/ParsoidOutputAccess.php +++ b/includes/parser/Parsoid/ParsoidOutputAccess.php @@ -25,14 +25,13 @@ use MediaWiki\Page\PageLookup; use MediaWiki\Page\PageRecord; use MediaWiki\Page\ParserOutputAccess; use MediaWiki\Parser\ParserOutput; +use MediaWiki\Parser\Parsoid\Config\SiteConfig; use MediaWiki\Revision\RevisionAccessException; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Revision\SlotRecord; use MediaWiki\Status\Status; -use MWUnknownContentModelException; use ParserOptions; -use Wikimedia\Parsoid\Config\SiteConfig; use Wikimedia\Parsoid\Core\ClientError; use Wikimedia\Parsoid\Core\ResourceLimitExceededException; @@ -81,32 +80,6 @@ class ParsoidOutputAccess { $this->contentHandlerFactory = $contentHandlerFactory; } - /** - * @param string $model - * - * @return bool - */ - public function supportsContentModel( string $model ): bool { - if ( $model === CONTENT_MODEL_WIKITEXT ) { - return true; - } - - // Check if the content model serializes to wikitext. - // NOTE: We could use isSupportedFormat( CONTENT_FORMAT_WIKITEXT ) if PageContent::getContent() - // would specify the format when calling serialize(). - try { - $handler = $this->contentHandlerFactory->getContentHandler( $model ); - if ( $handler->getDefaultFormat() === CONTENT_FORMAT_WIKITEXT ) { - return true; - } - } catch ( MWUnknownContentModelException $ex ) { - // If the content model is not known, it can't be supported. - return false; - } - - return $this->siteConfig->getContentModelHandler( $model ) !== null; - } - /** * @param PageIdentity $page * @param ParserOptions $parserOpts @@ -272,7 +245,7 @@ class ParsoidOutputAccess { private function adjustParserOptions( RevisionRecord $revision, ParserOptions $parserOpts ): void { $mainSlot = $revision->getSlot( SlotRecord::MAIN ); $contentModel = $mainSlot->getModel(); - if ( $this->supportsContentModel( $contentModel ) ) { + if ( $this->siteConfig->supportsContentModel( $contentModel ) ) { // Since we know Parsoid supports this content model, explicitly // call ParserOptions::setUseParsoid. This ensures that when // we query the parser-cache, the right cache key is called. diff --git a/maintenance/prewarmParsoidParserCache.php b/maintenance/prewarmParsoidParserCache.php index 1deae3d0cc8..dc4d08d2b85 100644 --- a/maintenance/prewarmParsoidParserCache.php +++ b/maintenance/prewarmParsoidParserCache.php @@ -2,6 +2,7 @@ use MediaWiki\Page\PageIdentity; use MediaWiki\Page\PageLookup; use MediaWiki\Page\ParserOutputAccess; +use MediaWiki\Parser\Parsoid\Config\SiteConfig as ParsoidSiteConfig; use MediaWiki\Parser\Parsoid\ParsoidOutputAccess; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionRecord; @@ -23,6 +24,7 @@ class PrewarmParsoidParserCache extends Maintenance { private ParsoidOutputAccess $parsoidOutputAccess; private PageLookup $pageLookup; private RevisionLookup $revisionLookup; + private ParsoidSiteConfig $parsoidSiteConfig; public function __construct() { parent::__construct(); @@ -57,6 +59,11 @@ class PrewarmParsoidParserCache extends Maintenance { return $this->parsoidOutputAccess; } + private function getParsoidSiteConfig(): ParsoidSiteConfig { + $this->parsoidSiteConfig = $this->getServiceContainer()->getParsoidSiteConfig(); + return $this->parsoidSiteConfig; + } + private function getQueryBuilder(): SelectQueryBuilder { $dbr = $this->getReplicaDB(); @@ -149,7 +156,7 @@ class PrewarmParsoidParserCache extends Maintenance { $mainSlot = $revision->getSlot( SlotRecord::MAIN ); // POA will write a dummy output to PC, but we don't want that here. Just skip! - if ( !$this->getParsoidOutputAccess()->supportsContentModel( $mainSlot->getModel() ) ) { + if ( !$this->getParsoidSiteConfig()->supportsContentModel( $mainSlot->getModel() ) ) { $this->output( '[Skipped] Content model "' . $mainSlot->getModel() . diff --git a/tests/phpunit/includes/jobqueue/JobFactoryTest.php b/tests/phpunit/includes/jobqueue/JobFactoryTest.php index 56ef965e732..41fb21f3a16 100644 --- a/tests/phpunit/includes/jobqueue/JobFactoryTest.php +++ b/tests/phpunit/includes/jobqueue/JobFactoryTest.php @@ -50,7 +50,8 @@ class JobFactoryTest extends MediaWikiIntegrationTestCase { 'services' => [ 'ParsoidOutputAccess', 'PageStore', - 'RevisionLookup' + 'RevisionLookup', + 'ParsoidSiteConfig', ], 'needsPage' => false ], diff --git a/tests/phpunit/includes/jobqueue/JobTest.php b/tests/phpunit/includes/jobqueue/JobTest.php index 2607a30410b..57b58798a83 100644 --- a/tests/phpunit/includes/jobqueue/JobTest.php +++ b/tests/phpunit/includes/jobqueue/JobTest.php @@ -132,7 +132,8 @@ class JobTest extends MediaWikiIntegrationTestCase { 'services' => [ 'ParsoidOutputAccess', 'PageStore', - 'RevisionLookup' + 'RevisionLookup', + 'ParsoidSiteConfig', ], 'needsPage' => false ], diff --git a/tests/phpunit/includes/jobqueue/jobs/ParsoidCachePrewarmJobTest.php b/tests/phpunit/includes/jobqueue/jobs/ParsoidCachePrewarmJobTest.php index 63f936cac55..891bf3d9c3a 100644 --- a/tests/phpunit/includes/jobqueue/jobs/ParsoidCachePrewarmJobTest.php +++ b/tests/phpunit/includes/jobqueue/jobs/ParsoidCachePrewarmJobTest.php @@ -35,7 +35,8 @@ class ParsoidCachePrewarmJobTest extends MediaWikiIntegrationTestCase { [ 'revId' => $rev1->getId(), 'pageId' => $page->getId() ], $this->getServiceContainer()->getParsoidOutputAccess(), $this->getServiceContainer()->getPageStore(), - $this->getServiceContainer()->getRevisionLookup() + $this->getServiceContainer()->getRevisionLookup(), + $this->getServiceContainer()->getParsoidSiteConfig() ); // NOTE: calling ->run() will not run the job scheduled in the queue but will @@ -65,7 +66,8 @@ class ParsoidCachePrewarmJobTest extends MediaWikiIntegrationTestCase { [ 'revId' => $rev2->getId(), 'pageId' => $page->getId(), 'causeAction' => 'just for testing' ], $this->getServiceContainer()->getParsoidOutputAccess(), $this->getServiceContainer()->getPageStore(), - $this->getServiceContainer()->getRevisionLookup() + $this->getServiceContainer()->getRevisionLookup(), + $this->getServiceContainer()->getParsoidSiteConfig() ); $jobQueueGroup = $this->getServiceContainer()->getJobQueueGroup(); diff --git a/tests/phpunit/includes/page/ArticleTest.php b/tests/phpunit/includes/page/ArticleTest.php index e1e47f25118..d978b87c39b 100644 --- a/tests/phpunit/includes/page/ArticleTest.php +++ b/tests/phpunit/includes/page/ArticleTest.php @@ -205,10 +205,8 @@ class ArticleTest extends \MediaWikiIntegrationTestCase { $parsoidOutputAccess = $this->createNoOpMock( ParsoidOutputAccess::class, - [ 'getParserOutput', 'supportsContentModel' ] + [ 'getParserOutput' ] ); - $parsoidOutputAccess->method( 'supportsContentModel' ) - ->willReturn( true ); $parsoidOutputAccess ->expects( $this->once() ) // This is the key assertion in this test case. ->method( 'getParserOutput' ) diff --git a/tests/phpunit/integration/includes/Rest/Handler/ParsoidOutputAccessTest.php b/tests/phpunit/integration/includes/Rest/Handler/ParsoidOutputAccessTest.php index b864056e5f8..dcbf7e621ab 100644 --- a/tests/phpunit/integration/includes/Rest/Handler/ParsoidOutputAccessTest.php +++ b/tests/phpunit/integration/includes/Rest/Handler/ParsoidOutputAccessTest.php @@ -1,7 +1,6 @@ assertNotNull( ParsoidRenderID::newFromParserOutput( $output1 ) ); } - public static function provideSupportsContentModels() { - yield [ CONTENT_MODEL_WIKITEXT, true ]; - yield [ CONTENT_MODEL_JSON, true ]; - yield [ CONTENT_MODEL_JAVASCRIPT, false ]; - yield [ 'with-text', true ]; - yield [ 'xyzzy', false ]; - } - - /** - * @dataProvider provideSupportsContentModels - */ - public function testSupportsContentModel( $model, $expected ) { - $contentHandlers = $this->getConfVar( 'ContentHandlers' ); - $this->overrideConfigValue( 'ContentHandlers', [ - 'with-text' => [ 'factory' => static function () { - return new TextContentHandler( 'with-text', [ CONTENT_FORMAT_WIKITEXT, 'plain/test' ] ); - } ], - ] + $contentHandlers ); - - $this->resetServicesWithMockedParsoid( 0 ); - $access = $this->getParsoidOutputAccessWithCache(); - $this->assertSame( $expected, $access->supportsContentModel( $model ) ); - } - /** * @covers \MediaWiki\Parser\Parsoid\ParsoidOutputAccess::getParserOutput */ diff --git a/tests/phpunit/integration/includes/parser/Parsoid/Config/SiteConfigTest.php b/tests/phpunit/integration/includes/parser/Parsoid/Config/SiteConfigTest.php new file mode 100644 index 00000000000..6a4b35a26df --- /dev/null +++ b/tests/phpunit/integration/includes/parser/Parsoid/Config/SiteConfigTest.php @@ -0,0 +1,36 @@ +getConfVar( 'ContentHandlers' ); + $this->overrideConfigValue( 'ContentHandlers', [ + 'with-text' => [ 'factory' => static function () { + return new TextContentHandler( 'with-text', [ CONTENT_FORMAT_WIKITEXT, 'plain/test' ] ); + } ], + ] + $contentHandlers ); + + $this->resetServices(); + $siteConfig = $this->getServiceContainer()->getParsoidSiteConfig(); + $this->assertSame( $expected, $siteConfig->supportsContentModel( $model ) ); + } +} diff --git a/tests/phpunit/unit/includes/parser/Parsoid/Config/SiteConfigTest.php b/tests/phpunit/unit/includes/parser/Parsoid/Config/SiteConfigTest.php index 646c7804f9d..f14e1d9749f 100644 --- a/tests/phpunit/unit/includes/parser/Parsoid/Config/SiteConfigTest.php +++ b/tests/phpunit/unit/includes/parser/Parsoid/Config/SiteConfigTest.php @@ -6,6 +6,7 @@ use ILanguageConverter; use Language; use MediaWiki\Config\HashConfig; use MediaWiki\Config\ServiceOptions; +use MediaWiki\Content\IContentHandlerFactory; use MediaWiki\Interwiki\InterwikiLookup; use MediaWiki\Languages\LanguageConverterFactory; use MediaWiki\Languages\LanguageFactory; @@ -107,6 +108,7 @@ class SiteConfigTest extends MediaWikiUnitTestCase { $this->createMockOrOverride( LanguageConverterFactory::class, $serviceOverrides ), $this->createMockOrOverride( LanguageNameUtils::class, $serviceOverrides ), $this->createMockOrOverride( UrlUtils::class, $serviceOverrides ), + $this->createMockOrOverride( IContentHandlerFactory::class, $serviceOverrides ), [], $this->createMockOrOverride( ParserFactory::class, $serviceOverrides ), new HashConfig( $configOverrides ),