From 417d8036ae9ae8bbe9932644c7c81510a4e680fd Mon Sep 17 00:00:00 2001 From: Amir Sarabadani Date: Thu, 24 May 2018 00:19:03 +0200 Subject: [PATCH] Add code to write to change_tag_def table Bug: T193868 Change-Id: Ia61b9878595dedd2725148e1078063bf1f127d2a --- includes/DefaultSettings.php | 14 ++ includes/changetags/ChangeTags.php | 108 ++++++++- .../includes/changetags/ChangeTagsTest.php | 226 ++++++++++++++++++ 3 files changed, 339 insertions(+), 9 deletions(-) diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 87ca0168bd..78d336a607 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -8910,6 +8910,20 @@ $wgActorTableSchemaMigrationStage = MIGRATION_OLD; */ $wgExpiryWidgetNoDatePicker = false; +/** + * change_tag table schema migration stage. + * + * - MIGRATION_OLD: Do not use change_tag_def table or ct_tag_id. + * - MIGRATION_WRITE_BOTH: Write to the change_tag_def table and ct_tag_id, but read from + * the old schema. This is different from the formal definition of the constants + * - MIGRATION_WRITE_NEW: Behaves the same as MIGRATION_WRITE_BOTH + * - MIGRATION_NEW: Use the change_tag_def table and ct_tag_id, do not read/write ct_tag + * + * @since 1.32 + * @var int One of the MIGRATION_* constants + */ +$wgChangeTagsSchemaMigrationStage = MIGRATION_OLD; + /** * For really cool vim folding this needs to be at the end: * vim: foldmarker=@{,@} foldmethod=marker diff --git a/includes/changetags/ChangeTags.php b/includes/changetags/ChangeTags.php index b64f85a9b4..8f8426fc4b 100644 --- a/includes/changetags/ChangeTags.php +++ b/includes/changetags/ChangeTags.php @@ -261,6 +261,8 @@ class ChangeTags { &$rev_id = null, &$log_id = null, $params = null, RecentChange $rc = null, User $user = null ) { + global $wgChangeTagsSchemaMigrationStage; + $tagsToAdd = array_filter( (array)$tagsToAdd ); // Make sure we're submitting all tags... $tagsToRemove = array_filter( (array)$tagsToRemove ); @@ -342,6 +344,35 @@ class ChangeTags { // insert a row into change_tag for each new tag if ( count( $tagsToAdd ) ) { + $changeTagMapping = []; + if ( $wgChangeTagsSchemaMigrationStage > MIGRATION_OLD ) { + $tagDefRows = []; + foreach ( $tagsToAdd as $tag ) { + $tagDefRows[] = [ + 'ctd_name' => $tag, + 'ctd_user_defined' => 0, + 'ctd_count' => 1 + ]; + } + + $dbw->upsert( + 'change_tag_def', + $tagDefRows, + [ 'ctd_name' ], + [ 'ctd_count = ctd_count + 1' ], + __METHOD__ + ); + + $res = $dbw->select( + 'change_tag_def', + [ 'ctd_name', 'ctd_id' ], + [ 'ctd_name' => $tagsToAdd ] + ); + foreach ( $res as $row ) { + $changeTagMapping[$row->ctd_name] = $row->ctd_id; + } + } + $tagsRows = []; foreach ( $tagsToAdd as $tag ) { // Filter so we don't insert NULLs as zero accidentally. @@ -354,9 +385,11 @@ class ChangeTags { 'ct_rc_id' => $rc_id, 'ct_log_id' => $log_id, 'ct_rev_id' => $rev_id, - 'ct_params' => $params + 'ct_params' => $params, + 'ct_tag_id' => isset( $changeTagMapping[$tag] ) ? $changeTagMapping[$tag] : null, ] ); + } $dbw->insert( 'change_tag', $tagsRows, __METHOD__, [ 'IGNORE' ] ); @@ -374,6 +407,20 @@ class ChangeTags { ] ); $dbw->delete( 'change_tag', $conds, __METHOD__ ); + if ( $dbw->affectedRows() && $wgChangeTagsSchemaMigrationStage > MIGRATION_OLD ) { + $dbw->update( + 'change_tag_def', + [ 'ctd_count = ctd_count - 1' ], + [ 'ctd_name' => $tag ], + __METHOD__ + ); + + $dbw->delete( + 'change_tag_def', + [ 'ctd_name' => $tag, 'ctd_count' => 0, 'ctd_user_defined' => 0 ], + __METHOD__ + ); + } } } @@ -828,8 +875,8 @@ class ChangeTags { } /** - * Defines a tag in the valid_tag table, without checking that the tag name - * is valid. + * Defines a tag in the valid_tag table and/or update ctd_user_defined field in change_tag_def, + * without checking that the tag name is valid. * Extensions should NOT use this function; they can use the ListDefinedTags * hook instead. * @@ -837,26 +884,63 @@ class ChangeTags { * @since 1.25 */ public static function defineTag( $tag ) { + global $wgChangeTagsSchemaMigrationStage; + $dbw = wfGetDB( DB_MASTER ); - $dbw->replace( 'valid_tag', + if ( $wgChangeTagsSchemaMigrationStage > MIGRATION_OLD ) { + $tagDef = [ + 'ctd_name' => $tag, + 'ctd_user_defined' => 1, + 'ctd_count' => 0 + ]; + $dbw->upsert( + 'change_tag_def', + $tagDef, + [ 'ctd_name' ], + [ 'ctd_user_defined' => 1 ], + __METHOD__ + ); + } + + $dbw->replace( + 'valid_tag', [ 'vt_tag' ], [ 'vt_tag' => $tag ], - __METHOD__ ); + __METHOD__ + ); // clear the memcache of defined tags self::purgeTagCacheAll(); } /** - * Removes a tag from the valid_tag table. The tag may remain in use by - * extensions, and may still show up as 'defined' if an extension is setting - * it from the ListDefinedTags hook. + * Removes a tag from the valid_tag table and/or update ctd_user_defined field in change_tag_def. + * The tag may remain in use by extensions, and may still show up as 'defined' + * if an extension is setting it from the ListDefinedTags hook. * * @param string $tag Tag to remove * @since 1.25 */ public static function undefineTag( $tag ) { + global $wgChangeTagsSchemaMigrationStage; + $dbw = wfGetDB( DB_MASTER ); + + if ( $wgChangeTagsSchemaMigrationStage > MIGRATION_OLD ) { + $dbw->update( + 'change_tag_def', + [ 'ctd_name' => $tag ], + [ 'ctd_user_defined' => 0 ], + __METHOD__ + ); + + $dbw->delete( + 'change_tag_def', + [ 'ctd_name' => $tag, 'ctd_count' => 0 ], + __METHOD__ + ); + } + $dbw->delete( 'valid_tag', [ 'vt_tag' => $tag ], __METHOD__ ); // clear the memcache of defined tags @@ -1108,6 +1192,7 @@ class ChangeTags { /** * Creates a tag by adding a row to the `valid_tag` table. + * and/or add it to `change_tag_def` table. * * Extensions should NOT use this function; they can use the ListDefinedTags * hook instead. @@ -1158,10 +1243,11 @@ class ChangeTags { * @since 1.25 */ public static function deleteTagEverywhere( $tag ) { + global $wgChangeTagsSchemaMigrationStage; $dbw = wfGetDB( DB_MASTER ); $dbw->startAtomic( __METHOD__ ); - // delete from valid_tag + // delete from valid_tag and/or set ctd_user_defined = 0 self::undefineTag( $tag ); // find out which revisions use this tag, so we can delete from tag_summary @@ -1180,6 +1266,10 @@ class ChangeTags { // delete from change_tag $dbw->delete( 'change_tag', [ 'ct_tag' => $tag ], __METHOD__ ); + if ( $wgChangeTagsSchemaMigrationStage > MIGRATION_OLD ) { + $dbw->delete( 'change_tag_def', [ 'ctd_name' => $tag ], __METHOD__ ); + } + $dbw->endAtomic( __METHOD__ ); // give extensions a chance diff --git a/tests/phpunit/includes/changetags/ChangeTagsTest.php b/tests/phpunit/includes/changetags/ChangeTagsTest.php index 63e0ec2268..215cdfdf90 100644 --- a/tests/phpunit/includes/changetags/ChangeTagsTest.php +++ b/tests/phpunit/includes/changetags/ChangeTagsTest.php @@ -2,9 +2,18 @@ /** * @covers ChangeTags + * @group Database */ class ChangeTagsTest extends MediaWikiTestCase { + public function setUp() { + parent::setUp(); + + $this->tablesUsed[] = 'change_tag'; + $this->tablesUsed[] = 'change_tag_def'; + $this->tablesUsed[] = 'tag_summary'; + } + // TODO only modifyDisplayQuery and getSoftwareTags are tested, nothing else is /** @dataProvider provideModifyDisplayQuery */ @@ -306,4 +315,221 @@ class ChangeTagsTest extends MediaWikiTestCase { sort( $actual ); $this->assertEquals( $expected, $actual ); } + + public function testUpdateTagsMigrationOld() { + $this->setMwGlobals( 'wgChangeTagsSchemaMigrationStage', MIGRATION_OLD ); + $dbw = wfGetDB( DB_MASTER ); + $dbw->delete( 'change_tag', '*' ); + $dbw->delete( 'change_tag_def', '*' ); + + $rcId = 123; + ChangeTags::updateTags( [ 'tag1', 'tag2' ], [], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( [], iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => null, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag' => 'tag2', + 'ct_tag_id' => null, + 'ct_rc_id' => 123 + ], + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag', 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + + $rcId = 124; + ChangeTags::updateTags( [ 'tag1', 'tag3' ], [], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( [], iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => null, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag' => 'tag2', + 'ct_tag_id' => null, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => null, + 'ct_rc_id' => 124 + ], + (object)[ + 'ct_tag' => 'tag3', + 'ct_tag_id' => null, + 'ct_rc_id' => 124 + ], + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag', 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + } + + public function testUpdateTagsMigrationWriteBoth() { + $this->setMwGlobals( 'wgChangeTagsSchemaMigrationStage', MIGRATION_WRITE_BOTH ); + $dbw = wfGetDB( DB_MASTER ); + $dbw->delete( 'change_tag', '*' ); + $dbw->delete( 'change_tag_def', '*' ); + + $rcId = 123; + ChangeTags::updateTags( [ 'tag1', 'tag2' ], [], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $expected = [ + (object)[ + 'ctd_name' => 'tag1', + 'ctd_id' => 1, + 'ctd_count' => 1 + ], + (object)[ + 'ctd_name' => 'tag2', + 'ctd_id' => 2, + 'ctd_count' => 1 + ], + ]; + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( $expected, iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => 1, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag' => 'tag2', + 'ct_tag_id' => 2, + 'ct_rc_id' => 123 + ], + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag', 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + + $rcId = 124; + ChangeTags::updateTags( [ 'tag1', 'tag3' ], [], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $expected = [ + (object)[ + 'ctd_name' => 'tag1', + 'ctd_id' => 1, + 'ctd_count' => 2 + ], + (object)[ + 'ctd_name' => 'tag2', + 'ctd_id' => 2, + 'ctd_count' => 1 + ], + (object)[ + 'ctd_name' => 'tag3', + 'ctd_id' => 3, + 'ctd_count' => 1 + ], + ]; + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( $expected, iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => 1, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag' => 'tag2', + 'ct_tag_id' => 2, + 'ct_rc_id' => 123 + ], + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => 1, + 'ct_rc_id' => 124 + ], + (object)[ + 'ct_tag' => 'tag3', + 'ct_tag_id' => 3, + 'ct_rc_id' => 124 + ], + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag', 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + } + + public function testDeleteTagsMigrationOld() { + $this->setMwGlobals( 'wgChangeTagsSchemaMigrationStage', MIGRATION_OLD ); + $dbw = wfGetDB( DB_MASTER ); + $dbw->delete( 'change_tag', '*' ); + $dbw->delete( 'change_tag_def', '*' ); + + $rcId = 123; + ChangeTags::updateTags( [ 'tag1', 'tag2' ], [], $rcId ); + + ChangeTags::updateTags( [], [ 'tag2' ], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( [], iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => null, + 'ct_rc_id' => 123 + ] + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag', 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + } + + public function testDeleteTagsMigrationWriteBoth() { + $this->setMwGlobals( 'wgChangeTagsSchemaMigrationStage', MIGRATION_WRITE_BOTH ); + $dbw = wfGetDB( DB_MASTER ); + $dbw->delete( 'change_tag', '*' ); + $dbw->delete( 'change_tag_def', '*' ); + + $rcId = 123; + ChangeTags::updateTags( [ 'tag1', 'tag2' ], [], $rcId ); + + ChangeTags::updateTags( [], [ 'tag2' ], $rcId ); + + $dbr = wfGetDB( DB_REPLICA ); + + $expected = [ + (object)[ + 'ctd_name' => 'tag1', + 'ctd_id' => 1, + 'ctd_count' => 1 + ], + ]; + $res = $dbr->select( 'change_tag_def', [ 'ctd_name', 'ctd_id', 'ctd_count' ], '' ); + $this->assertEquals( $expected, iterator_to_array( $res, false ) ); + + $expected2 = [ + (object)[ + 'ct_tag' => 'tag1', + 'ct_tag_id' => 1, + 'ct_rc_id' => 123 + ] + ]; + $res2 = $dbr->select( 'change_tag', [ 'ct_tag', 'ct_tag_id', 'ct_rc_id' ], '' ); + $this->assertEquals( $expected2, iterator_to_array( $res2, false ) ); + } + } -- 2.20.1