Hello Ladsgroup, jenkins-bot, Thiemo Mättig (WMDE),
I'd like you to do a code review. Please visit
https://gerrit.wikimedia.org/r/395833
to review the following change.
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.
Change-Id: I9e7c8f18be89b6bea17286c9e5014d330f310002
---
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(+), 65 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/33/395833/1
diff --git a/client/includes/Changes/AffectedPagesFinder.php
b/client/includes/Changes/AffectedPagesFinder.php
index 9b0850e..cec0eeb 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 {
@@ -218,7 +218,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 c10438b..9c62ac9 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;
/**
@@ -295,14 +292,6 @@
}
}
- if ( isset( $info['compactDiff'] ) ) {
- $diff = $info['compactDiff'];
-
- if ( $diff instanceof EntityDiffChangedAspects ) {
- $info['compactDiff'] = $diff->serialize();
- }
- }
-
// Make sure we never serialize objects.
// This is a lot of overhead, so we only do it during testing.
if ( defined( 'MW_PHPUNIT_TEST' ) ) {
@@ -372,14 +361,6 @@
}
$info['diff'] = $factory->newFromArray( $info['diff'] );
- }
-
- if ( isset( $info['compactDiff'] ) && is_array(
$info['compactDiff'] ) && $info['compactDiff'] ) {
- $compactDiff = ( new EntityDiffChangedAspectsFactory()
)->newFromEntityDiff(
- new Diff()
- );
- $compactDiff->unserialize( $info['compactDiff'] );
- $info['compactDiff'] = $compactDiff;
}
return $info;
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/395833
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e7c8f18be89b6bea17286c9e5014d330f310002
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Hoo man <h...@online.de>
Gerrit-Reviewer: Ladsgroup <ladsgr...@gmail.com>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <thiemo.kr...@wikimedia.de>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
