Ladsgroup has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/397798 )
Change subject: Clean up all backward compatibilities for EntityCompactDiff ...................................................................... Clean up all backward compatibilities for EntityCompactDiff Bug: T182137 Change-Id: Ieee787509a33e75e2ced2d161dd750fc0bb9be57 --- M client/includes/Changes/AffectedPagesFinder.php M client/tests/phpunit/includes/Changes/AffectedPagesFinderTest.php M client/tests/phpunit/includes/Changes/InjectRCRecordsJobTest.php M client/tests/phpunit/includes/RecentChanges/RecentChangeFactoryTest.php M lib/includes/Changes/DiffChange.php M lib/includes/Changes/EntityChange.php M lib/includes/Changes/ItemChange.php M lib/tests/phpunit/Changes/EntityChangeTest.php M lib/tests/phpunit/Changes/ItemChangeTest.php M repo/tests/phpunit/includes/Store/Sql/SqlChangeStoreTest.php 10 files changed, 43 insertions(+), 265 deletions(-) Approvals: Ladsgroup: Verified Thiemo Kreuz (WMDE): Looks good to me, approved diff --git a/client/includes/Changes/AffectedPagesFinder.php b/client/includes/Changes/AffectedPagesFinder.php index a5450e2..134661c 100644 --- a/client/includes/Changes/AffectedPagesFinder.php +++ b/client/includes/Changes/AffectedPagesFinder.php @@ -16,7 +16,6 @@ use Wikibase\DataModel\Entity\EntityId; use Wikibase\EntityChange; use Wikibase\ItemChange; -use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; use Wikibase\Lib\Store\StorageException; /** @@ -121,15 +120,7 @@ public function getChangedAspects( EntityChange $change ) { $aspects = []; - $info = $change->getInfo(); - // We might unserialize old EntityChange which doesn't have getCompactDiff method - if ( array_key_exists( 'compactDiff', $info ) ) { - $diffAspects = $info['compactDiff']; - } else { - $diffAspects = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( - $change->getDiff() - ); - } + $diffAspects = $change->getCompactDiff(); if ( $diffAspects->getSiteLinkChanges() !== [] ) { $sitelinkChanges = $diffAspects->getSiteLinkChanges(); @@ -239,15 +230,7 @@ $usages = $this->transformAllPageEntityUsages( $usages, $entityId, $changedAspects ); if ( $change instanceof ItemChange && in_array( EntityUsage::TITLE_USAGE, $changedAspects ) ) { - $info = $change->getInfo(); - // We might unserialize old EntityChange which doesn't have getCompactDiff method - if ( array_key_exists( 'compactDiff', $info ) ) { - $diffChangedAspects = $info['compactDiff']; - } else { - $diffChangedAspects = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( - $change->getDiff() - ); - } + $diffChangedAspects = $change->getCompactDiff(); $namesFromDiff = $this->getPagesReferencedInDiff( $diffChangedAspects->getSiteLinkChanges() ); diff --git a/client/tests/phpunit/includes/Changes/AffectedPagesFinderTest.php b/client/tests/phpunit/includes/Changes/AffectedPagesFinderTest.php index 5ce984d..4106fc6 100644 --- a/client/tests/phpunit/includes/Changes/AffectedPagesFinderTest.php +++ b/client/tests/phpunit/includes/Changes/AffectedPagesFinderTest.php @@ -18,7 +18,6 @@ use Wikibase\DataModel\Entity\PropertyId; use Wikibase\DataModel\Snak\PropertyValueSnak; use Wikibase\EntityChange; -use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; use Wikibase\Lib\Store\SiteLinkLookup; use Wikibase\Lib\Store\StorageException; use Wikibase\Lib\Tests\Changes\TestChanges; @@ -218,32 +217,6 @@ $actual = $referencedPagesFinder->getChangedAspects( $change ); - sort( $expected ); - sort( $actual ); - $this->assertEquals( $expected, $actual ); - } - - /** - * @dataProvider getChangedAspectsProvider - */ - public function testGetChangedAspectsUsingEntityChangeAspects( - array $expected, - EntityChange $change, - $trackUsagesInAllLanguages = false - ) { - $referencedPagesFinder = $this->getAffectedPagesFinder( [], [], $trackUsagesInAllLanguages ); - $info = $change->getInfo(); - if ( !array_key_exists( 'compactDiff', $info ) ) { - $aspects = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( - $change->getDiff() - ); - $info = $change->getInfo(); - $info['compactDiff'] = $aspects; - $change->setField( 'info', $info ); - - } - - $actual = $referencedPagesFinder->getChangedAspects( $change ); sort( $expected ); sort( $actual ); $this->assertEquals( $expected, $actual ); diff --git a/client/tests/phpunit/includes/Changes/InjectRCRecordsJobTest.php b/client/tests/phpunit/includes/Changes/InjectRCRecordsJobTest.php index 46a2221..e8bdd4e 100644 --- a/client/tests/phpunit/includes/Changes/InjectRCRecordsJobTest.php +++ b/client/tests/phpunit/includes/Changes/InjectRCRecordsJobTest.php @@ -17,6 +17,7 @@ use Wikibase\EntityChange; use Wikibase\ItemChange; use Wikibase\Lib\Changes\EntityChangeFactory; +use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; use Wikibase\Lib\Store\Sql\EntityChangeLookup; use Wikimedia\Rdbms\LBFactory; use Wikimedia\TestingAccessWrapper; @@ -218,7 +219,7 @@ 'object_id' => $itemId->getSerialization(), ] ); - $diff = new ItemDiff(); + $diff = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( new ItemDiff() ); return [ 'mock change' => [ @@ -251,7 +252,7 @@ 'type' => 'wikibase-item~change', 'object_id' => $itemId->getSerialization(), 'info' => [ - 'diff' => $diff, + 'compactDiff' => $diff, 'stuff' => 'Fun Stuff', 'metadata' => [ 'foo' => 'Kitten' @@ -269,7 +270,7 @@ 'type' => 'wikibase-item~change', 'object_id' => $itemId->getSerialization(), 'info' => [ - 'diff' => $diff, + 'compactDiff' => $diff, 'change-ids' => [ 101, 102 ], 'changes' => [ $child1, $child2 ], ] diff --git a/client/tests/phpunit/includes/RecentChanges/RecentChangeFactoryTest.php b/client/tests/phpunit/includes/RecentChanges/RecentChangeFactoryTest.php index 62daca7..b60a22e 100644 --- a/client/tests/phpunit/includes/RecentChanges/RecentChangeFactoryTest.php +++ b/client/tests/phpunit/includes/RecentChanges/RecentChangeFactoryTest.php @@ -3,9 +3,12 @@ namespace Wikibase\Client\Tests\RecentChanges; use Diff\DiffOp\Diff\Diff; -use Diff\DiffOp\DiffOpChange; use Diff\MapDiffer; use Language; +use Wikibase\DataModel\Entity\Item; +use Wikibase\DataModel\Services\Diff\ItemDiffer; +use Wikibase\DataModel\SiteLink; +use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; use Wikibase\Lib\Tests\Changes\MockRepoClientCentralIdLookup; use SiteLookup; use Wikimedia\TestingAccessWrapper; @@ -68,7 +71,10 @@ // instantiate and handle the change $type = 'wikibase-' . $entityId->getEntityType() . '~' . $action; $instance->setField( 'type', $type ); - $instance->setDiff( $diff ); + $aspects = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( + $diff + ); + $instance->setCompactDiff( $aspects ); return $instance; } @@ -134,8 +140,15 @@ $change = $this->newEntityChange( 'change', new ItemId( 'Q17' ), $emptyDiff, $fields ); $change->setMetadata( $metadata ); - $diffOp = new Diff( [ 'testwiki' => new DiffOpChange( 'RecentChangeFactoryTest', 'Bar' ) ] ); - $siteLinkDiff = new ItemDiff( [ 'links' => $diffOp ] ); + $itemDiffer = new ItemDiffer(); + $emptyItem = new Item( new ItemId( 'Q17' ), null, null, null ); + $oldItem = $emptyItem->copy(); + $oldItem->addSiteLink( new SiteLink( 'testwiki', 'RecentChangeFactoryTest' ) ); + + $newItem = $emptyItem->copy(); + $newItem->addSiteLink( new SiteLink( 'testwiki', 'Bar' ) ); + + $siteLinkDiff = $itemDiffer->diffItems( $oldItem, $newItem ); $siteLinkChange = $this->newEntityChange( 'change', new ItemId( 'Q17' ), $siteLinkDiff, $fields ); $siteLinkChange->setMetadata( $metadata ); diff --git a/lib/includes/Changes/DiffChange.php b/lib/includes/Changes/DiffChange.php index f004ca4..281c93d 100644 --- a/lib/includes/Changes/DiffChange.php +++ b/lib/includes/Changes/DiffChange.php @@ -16,30 +16,6 @@ abstract class DiffChange extends ChangeRow { /** - * @param string $cache set to 'cache' to cache the unserialized diff. - * - * @return Diff - */ - public function getDiff( $cache = 'no' ) { - $info = $this->getInfo( $cache ); - - if ( !array_key_exists( 'diff', $info ) ) { - // This shouldn't happen, but we should be robust against corrupt, incomplete - // obsolete instances in the database, etc. - wfLogWarning( 'Cannot get the diff when it has not been set yet.' ); - return new Diff(); - } else { - return $info['diff']; - } - } - - public function setDiff( Diff $diff ) { - $info = $this->getInfo(); - $info['diff'] = $diff; - $this->setField( 'info', $info ); - } - - /** * @return EntityDiffChangedAspects */ public function getCompactDiff() { diff --git a/lib/includes/Changes/EntityChange.php b/lib/includes/Changes/EntityChange.php index 9c6755c..d8f398d 100644 --- a/lib/includes/Changes/EntityChange.php +++ b/lib/includes/Changes/EntityChange.php @@ -2,24 +2,15 @@ namespace Wikibase; -use Deserializers\Deserializer; use Diff\DiffOp\Diff\Diff; -use Diff\DiffOp\DiffOp; -use Diff\DiffOpFactory; use MWException; use RecentChange; use Revision; -use RuntimeException; -use Serializers\Serializer; use User; -use Wikibase\Client\WikibaseClient; use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\BasicEntityIdParser; -use Wikibase\DataModel\Services\Diff\EntityTypeAwareDiffOpFactory; -use Wikibase\DataModel\Statement\Statement; use Wikibase\Lib\Changes\EntityDiffChangedAspects; use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; -use Wikibase\Repo\WikibaseRepo; /** * Represents a change for an entity; to be extended by various change subtypes @@ -278,23 +269,6 @@ $info = array_diff_key( $info, array_flip( $skipKeys ) ); - if ( isset( $info['diff'] ) ) { - $diff = $info['diff']; - - if ( $diff instanceof DiffOp ) { - $info['diff'] = $diff->toArray( function ( $data ) { - if ( !( $data instanceof Statement ) ) { - return $data; - } - - $array = $this->getStatementSerializer()->serialize( $data ); - $array['_claimclass_'] = get_class( $data ); - - return $array; - } ); - } - } - if ( isset( $info['compactDiff'] ) ) { $diff = $info['compactDiff']; @@ -310,8 +284,8 @@ $info, function ( $v ) { if ( is_object( $v ) ) { - throw new MWException( "Refusing to serialize PHP object of type " - . get_class( $v ) ); + throw new MWException( "Refusing to serialize PHP object of type " . + get_class( $v ) ); } } ); @@ -319,38 +293,6 @@ //XXX: we could JSON_UNESCAPED_UNICODE here, perhaps. return json_encode( $info ); - } - - /** - * @throws RuntimeException - * @return Serializer - */ - private function getStatementSerializer() { - // FIXME: the change row system needs to be reworked to either allow for sane injection - // or to avoid this kind of configuration dependent tasks. - if ( WikibaseSettings::isRepoEnabled() ) { - return WikibaseRepo::getDefaultInstance()->getStatementSerializer(); - } elseif ( WikibaseSettings::isClientEnabled() ) { - throw new RuntimeException( 'Cannot serialize statements on the client' ); - } else { - throw new RuntimeException( 'Need either client or repo loaded' ); - } - } - - /** - * @throws RuntimeException - * @return Deserializer - */ - private function getStatementDeserializer() { - // FIXME: the change row system needs to be reworked to either allow for sane injection - // or to avoid this kind of configuration dependent tasks. - if ( WikibaseSettings::isRepoEnabled() ) { - return WikibaseRepo::getDefaultInstance()->getInternalFormatStatementDeserializer(); - } elseif ( WikibaseSettings::isClientEnabled() ) { - return WikibaseClient::getDefaultInstance()->getInternalFormatStatementDeserializer(); - } else { - throw new RuntimeException( 'Need either client or repo loaded' ); - } } /** @@ -366,14 +308,6 @@ $info = parent::unserializeInfo( $serialization ); - if ( isset( $info['diff'] ) && is_array( $info['diff'] ) && $info['diff'] ) { - if ( $factory === null ) { - $factory = $this->newDiffOpFactory(); - } - - $info['diff'] = $factory->newFromArray( $info['diff'] ); - } - if ( isset( $info['compactDiff'] ) && is_string( $info['compactDiff'] ) && $info['compactDiff'] ) { @@ -385,26 +319,6 @@ } return $info; - } - - /** - * @return DiffOpFactory - */ - private function newDiffOpFactory() { - return new EntityTypeAwareDiffOpFactory( function ( array $data ) { - if ( is_array( $data ) && isset( $data['_claimclass_'] ) ) { - $class = $data['_claimclass_']; - - if ( $class === Statement::class - || is_subclass_of( $class, Statement::class ) - ) { - unset( $data['_claimclass_'] ); - return $this->getStatementDeserializer()->deserialize( $data ); - } - } - - return $data; - } ); } } diff --git a/lib/includes/Changes/ItemChange.php b/lib/includes/Changes/ItemChange.php index 706d55e..11329be 100644 --- a/lib/includes/Changes/ItemChange.php +++ b/lib/includes/Changes/ItemChange.php @@ -6,7 +6,6 @@ use Diff\DiffOpChange; use Diff\DiffOp\Diff\Diff; use Diff\DiffOpRemove; -use Wikibase\DataModel\Services\Diff\ItemDiff; use Wikibase\Lib\Changes\EntityDiffChangedAspects; /** @@ -20,28 +19,12 @@ * @return Diff */ public function getSiteLinkDiff() { - $info = $this->getInfo(); - if ( !array_key_exists( 'diff', $info ) ) { - if ( !array_key_exists( 'compactDiff', $info ) ) { - $this->logWarning( $this ); - return new Diff(); - } - - $aspects = $info['compactDiff']; - if ( !( $aspects instanceof EntityDiffChangedAspects ) ) { - $this->logWarning( $aspects ); - return new Diff(); - } - return $this->getDiffFromSiteLinkChanges( $aspects->getSiteLinkChanges() ); - - } - $diff = $this->getDiff(); - if ( !( $diff instanceof ItemDiff ) ) { - $this->logWarning( $diff ); + $aspects = $this->getCompactDiff(); + if ( !( $aspects instanceof EntityDiffChangedAspects ) ) { + $this->logWarning( $aspects ); return new Diff(); - } else { - return $diff->getSiteLinkDiff(); } + return $this->getDiffFromSiteLinkChanges( $aspects->getSiteLinkChanges() ); } private function getDiffFromSiteLinkChanges( array $siteLinkChanges ) { diff --git a/lib/tests/phpunit/Changes/EntityChangeTest.php b/lib/tests/phpunit/Changes/EntityChangeTest.php index 027a149..d5dd589 100644 --- a/lib/tests/phpunit/Changes/EntityChangeTest.php +++ b/lib/tests/phpunit/Changes/EntityChangeTest.php @@ -2,11 +2,9 @@ namespace Wikibase\Lib\Tests\Changes; -use Diff\DiffOp\DiffOpAdd; use MWException; use RecentChange; use Revision; -use RuntimeException; use stdClass; use Wikibase\Lib\Changes\EntityDiffChangedAspects; use Wikimedia\TestingAccessWrapper; @@ -14,8 +12,6 @@ use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\Item; use Wikibase\DataModel\Entity\ItemId; -use Wikibase\DataModel\Snak\PropertyNoValueSnak; -use Wikibase\DataModel\Statement\Statement; use Wikibase\EntityChange; use Wikibase\ItemContent; use Wikibase\WikibaseSettings; @@ -340,17 +336,10 @@ } public function testDoesNotSerializeObjects() { - $info = [ 'array' => [ 'object' => new EntityChange() ] ]; + $info = [ 'array' => [ 'object' => new stdClass() ] ]; $change = new EntityChange( [ 'info' => $info ] ); $this->setExpectedException( MWException::class ); $change->getSerializedInfo(); - } - - public function testSerializeAndUnserializeInfo() { - $info = [ 'diff' => new DiffOpAdd( '' ) ]; - $change = new EntityChange( [ 'info' => $info ] ); - $change->setField( 'info', $change->getSerializedInfo() ); - $this->assertEquals( $info, $change->getInfo() ); } public function testSerializeAndUnserializeInfoCompactDiff() { @@ -379,49 +368,6 @@ $change = new EntityChange( [ 'info' => $info ] ); $change->setField( 'info', $change->getSerializedInfo() ); $this->assertEquals( $info, $change->getInfo() ); - } - - public function testGivenStatement_serializeInfoSerializesStatement() { - $statement = new Statement( new PropertyNoValueSnak( 1 ) ); - $info = [ 'diff' => new DiffOpAdd( $statement ) ]; - $expected = [ - 'mainsnak' => [ - 'snaktype' => 'novalue', - 'property' => 'P1', - 'hash' => 'any hash', - ], - 'type' => 'statement', - 'rank' => 'normal', - '_claimclass_' => Statement::class, - ]; - - $change = new EntityChange( [ 'info' => $info ] ); - - if ( !WikibaseSettings::isRepoEnabled() ) { - $this->setExpectedException( RuntimeException::class ); - } - - $json = $change->getSerializedInfo(); - $array = json_decode( $json, true ); - $array['diff']['newvalue']['mainsnak']['hash'] = 'any hash'; - $this->assertSame( $expected, $array['diff']['newvalue'] ); - } - - public function testGivenStatementSerialization_getInfoDeserializesStatement() { - $data = [ - 'mainsnak' => [ - 'snaktype' => 'novalue', - 'property' => 'P1', - ], - 'type' => 'statement', - '_claimclass_' => Statement::class, - ]; - $json = json_encode( [ 'diff' => [ 'type' => 'add', 'newvalue' => $data ] ] ); - - $change = new EntityChange( [ 'info' => $json ] ); - $info = $change->getInfo(); - $statement = $info['diff']->getNewValue(); - $this->assertInstanceOf( Statement::class, $statement ); } } diff --git a/lib/tests/phpunit/Changes/ItemChangeTest.php b/lib/tests/phpunit/Changes/ItemChangeTest.php index 7054689..67b2e7f 100644 --- a/lib/tests/phpunit/Changes/ItemChangeTest.php +++ b/lib/tests/phpunit/Changes/ItemChangeTest.php @@ -56,24 +56,6 @@ $this->assertInstanceOf( Diff::class, $siteLinkDiff, 'getSiteLinkDiff must return a Diff' ); } - /** - * @dataProvider changeProvider - * - * @param ItemChange $change - */ - public function testGetSiteLinkDiffFromCompactDiff( ItemChange $change ) { - $info = $change->getInfo(); - if ( !array_key_exists( 'compactDiff', $info ) ) { - $aspects = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( - $change->getDiff() - ); - $info['compactDiff'] = $aspects; - $change->setField( 'info', $info ); - } - $siteLinkDiff = $change->getSiteLinkDiff(); - $this->assertInstanceOf( Diff::class, $siteLinkDiff, 'getSiteLinkDiff must return a Diff' ); - } - public function changeBackwardsCompatProvider() { global $wgDevelopmentWarnings; @@ -89,10 +71,10 @@ // We may hit a plain diff generated by old code. // Make sure we can deal with that. - $diff = new Diff(); - $change = new ItemChange( [ 'type' => 'test' ] ); - $change->setDiff( $diff ); + $change->setCompactDiff( + ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( new Diff() ) + ); $cases['plain-diff'] = [ $change ]; @@ -113,7 +95,9 @@ //NOTE: ItemChange's constructor may or may not already fix the bad diff. $change = new ItemChange( [ 'type' => 'test' ] ); - $change->setDiff( $diff ); + $change->setCompactDiff( + ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( $diff ) + ); $cases['atomic-sitelink-diff'] = [ $change ]; } finally { diff --git a/repo/tests/phpunit/includes/Store/Sql/SqlChangeStoreTest.php b/repo/tests/phpunit/includes/Store/Sql/SqlChangeStoreTest.php index 78c0bea..46432aa 100644 --- a/repo/tests/phpunit/includes/Store/Sql/SqlChangeStoreTest.php +++ b/repo/tests/phpunit/includes/Store/Sql/SqlChangeStoreTest.php @@ -6,6 +6,7 @@ use Diff\DiffOp\Diff\Diff; use Wikibase\DataModel\Entity\ItemId; use Wikibase\EntityChange; +use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; use Wikibase\Repo\Store\Sql\SqlChangeStore; use Wikibase\Repo\WikibaseRepo; @@ -33,7 +34,9 @@ $changeWithDiff = $factory->newForEntity( EntityChange::REMOVE, new ItemId( 'Q42' ) ); $changeWithDiff->setField( 'time', $time ); - $changeWithDiff->setDiff( new Diff() ); + $changeWithDiff->setCompactDiff( + ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( new Diff() ) + ); $rc = new RecentChange(); $rc->setAttribs( [ @@ -69,7 +72,9 @@ 'change_object_id' => 'Q42', 'change_revision_id' => '0', 'change_user_id' => '0', - 'change_info' => '{"diff":{"type":"diff","isassoc":null,"operations":[]}}', + 'change_info' => '{"compactDiff":"{\"arrayFormatVersion\":1,' . + '\"labelChanges\":[],\"descriptionChanges\":[],\"statementChanges\":[],' . + '\"siteLinkChanges\":[],\"otherChanges\":false}"}', ], $changeWithDiff ], -- To view, visit https://gerrit.wikimedia.org/r/397798 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ieee787509a33e75e2ced2d161dd750fc0bb9be57 Gerrit-PatchSet: 9 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Ladsgroup <ladsgr...@gmail.com> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Ladsgroup <ladsgr...@gmail.com> Gerrit-Reviewer: Thiemo Kreuz (WMDE) <thiemo.kr...@wikimedia.de> Gerrit-Reviewer: WMDE-leszek <leszek.mani...@wikimedia.de> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits