Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/73763
Change subject: (bug 51363) Work around broken diffs.
......................................................................
(bug 51363) Work around broken diffs.
Without I8ddc15d9, MapDiffer will return atomic diffs for
associative substructures that changed only the order of
entries, resulting in failure of code that expected these
diffs to always be instances of Diff.
This patch works around the broken diffs.
Note that this patch is still needed even after I8ddc15d9 is applied,
since we may still load broken diffs from the database or elsewhere.
Change-Id: I3835156f35e168ca3acd71a299ef8a218cb3a633
---
M DataModel/EntityDiff.php
M DataModel/ItemDiff.php
M tests/phpunit/ItemDiffTest.php
3 files changed, 117 insertions(+), 22 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseDataModel
refs/changes/63/73763/1
diff --git a/DataModel/EntityDiff.php b/DataModel/EntityDiff.php
index 25094c8..b7e2c82 100644
--- a/DataModel/EntityDiff.php
+++ b/DataModel/EntityDiff.php
@@ -55,10 +55,48 @@
* @param \Diff\DiffOp[] $operations
*/
public function __construct( array $operations = array() ) {
+ $this->fixSubstructureDiff( $operations, 'aliases' );
+ $this->fixSubstructureDiff( $operations, 'label' );
+ $this->fixSubstructureDiff( $operations, 'description' );
+ $this->fixSubstructureDiff( $operations, 'claim' );
+
parent::__construct( $operations, true );
}
/**
+ * Checks the type of a substructure diff, and replaces it if needed.
+ * This is needed for backwards compatibility with old versions of
+ * MapDiffer: As of commit ff65735a125e, MapDiffer may generate atomic
diffs for
+ * substructures even in recursive mode (bug 51363).
+ *
+ * @param array &$operations All change ops; This is a reference, so the
+ * substructure diff can be replaced if need be.
+ * @param string $key The key of the substructure
+ */
+ protected function fixSubstructureDiff( array &$operations, $key ) {
+ if ( !isset( $operations[$key] ) ) {
+ return;
+ }
+
+ if ( !$operations[$key] instanceof Diff ) {
+ $warning = "Invalid substructure diff for key $key: " .
get_class( $operations[$key] );
+
+ if ( function_exists( 'wfLogWarning' ) ) {
+ wfLogWarning( $warning );
+ } else {
+ trigger_error( $warning, E_USER_WARNING );
+ }
+
+ // We could look into the atomic diff, see if it uses
arrays as values,
+ // and construct a new Diff according to these values.
But since the
+ // actual old behavior of MapDiffer didn't cause that
to happen, let's
+ // just use an empty diff, which is what MapDiffer
should have returned
+ // in the actual broken case mentioned in bug 51363.
+ $operations[$key] = new Diff( array(), true );
+ }
+ }
+
+ /**
* Returns a Diff object with the aliases differences.
*
* @since 0.1
diff --git a/DataModel/ItemDiff.php b/DataModel/ItemDiff.php
index d1d2b1c..0c5ee2d 100644
--- a/DataModel/ItemDiff.php
+++ b/DataModel/ItemDiff.php
@@ -3,6 +3,7 @@
namespace Wikibase;
use Diff\Diff;
+use Diff\DiffOp;
/**
* Represents a diff between two Wikibase\Item instances.
@@ -33,6 +34,17 @@
class ItemDiff extends EntityDiff {
/**
+ * Constructor.
+ *
+ * @param \Diff\DiffOp[] $operations
+ */
+ public function __construct( array $operations = array() ) {
+ $this->fixSubstructureDiff( $operations, 'links' );
+
+ parent::__construct( $operations, true );
+ }
+
+ /**
* Returns a Diff object with the sitelink differences.
*
* @since 0.1
@@ -41,28 +53,6 @@
*/
public function getSiteLinkDiff() {
return isset( $this['links'] ) ? $this['links'] : new Diff(
array(), true );
- }
-
- /**
- * Returns a Diff object with the sitelink differences.
- *
- * @since 0.1
- *
- * @return Diff
- */
- public function getClaimDiff() {
- return isset( $this['claim'] ) ? $this['claim'] : new Diff(
array(), true );
- }
-
- /**
- * Returns a Diff object with the sitelink differences.
- *
- * @since 0.1
- *
- * @return Diff
- */
- public function getLabelDiff() {
- return isset( $this['label'] ) ? $this['label'] : new Diff(
array(), true );
}
/**
diff --git a/tests/phpunit/ItemDiffTest.php b/tests/phpunit/ItemDiffTest.php
index 12c0569..cccbb9c 100644
--- a/tests/phpunit/ItemDiffTest.php
+++ b/tests/phpunit/ItemDiffTest.php
@@ -2,9 +2,14 @@
namespace Wikibase\Test;
+use Diff\Diff;
+use Diff\DiffOpAdd;
+use Diff\DiffOpChange;
use Wikibase\DataModel\SimpleSiteLink;
use Wikibase\Entity;
+use Wikibase\EntityDiff;
use Wikibase\Item;
+use Wikibase\ItemDiff;
/**
* @covers Wikibase\ItemDiff
@@ -39,8 +44,10 @@
* @author Daniel Kinzler
* @author Jens Ohlig <[email protected]>
* @author Jeroen De Dauw < [email protected] >
+ * @author Daniel Kinzler
*/
class ItemDiffTest extends EntityDiffOldTest {
+ //TODO: make the new EntityDiffTest also run for Items.
public static function provideApplyData() {
$originalTests = parent::generateApplyData(
\Wikibase\Item::ENTITY_TYPE );
@@ -101,4 +108,64 @@
$this->assertEquals( $a->getSimpleSiteLinks(),
$b->getSimpleSiteLinks() );
}
+ public function isEmptyProvider() {
+ $argLists = array();
+
+ $argLists['no ops'] = array( array(), true );
+
+ $argLists['label changed'] = array( array( 'label' => new Diff(
array( 'x' => new DiffOpAdd( 'foo' ) ) ) ), false );
+
+ $argLists['empty links diff'] = array( array( 'links' => new
Diff( array(), true ) ), true );
+
+ $argLists['non-empty links diff'] = array( array( 'links' =>
new Diff( array( new DiffOpAdd( 'foo' ) ), true ) ), false );
+
+ return $argLists;
+ }
+
+ /**
+ * @dataProvider isEmptyProvider
+ *
+ * @param array $diffOps
+ * @param boolean $isEmpty
+ */
+ public function testIsEmpty( array $diffOps, $isEmpty ) {
+ $diff = new ItemDiff( $diffOps );
+ $this->assertEquals( $isEmpty, $diff->isEmpty() );
+ }
+
+ /**
+ * Checks that ItemDiff can handle atomic diffs for substructures.
+ * This is needed for backwards compatibility with old versions of
+ * MapDiffer: As of commit ff65735a125e, MapDiffer may generate atomic
+ * diffs for substructures even in recursive mode (bug 51363).
+ */
+ public function testAtomicSubstructureWorkaround() {
+ $oldErrorLevel = error_reporting( E_ERROR );
+
+ try {
+ $atomicListDiff = new DiffOpChange(
+ array( 'a' => 'A', 'b' => 'B' ),
+ array( 'b' => 'B', 'a' => 'A' )
+ );
+
+ $diff = new ItemDiff( array(
+ 'aliases' => $atomicListDiff,
+ 'label' => $atomicListDiff,
+ 'description' => $atomicListDiff,
+ 'claim' => $atomicListDiff,
+ 'links' => $atomicListDiff,
+ ) );
+
+ $this->assertInstanceOf( '\Diff\Diff',
$diff->getAliasesDiff() );
+ $this->assertInstanceOf( '\Diff\Diff',
$diff->getLabelsDiff() );
+ $this->assertInstanceOf( '\Diff\Diff',
$diff->getDescriptionsDiff() );
+ $this->assertInstanceOf( '\Diff\Diff',
$diff->getClaimsDiff() );
+ $this->assertInstanceOf( '\Diff\Diff',
$diff->getSiteLinkDiff() );
+ } catch ( \Exception $ex ) { // PHP 5.3 doesn't have `finally`
+ // make sure we always restore the warning level
+ error_reporting( $oldErrorLevel );
+ throw $ex;
+ }
+ }
+
}
--
To view, visit https://gerrit.wikimedia.org/r/73763
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3835156f35e168ca3acd71a299ef8a218cb3a633
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikibaseDataModel
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
