Hoo man has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/395837 )
Change subject: Revert "Transmit compact diff instead of suppressed diff" ...................................................................... Revert "Transmit compact diff instead of suppressed diff" At least for wmf11 we need to keep transmitting the old diff, wmf10 client's can't yet read the new one. This is because https://gerrit.wikimedia.org/r/393780 and https://gerrit.wikimedia.org/r/394432 are needed on all wikis first. This reverts commit f77b47c5e30c297a43e4ce354c1dfb4a29a638fa. Bug: T182243 Change-Id: Icc06c9949aa4c708fc615feee35639786e99ddca --- M client/includes/Changes/AffectedPagesFinder.php M client/tests/phpunit/includes/Changes/ChangeRunCoalescerTest.php M lib/includes/Changes/DiffChange.php M lib/includes/Changes/EntityChange.php M lib/includes/Changes/EntityChangeFactory.php M lib/tests/phpunit/Changes/EntityChangeFactoryTest.php 6 files changed, 72 insertions(+), 57 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/37/395837/1 diff --git a/client/includes/Changes/AffectedPagesFinder.php b/client/includes/Changes/AffectedPagesFinder.php index a5450e2..4794df7 100644 --- a/client/includes/Changes/AffectedPagesFinder.php +++ b/client/includes/Changes/AffectedPagesFinder.php @@ -122,7 +122,7 @@ $aspects = []; $info = $change->getInfo(); - // We might unserialize old EntityChange which doesn't have getCompactDiff method + // We might unserialize old EntityChange which doesn't have getAspectsDiff method if ( array_key_exists( 'compactDiff', $info ) ) { $diffAspects = $info['compactDiff']; } else { @@ -240,7 +240,7 @@ if ( $change instanceof ItemChange && in_array( EntityUsage::TITLE_USAGE, $changedAspects ) ) { $info = $change->getInfo(); - // We might unserialize old EntityChange which doesn't have getCompactDiff method + // We might unserialize old EntityChange which doesn't have getAspectsDiff method if ( array_key_exists( 'compactDiff', $info ) ) { $diffChangedAspects = $info['compactDiff']; } else { diff --git a/client/tests/phpunit/includes/Changes/ChangeRunCoalescerTest.php b/client/tests/phpunit/includes/Changes/ChangeRunCoalescerTest.php index 8081361..4bc5c1c 100644 --- a/client/tests/phpunit/includes/Changes/ChangeRunCoalescerTest.php +++ b/client/tests/phpunit/includes/Changes/ChangeRunCoalescerTest.php @@ -2,6 +2,8 @@ namespace Wikibase\Client\Tests\Changes; +use Diff\DiffOp\AtomicDiffOp; +use Traversable; use Wikibase\Change; use Wikibase\Client\Changes\ChangeRunCoalescer; use Wikibase\DataModel\Entity\Item; @@ -10,7 +12,6 @@ use Wikibase\DataModel\SiteLink; use Wikibase\EntityChange; use Wikibase\ItemChange; -use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; use Wikibase\Lib\Store\EntityRevisionLookup; use Wikibase\Lib\Tests\MockRepository; use Wikibase\Lib\Tests\Changes\TestChanges; @@ -132,8 +133,7 @@ } $change->setEntityId( new ItemId( $values['object_id'] ) ); - $diffAspects = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( $diff ); - $change->setCompactDiff( $diffAspects ); + $change->setDiff( $diff ); return $change; } @@ -192,10 +192,29 @@ $this->assertArrayEquals( $expected->getMetadata(), $actual->getMetadata(), false, true ); } - $this->assertSame( - $expected->getCompactDiff()->toArray(), - $actual->getCompactDiff()->toArray() - ); + $this->assertDiffsEqual( $expected->getDiff(), $actual->getDiff() ); + } + + private function assertDiffsEqual( $expected, $actual, $path = '' ) { + if ( $expected instanceof AtomicDiffOp ) { + //$this->assertEquals( $expected->getType(), $actual->getType(), $path . ' DiffOp.type' ); + $this->assertEquals( serialize( $expected ), serialize( $actual ), $path . ' DiffOp' ); + return; + } + + if ( $expected instanceof Traversable ) { + $expected = iterator_to_array( $expected ); + $actual = iterator_to_array( $actual ); + } + + foreach ( $expected as $key => $expectedValue ) { + $currentPath = "$path/$key"; + $this->assertArrayHasKey( $key, $actual, $currentPath . " missing key" ); + $this->assertDiffsEqual( $expectedValue, $actual[$key], $currentPath ); + } + + $extraKeys = array_diff( array_keys( $actual ), array_keys( $expected ) ); + $this->assertEquals( [], $extraKeys, $path . " extra keys" ); } public function provideCoalesceChanges() { diff --git a/lib/includes/Changes/DiffChange.php b/lib/includes/Changes/DiffChange.php index f004ca4..7c651f6 100644 --- a/lib/includes/Changes/DiffChange.php +++ b/lib/includes/Changes/DiffChange.php @@ -3,8 +3,6 @@ namespace Wikibase; use Diff\DiffOp\Diff\Diff; -use Wikibase\Lib\Changes\EntityDiffChangedAspects; -use Wikibase\Lib\Changes\EntityDiffChangedAspectsFactory; /** * Class for changes that can be represented as a Diff. @@ -36,28 +34,6 @@ public function setDiff( Diff $diff ) { $info = $this->getInfo(); $info['diff'] = $diff; - $this->setField( 'info', $info ); - } - - /** - * @return EntityDiffChangedAspects - */ - public function getCompactDiff() { - $info = $this->getInfo(); - - if ( !array_key_exists( 'compactDiff', $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 EntityDiffChangedAspectsFactory() )->newFromEntityDiff( new Diff() ); - } else { - return $info['compactDiff']; - } - } - - public function setCompactDiff( EntityDiffChangedAspects $diff ) { - $info = $this->getInfo(); - $info['compactDiff'] = $diff; $this->setField( 'info', $info ); } diff --git a/lib/includes/Changes/EntityChange.php b/lib/includes/Changes/EntityChange.php index 9c6755c..ca993bd 100644 --- a/lib/includes/Changes/EntityChange.php +++ b/lib/includes/Changes/EntityChange.php @@ -3,7 +3,6 @@ namespace Wikibase; use Deserializers\Deserializer; -use Diff\DiffOp\Diff\Diff; use Diff\DiffOp\DiffOp; use Diff\DiffOpFactory; use MWException; @@ -17,8 +16,6 @@ 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; /** @@ -292,14 +289,6 @@ return $array; } ); - } - } - - if ( isset( $info['compactDiff'] ) ) { - $diff = $info['compactDiff']; - - if ( $diff instanceof EntityDiffChangedAspects ) { - $info['compactDiff'] = $diff->serialize(); } } diff --git a/lib/includes/Changes/EntityChangeFactory.php b/lib/includes/Changes/EntityChangeFactory.php index ba2b3cd..82c2aa9 100644 --- a/lib/includes/Changes/EntityChangeFactory.php +++ b/lib/includes/Changes/EntityChangeFactory.php @@ -8,6 +8,9 @@ use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\EntityIdParser; use Wikibase\DataModel\Services\Diff\EntityDiffer; +use Wikibase\DataModel\Statement\StatementListProvider; +use Wikibase\DataModel\Term\AliasGroupList; +use Wikibase\DataModel\Term\FingerprintProvider; use Wikibase\EntityChange; use Wikimedia\Assert\Assert; @@ -137,6 +140,9 @@ throw new MWException( 'Either $oldEntity or $newEntity must be given' ); } + $this->minimizeEntityForDiffing( $oldEntity ); + $this->minimizeEntityForDiffing( $newEntity ); + if ( $oldEntity === null ) { $id = $newEntity->getId(); $diff = $this->entityDiffer->getConstructionDiff( $newEntity ); @@ -151,10 +157,29 @@ } $instance = $this->newForEntity( $action, $id ); - $aspectsDiff = ( new EntityDiffChangedAspectsFactory() )->newFromEntityDiff( $diff ); - $instance->setCompactDiff( $aspectsDiff ); + $instance->setDiff( $diff ); return $instance; } + /** + * Hack: Don't include statement, description and alias diffs, since those are unused and not + * helpful performance-wise to the dispatcher and change handling. + * + * @fixme Implement T113468 and remove this. + * + * @param EntityDocument|null $entity + */ + private function minimizeEntityForDiffing( EntityDocument $entity = null ) { + if ( $entity instanceof StatementListProvider ) { + $entity->getStatements()->clear(); + } + + if ( $entity instanceof FingerprintProvider ) { + $fingerprint = $entity->getFingerprint(); + + $fingerprint->setAliasGroups( new AliasGroupList() ); + } + } + } diff --git a/lib/tests/phpunit/Changes/EntityChangeFactoryTest.php b/lib/tests/phpunit/Changes/EntityChangeFactoryTest.php index 1beec9d..0ca1f57 100644 --- a/lib/tests/phpunit/Changes/EntityChangeFactoryTest.php +++ b/lib/tests/phpunit/Changes/EntityChangeFactoryTest.php @@ -2,6 +2,9 @@ namespace Wikibase\Lib\Tests\Changes; +use Diff\DiffOp\Diff\Diff; +use Diff\DiffOp\DiffOpAdd; +use Diff\DiffOp\DiffOpRemove; use Wikibase\DataModel\Entity\BasicEntityIdParser; use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\Item; @@ -126,8 +129,8 @@ $this->assertEquals( 'wikibase-item~update', $change->getType(), 'type' ); $this->assertEquals( - [ 'es' ], - $change->getCompactDiff()->getLabelChanges(), + new Diff( [ 'es' => new DiffOpAdd( 'gato' ) ] ), + $change->getDiff()->getLabelsDiff(), 'diff' ); } @@ -146,8 +149,8 @@ $this->assertEquals( 'wikibase-item~add', $change->getType(), 'type' ); $this->assertEquals( - [ 'en' ], - $change->getCompactDiff()->getLabelChanges(), + new Diff( [ 'en' => new DiffOpAdd( 'kitten' ) ] ), + $change->getDiff()->getLabelsDiff(), 'diff' ); } @@ -166,8 +169,8 @@ $this->assertEquals( 'wikibase-property~remove', $change->getType(), 'type' ); $this->assertEquals( - [ 'de' ], - $change->getCompactDiff()->getLabelChanges(), + new Diff( [ 'de' => new DiffOpRemove( 'Katze' ) ] ), + $change->getDiff()->getLabelsDiff(), 'diff' ); } @@ -186,8 +189,12 @@ $this->assertEquals( 'wikibase-item~restore', $change->getType(), 'type' ); $this->assertEquals( - [ 'enwiki' => [ null, 'Kitten', false ] ], - $change->getCompactDiff()->getSiteLinkChanges(), + new Diff( [ + 'enwiki' => new Diff( [ + 'name' => new DiffOpAdd( 'Kitten' ) + ] ) + ] ), + $change->getDiff()->getSiteLinkDiff(), 'diff' ); } @@ -211,9 +218,8 @@ $change = $factory->newFromUpdate( EntityChange::UPDATE, $item, $updatedItem ); - $this->assertSame( - [ 'P10', 'P9000' ], - $change->getCompactDiff()->getStatementChanges(), + $this->assertTrue( + $change->getDiff()->isEmpty(), 'Diff excludes statement changes and is empty' ); } -- To view, visit https://gerrit.wikimedia.org/r/395837 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Icc06c9949aa4c708fc615feee35639786e99ddca Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: wmf/1.31.0-wmf.11 Gerrit-Owner: Hoo man <h...@online.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
