jenkins-bot has submitted this change and it was merged. Change subject: Use new db schema ......................................................................
Use new db schema The new schema introduced by this commit is a slightly altered version of that discussed in: https://phabricator.wikimedia.org/T145412#2653021 Changes: - wiki id is not recorded, instead a language is simply stored. There is no central sites table, and thus a numeric id for a site is hard, unless we introduce our own sites table or some other global sites table. Without either of these we would have to store the whole id eg. enwiktionary, but I can see little need for this and just 'en' should be fine. - There is no unique index accross language, namespace and key as per the discussed issues where multiple different titles could be normalized to be have the same key. There is however a regular index accross these fields. The functionality of Cognate remains the same before and after this change so there is nothing blocking merging this right away. Future todos: - Actually handle custom namespaces, it has already been set out in the ticket and will not be hard to implement in the future. - Introduce some normalizing of titles -> keys Bug: T145412 Change-Id: I01b4612a2e6454f620e7787d29364d0556759e1c --- A db/addCognateTitles.sql D db/addInterLanguageTable.sql M src/CognateHooks.php M src/CognateStore.php M src/hooks/CognatePageHookHandler.php M tests/phpunit/CognateStoreTest.php M tests/phpunit/hooks/CognatePageHookHandlerTest.php 7 files changed, 139 insertions(+), 98 deletions(-) Approvals: Hoo man: Looks good to me, approved jenkins-bot: Verified diff --git a/db/addCognateTitles.sql b/db/addCognateTitles.sql new file mode 100644 index 0000000..e6c4ff4 --- /dev/null +++ b/db/addCognateTitles.sql @@ -0,0 +1,11 @@ +-- Add cognate_titles table + +CREATE TABLE IF NOT EXISTS /*_*/cognate_titles ( + cgti_site VARBINARY(32) NOT NULL, + cgti_namespace INT NOT NULL, + cgti_title VARBINARY(255), + cgti_key VARBINARY(255) NOT NULL, + PRIMARY KEY (cgti_site, cgti_namespace, cgti_title) + )/*$wgDBTableOptions*/; + +CREATE INDEX /*i*/cgti_keys ON /*_*/cognate_titles (cgti_site, cgti_namespace, cgti_key); \ No newline at end of file diff --git a/db/addInterLanguageTable.sql b/db/addInterLanguageTable.sql deleted file mode 100644 index 79b0fb6..0000000 --- a/db/addInterLanguageTable.sql +++ /dev/null @@ -1,9 +0,0 @@ --- Add article assessments table - -CREATE TABLE IF NOT EXISTS /*_*/inter_language_titles ( - ilt_language VARCHAR(20) NOT NULL, - ilt_title VARCHAR(255) NOT NULL - )/*$wgDBTableOptions*/; - -CREATE INDEX /*i*/ilt_language ON /*_*/inter_language_titles (ilt_language); -CREATE UNIQUE INDEX /*i*/ilt_title_language ON /*_*/inter_language_titles (ilt_title, ilt_language); \ No newline at end of file diff --git a/src/CognateHooks.php b/src/CognateHooks.php index 1bd91ce..9bc2d4b 100644 --- a/src/CognateHooks.php +++ b/src/CognateHooks.php @@ -50,11 +50,12 @@ return true; } + /** @var CognateStore $store */ $store = MediaWikiServices::getInstance()->getService( 'CognateStore' ); - $dbKey = $title->getDBkey(); - $languages = $store->getTranslationsForPage( $wgLanguageCode, $dbKey ); + $languages = $store->getLinksForPage( $wgLanguageCode, $title ); - foreach( $languages as $lang ) { + $dbKey = $title->getDBkey(); + foreach ( $languages as $lang ) { if ( !isset( $links[$lang] ) ) { $links[$lang] = $lang . ':' . $dbKey; } @@ -68,23 +69,20 @@ /** * Run database updates * - * Only runs the update when $wgCognateWiki is false (i.e. for testing and - * when updating the "main" wiktionary project. + * Only runs the update when both $wgCognateDb and $wgCognateCluster are false + * i.e. for testing. * * @param DatabaseUpdater $updater DatabaseUpdater object * @return bool */ public static function onLoadExtensionSchemaUpdates( DatabaseUpdater $updater = null ) { - global $wgCognateWiki; + global $wgCognateDb, $wgCognateCluster; - if ( $wgCognateWiki ) { - return true; + if ( $wgCognateDb === false && $wgCognateCluster === false ) { + $updater->addExtensionUpdate( + [ 'addTable', 'cognate_titles', __DIR__ . '/../db/addCognateTitles.sql', true ] + ); } - - $dbDir = __DIR__ . '/../db'; - $updater->addExtensionUpdate( - array( 'addTable', 'inter_language_titles', "$dbDir/addInterLanguageTable.sql", true ) - ); return true; } diff --git a/src/CognateStore.php b/src/CognateStore.php index 287c58c..53e16e0 100644 --- a/src/CognateStore.php +++ b/src/CognateStore.php @@ -1,79 +1,88 @@ <?php +use MediaWiki\Linker\LinkTarget; + /** * @license GNU GPL v2+ * @author Gabriel Birke < [email protected] > + * @author Addshore */ class CognateStore { /** - * @var LoadBalancer + * @var ILoadBalancer */ private $loadBalancer; - const TABLE_NAME = 'inter_language_titles'; + const TITLES_TABLE_NAME = 'cognate_titles'; /** - * @param LoadBalancer $loadBalancer + * @param ILoadBalancer $loadBalancer */ - public function __construct( LoadBalancer $loadBalancer ) { + public function __construct( ILoadBalancer $loadBalancer ) { $this->loadBalancer = $loadBalancer; } /** - * @param string $language Language code, taken from $wgLanguageCode - * @param string $title Page title + * @param string $siteLinkPrefix The prefix for generated links + * @param LinkTarget $linkTarget + * * @return bool */ - public function savePage( $language, $title ) { + public function savePage( $siteLinkPrefix, LinkTarget $linkTarget ) { $pageData = [ - 'ilt_language' => $language, - 'ilt_title' => $title + 'cgti_site' => $siteLinkPrefix, + 'cgti_title' => $linkTarget->getDBkey(), + 'cgti_namespace' => $linkTarget->getNamespace(), + 'cgti_key' => $linkTarget->getDBkey(),// TODO normalize ]; $dbw = $this->loadBalancer->getConnectionRef( DB_MASTER ); - $result = $dbw->insert( self::TABLE_NAME, $pageData, __METHOD__, [ 'IGNORE' ] ); + $result = $dbw->insert( self::TITLES_TABLE_NAME, $pageData, __METHOD__, [ 'IGNORE' ] ); return $result; } /** - * @param string $language Language code, taken from $wgLanguageCode - * @param string $title Page title + * @param string $siteLinkPrefix The prefix for generated links + * @param LinkTarget $linkTarget + * * @return bool */ - public function deletePage( $language, $title ) { + public function deletePage( $siteLinkPrefix, LinkTarget $linkTarget ) { $pageData = [ - 'ilt_language' => $language, - 'ilt_title' => $title + 'cgti_site' => $siteLinkPrefix, + 'cgti_title' => $linkTarget->getDBkey(), + 'cgti_namespace' => $linkTarget->getNamespace(), ]; - $dbw = $this->loadBalancer->getConnection( DB_MASTER ); - $result = $dbw->delete( self::TABLE_NAME, $pageData, __METHOD__ ); - $this->loadBalancer->reuseConnection( $dbw ); + $dbw = $this->loadBalancer->getConnectionRef( DB_MASTER ); + $result = $dbw->delete( self::TITLES_TABLE_NAME, $pageData, __METHOD__ ); - return $result; + return (bool)$result; } /** - * Get the language codes where a translations is available - * - * @param string $language Language code to exclude - * @param string $title Page title - * @return array language codes + * @param string $siteLinkPrefix The prefix for generated links + * @param LinkTarget $linkTarget + * @return string[] language codes */ - public function getTranslationsForPage( $language, $title ) { - $languages = []; - + public function getLinksForPage( $siteLinkPrefix, LinkTarget $linkTarget ) { $dbr = $this->loadBalancer->getConnectionRef( DB_SLAVE ); - $result = $dbr->select( self::TABLE_NAME, ['ilt_language'], [ - 'ilt_language != ' . $dbr->addQuotes( $language ), - 'ilt_title' => $title - ] ); + $result = $dbr->select( + self::TITLES_TABLE_NAME, + [ 'cgti_site' ], + [ + 'cgti_site != ' . $dbr->addQuotes( $siteLinkPrefix ), + 'cgti_key' => $linkTarget->getDBkey(),// TODO normalize + 'cgti_namespace' => $linkTarget->getNamespace(), + ] + ); + $siteLinkPrefixes = []; while ( $row = $result->fetchRow() ) { - $languages[] = $row[ 'ilt_language' ]; + $siteLinkPrefixes[] = $row[ 'cgti_site' ]; } - return $languages; + return $siteLinkPrefixes; } } diff --git a/src/hooks/CognatePageHookHandler.php b/src/hooks/CognatePageHookHandler.php index 1fb288f..ae09a79 100644 --- a/src/hooks/CognatePageHookHandler.php +++ b/src/hooks/CognatePageHookHandler.php @@ -1,5 +1,6 @@ <?php +use MediaWiki\Linker\LinkTarget; use MediaWiki\MediaWikiServices; use Wikimedia\Assert\Assert; @@ -34,7 +35,7 @@ * Occurs after the save page request has been processed. * @see https://www.mediawiki.org/wiki/Manual:Hooks/PageContentSaveComplete * - * @param WikiPage $article + * @param WikiPage $page * @param User $user * @param Content $content * @param string $summary @@ -47,7 +48,7 @@ * @param integer $baseRevId */ public function onPageContentSaveComplete( - WikiPage $article, + WikiPage $page, User $user, Content $content, $summary, @@ -59,9 +60,9 @@ Status $status, $baseRevId ) { - if ( $article->getTitle()->inNamespaces( $this->namespaces ) ) { - $store = MediaWikiServices::getInstance()->getService( 'CognateStore' ); - $store->savePage( $this->languageCode, $article->getTitle()->getDBkey() ); + $titleValue = $page->getTitle()->getTitleValue(); + if ( $this->isActionableTarget( $titleValue ) ) { + $this->getStore()->savePage( $this->languageCode, $titleValue ); } } @@ -78,17 +79,26 @@ Content $content = null, array &$updates ) { - $title = $page->getTitle(); + $titleValue = $page->getTitle()->getTitleValue(); $language = $this->languageCode; - if ( $title->inNamespaces( $this->namespaces ) ) { - $updates[] = new MWCallableUpdate( - function () use ( $title, $language ){ - $store = MediaWikiServices::getInstance()->getService( 'CognateStore' ); - $store->deletePage( $language, $title->getDBkey() ); - }, - __METHOD__ - ); + if ( $this->isActionableTarget( $titleValue ) ) { + $updates[] = $this->newDeferrableDelete( $titleValue, $language ); } + } + + /** + * @param TitleValue $titleValue + * @param string $language + * + * @return MWCallableUpdate + */ + private function newDeferrableDelete( TitleValue $titleValue, $language ) { + return new MWCallableUpdate( + function () use ( $language, $titleValue ){ + $this->getStore()->deletePage( $language, $titleValue ); + }, + __METHOD__ + ); } /** @@ -106,10 +116,30 @@ $comment, $oldPageId ) { - if ( $title->inNamespaces( $this->namespaces ) ) { - $store = MediaWikiServices::getInstance()->getService( 'CognateStore' ); - $store->savePage( $this->languageCode, $title->getDBkey() ); + $titleValue = $title->getTitleValue(); + if ( $this->isActionableTarget( $titleValue ) ) { + $this->getStore()->savePage( $this->languageCode, $titleValue ); } } + /** + * Actionable targets have a namespace id that is: + * - One of the default MediaWiki (between NS_MAIN and NS_CATEGORY_TALK + * - Defined as a namespace to record in the configuration + * @param LinkTarget $linkTarget + * @return bool + */ + private function isActionableTarget( LinkTarget $linkTarget ) { + $namespace = $linkTarget->getNamespace(); + return in_array( $namespace, $this->namespaces ) && + $namespace >= NS_MAIN && $namespace <= NS_CATEGORY_TALK; + } + + /** + * @return CognateStore + */ + private function getStore() { + return MediaWikiServices::getInstance()->getService( 'CognateStore' ); + } + } \ No newline at end of file diff --git a/tests/phpunit/CognateStoreTest.php b/tests/phpunit/CognateStoreTest.php index bf8ec45..f585ebd 100644 --- a/tests/phpunit/CognateStoreTest.php +++ b/tests/phpunit/CognateStoreTest.php @@ -1,5 +1,7 @@ <?php +use MediaWiki\MediaWikiServices; + /** * @license GNU GPL v2+ * @author Gabriel Birke < [email protected] > @@ -12,47 +14,47 @@ protected function setUp() { parent::setUp(); - $this->tablesUsed = [ 'inter_language_titles' ]; - $this->store = new CognateStore( wfGetLB(), false ); + $this->tablesUsed = [ CognateStore::TITLES_TABLE_NAME ]; + $this->store = MediaWikiServices::getInstance()->getService( 'CognateStore' ); } public function testSavePageCreatesNewEntry() { - $this->store->savePage( 'en', 'My_test_page' ); + $this->store->savePage( 'en', new TitleValue( 0, 'My_test_page' ) ); $this->assertSelect( - 'inter_language_titles', - [ 'ilt_language', 'ilt_title' ], - [ 'ilt_title != "UTPage"' ], - [ [ 'en', 'My_test_page' ] ] + 'cognate_titles', + [ 'cgti_site', 'cgti_title', 'cgti_key', 'cgti_namespace' ], + [ 'cgti_title != "UTPage"' ], + [ [ 'en', 'My_test_page', 'My_test_page', 0 ] ] ); } public function testSavePageWithExistingEntryIgnoresErrors() { - $this->store->savePage( 'en', 'My_second_test_page' ); - $this->store->savePage( 'en', 'My_second_test_page' ); + $this->store->savePage( 'en', new TitleValue( 0, 'My_second_test_page' ) ); + $this->store->savePage( 'en', new TitleValue( 0, 'My_second_test_page' ) ); $this->assertSelect( - 'inter_language_titles', - [ 'ilt_language', 'ilt_title' ], - [ 'ilt_title != "UTPage"' ], - [ [ 'en', 'My_second_test_page' ] ] + 'cognate_titles', + [ 'cgti_site', 'cgti_title', 'cgti_key', 'cgti_namespace' ], + [ 'cgti_title != "UTPage"' ], + [ [ 'en', 'My_second_test_page', 'My_second_test_page', 0 ] ] ); } public function testGetTranslationsForPageReturnsAllLanguages() { - $this->store->savePage( 'en', 'My_test_page' ); - $this->store->savePage( 'en', 'Another_unrelated_page' ); - $this->store->savePage( 'de', 'My_test_page' ); - $this->store->savePage( 'eo', 'My_test_page' ); - $languages = $this->store->getTranslationsForPage( 'en', 'My_test_page' ); + $this->store->savePage( 'en', new TitleValue( 0, 'My_test_page' ) ); + $this->store->savePage( 'en', new TitleValue( 0, 'Another_unrelated_page' ) ); + $this->store->savePage( 'de', new TitleValue( 0, 'My_test_page' ) ); + $this->store->savePage( 'eo', new TitleValue( 0, 'My_test_page' ) ); + $languages = $this->store->getLinksForPage( 'en', new TitleValue( 0, 'My_test_page' ) ); $this->assertArrayEquals( [ 'de', 'eo' ], $languages ); } public function testSaveAndDeletePageResultsInNoEntry() { - $this->store->savePage( 'en', 'My_test_page' ); - $this->store->deletePage( 'en', 'My_test_page' ); + $this->store->savePage( 'en', new TitleValue( 0, 'My_test_page' ) ); + $this->store->deletePage( 'en', new TitleValue( 0, 'My_test_page' ) ); $this->assertSelect( - 'inter_language_titles', - [ 'ilt_language', 'ilt_title' ], - [ 'ilt_title != "UTPage"' ], + 'cognate_titles', + [ 'cgti_site', 'cgti_title', 'cgti_key', 'cgti_namespace' ], + [ 'cgti_title != "UTPage"' ], [] ); } diff --git a/tests/phpunit/hooks/CognatePageHookHandlerTest.php b/tests/phpunit/hooks/CognatePageHookHandlerTest.php index 3834b01..3e40c4d 100644 --- a/tests/phpunit/hooks/CognatePageHookHandlerTest.php +++ b/tests/phpunit/hooks/CognatePageHookHandlerTest.php @@ -34,7 +34,7 @@ ->method( 'deletePage' ); $this->store->expects( $this->once() ) ->method( 'savePage' ) - ->with( 'abc2', 'ArticleDbKey' ); + ->with( 'abc2', new TitleValue( 0, 'ArticleDbKey' ) ); $this->call_onPageContentSaveComplete( [ 0 ], 'abc2', new TitleValue( 0, 'ArticleDbKey' ) ); } @@ -80,7 +80,7 @@ public function test_onWikiPageDeletionUpdates_namespaceMatch() { $this->store->expects( $this->once() ) ->method( 'deletePage' ) - ->with( 'abc2', 'ArticleDbKey' ); + ->with( 'abc2', new TitleValue( 0, 'ArticleDbKey' ) ); $this->store->expects( $this->never() ) ->method( 'savePage' ); @@ -146,7 +146,7 @@ ->method( 'deletePage' ); $this->store->expects( $this->once() ) ->method( 'savePage' ) - ->with( 'abc2', 'ArticleDbKey' ); + ->with( 'abc2', new TitleValue( 0, 'ArticleDbKey' ) ); $this->call_onArticleUndelete( [ 0 ], -- To view, visit https://gerrit.wikimedia.org/r/312257 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I01b4612a2e6454f620e7787d29364d0556759e1c Gerrit-PatchSet: 6 Gerrit-Project: mediawiki/extensions/Cognate Gerrit-Branch: master Gerrit-Owner: Addshore <[email protected]> Gerrit-Reviewer: Addshore <[email protected]> Gerrit-Reviewer: Daniel Kinzler <[email protected]> Gerrit-Reviewer: Gabriel Birke <[email protected]> Gerrit-Reviewer: Hoo man <[email protected]> Gerrit-Reviewer: Jcrespo <[email protected]> Gerrit-Reviewer: Legoktm <[email protected]> Gerrit-Reviewer: Springle <[email protected]> Gerrit-Reviewer: WMDE-Fisch <[email protected]> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
