jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/391900 )
Change subject: Change handling of sitelinks in EntityDiffChangedAspects
......................................................................
Change handling of sitelinks in EntityDiffChangedAspects
Per discussions in I0ac579e2063dfe92a974841330586423e5e7f393
Bug: T113468
Change-Id: Id185a573c5e0dab5b2cae6e389d5b1018cac2a04
---
M lib/includes/Changes/EntityDiffChangedAspects.php
M lib/includes/Changes/EntityDiffChangedAspectsFactory.php
M lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
M lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
4 files changed, 75 insertions(+), 22 deletions(-)
Approvals:
jenkins-bot: Verified
Thiemo Mättig (WMDE): Looks good to me, approved
diff --git a/lib/includes/Changes/EntityDiffChangedAspects.php
b/lib/includes/Changes/EntityDiffChangedAspects.php
index 20b8615..b3cc89d 100644
--- a/lib/includes/Changes/EntityDiffChangedAspects.php
+++ b/lib/includes/Changes/EntityDiffChangedAspects.php
@@ -44,10 +44,10 @@
private $statementChanges;
/**
- * Map of site ids to bool. The bool indicates whether only the badge
has changed (false)
- * or the actual value of the sitelink changed (true).
+ * Map of site ids to array of old value, new value and boolean value
determining if badge
+ * has changed or not
*
- * @var bool[]
+ * @var array[]
*/
private $siteLinkChanges;
@@ -78,7 +78,7 @@
Assert::parameterElementType( 'string', $descriptionChanges,
'$descriptionChanges' );
Assert::parameterElementType( 'string', $statementChanges,
'$statementChanges' );
Assert::parameterElementType( 'string', array_keys(
$siteLinkChanges ), 'array_keys( $siteLinkChanges )' );
- Assert::parameterElementType( 'boolean', $siteLinkChanges,
'$siteLinkChanges' );
+ Assert::parameterElementType( 'array', $siteLinkChanges,
'$siteLinkChanges' );
Assert::parameterType( 'boolean', $otherChanges,
'$otherChanges' );
$this->labelChanges = $labelChanges;
@@ -116,9 +116,10 @@
}
/**
- * Map of site ids to bool: only the badge has changed (false) or the
actual sitelink changed (true)
+ * Map of site ids to array of old value, new value and boolean value
determining if badge
+ * has changed or not
*
- * @return bool[]
+ * @return array[]
*/
public function getSiteLinkChanges() {
return $this->siteLinkChanges;
diff --git a/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
b/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
index 0d7b25a..2ab1671 100644
--- a/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
+++ b/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
@@ -83,7 +83,7 @@
$siteLinkChanges = [];
foreach ( $siteLinkDiff as $siteId => $diffPerSite ) {
- $siteLinkChanges[$siteId] = !$this->isBadgesOnlyChange(
$diffPerSite );
+ $siteLinkChanges[$siteId] =
$this->getSiteLinkChangePerSite( $diffPerSite );
}
return $siteLinkChanges;
@@ -92,10 +92,26 @@
/**
* @param DiffOp $siteLinkDiffOp
*
- * @return bool
+ * @return array [ string|null $oldPageName, string|null $newPageName,
bool $badgesChanged ]
*/
- private function isBadgesOnlyChange( DiffOp $siteLinkDiffOp ) {
- return $siteLinkDiffOp instanceof Diff && !array_key_exists(
'name', $siteLinkDiffOp );
+ private function getSiteLinkChangePerSite( DiffOp $siteLinkDiffOp ) {
+ if ( !$siteLinkDiffOp instanceof Diff ) {
+ return [ null, null, false ];
+ }
+
+ $removals = $siteLinkDiffOp->getRemovedValues();
+ $additions = $siteLinkDiffOp->getAddedValues();
+ $changes = $siteLinkDiffOp->getChanges();
+
+ $oldValue = ( array_key_exists( 'name', $removals ) ) ?
$removals['name'] : null;
+ $newValue = ( array_key_exists( 'name', $additions ) ) ?
$additions['name'] : null;
+
+ if ( array_key_exists( 'name', $changes ) ) {
+ $oldValue = $changes['name']->getOldValue();
+ $newValue = $changes['name']->getNewValue();
+ }
+
+ return [ $oldValue, $newValue, array_key_exists( 'badges',
$siteLinkDiffOp ) ];
}
/**
diff --git a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
index ca19dd3..a0e87f8 100644
--- a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
+++ b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
@@ -56,17 +56,23 @@
$statementP1P2Diff['statementChanges'] = [ 'P1', 'P2' ];
$siteLinkDiff = $emptyDiff;
- $siteLinkDiff['siteLinkChanges'] = [ 'enwiki' => true ];
+ $siteLinkDiff['siteLinkChanges'] = [ 'enwiki' => [ null, 'PHP',
false ] ];
+
+ $siteLinkWithBadgeDiff = $emptyDiff;
+ $siteLinkWithBadgeDiff['siteLinkChanges'] = [ 'enwiki' => [
null, 'PHP', true ] ];
$siteLinkBadgeOnlyDiff = $emptyDiff;
- $siteLinkBadgeOnlyDiff['siteLinkChanges'] = [ 'enwiki' => false
];
+ $siteLinkBadgeOnlyDiff['siteLinkChanges'] = [ 'enwiki' => [
null, null, true ] ];
$berlinEmptyDiff = [
'arrayFormatVersion' =>
EntityDiffChangedAspects::ARRAYFORMATVERSION,
'labelChanges' => [ 'de', 'ru' ],
'descriptionChanges' => [ 'de', 'es' ],
'statementChanges' => [ 'P2' ],
- 'siteLinkChanges' => [ 'dewiki' => true, 'enwiki' =>
true ],
+ 'siteLinkChanges' => [
+ 'dewiki' => [ null, 'Berlin', true ],
+ 'enwiki' => [ null, 'Berlin', false ]
+ ],
'otherChanges' => false,
];
@@ -75,7 +81,11 @@
'labelChanges' => [ 'fr', 'ru' ],
'descriptionChanges' => [ 'de', 'es', 'pl' ],
'statementChanges' => [ 'P2' ],
- 'siteLinkChanges' => [ 'dewiki' => true, 'enwiki' =>
true, 'ruwiki' => true ],
+ 'siteLinkChanges' => [
+ 'dewiki' => [ null, 'Paris', true ],
+ 'enwiki' => [ null, 'Paris', false ],
+ 'ruwiki' => [ null, 'Paris', false ]
+ ],
'otherChanges' => false,
];
@@ -84,7 +94,11 @@
'labelChanges' => [ 'de', 'fr', 'ru' ],
'descriptionChanges' => [ 'pl' ],
'statementChanges' => [ 'P2' ],
- 'siteLinkChanges' => [ 'dewiki' => true, 'enwiki' =>
true, 'ruwiki' => true ],
+ 'siteLinkChanges' => [
+ 'dewiki' => [ 'Berlin', 'Paris', false ],
+ 'enwiki' => [ 'Berlin', 'Paris', false ],
+ 'ruwiki' => [ null, 'Paris', false ]
+ ],
'otherChanges' => false,
];
@@ -250,7 +264,7 @@
$siteLinkItem
],
'sitelink change with badge' => [
- $siteLinkDiff,
+ $siteLinkWithBadgeDiff,
$emptyItem,
$siteLinkBadgeItem
],
@@ -280,7 +294,7 @@
$reverseTests = [];
foreach ( $cases as $testDescription => $case ) {
$reverseTests[$testDescription . ' (reversed)'] = [
- $case[0],
+ $this->reverseDiff( $case[0] ),
$case[2],
$case[1]
];
@@ -289,6 +303,28 @@
return array_merge( $cases, $reverseTests );
}
+ private function reverseDiff( array $diffs ) {
+ $newDiff = [];
+ foreach ( $diffs as $diffType => $diff ) {
+ if ( $diffType === 'siteLinkChanges' ) {
+ $newDiff[$diffType] =
$this->reverseSiteLinkDiff( $diff );
+ continue;
+ }
+
+ $newDiff[$diffType] = $diff;
+ }
+
+ return $newDiff;
+ }
+
+ private function reverseSiteLinkDiff( $diff ) {
+ $newSiteLinkDiff = [];
+ foreach ( $diff as $site => $siteDiff ) {
+ $newSiteLinkDiff[$site] = [ $siteDiff[1], $siteDiff[0],
$siteDiff[2] ];
+ }
+ return $newSiteLinkDiff;
+ }
+
/**
* @dataProvider provideNewFromEntityDiff
*/
diff --git a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
index bd1bd23..3d9cee9 100644
--- a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
+++ b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
@@ -23,7 +23,7 @@
'labelChanges' => [ 'a', '1' ],
'descriptionChanges' => [ 'b', '2' ],
'statementChanges' => [ 'c', '3' ],
- 'siteLinkChanges' => [ 'd' => true ],
+ 'siteLinkChanges' => [ 'd' => [ null, null, true ] ],
'otherChanges' => true,
];
@@ -37,7 +37,7 @@
$invalidStatementChanges['statementChanges'] = [ 'c', 3 ];
$invalidSiteLinkChangesKeys = $validParams;
- $invalidSiteLinkChangesKeys['siteLinkChanges'] = [ 1 => true ];
+ $invalidSiteLinkChangesKeys['siteLinkChanges'] = [ 1 => [ null,
null, true ] ];
$invalidSiteLinkChangesValues = $validParams;
$invalidSiteLinkChangesValues['siteLinkChanges'] = [ 'd' => 12
];
@@ -75,7 +75,7 @@
[ 'a', '1' ],
[ 'b', '2' ],
[ 'c', '3' ],
- [ 'd' => true ],
+ [ 'd' => [ null, null, true ] ],
true
);
}
@@ -103,7 +103,7 @@
public function testGetSiteLinkChanges() {
$this->assertSame(
- [ 'd' => true ],
+ [ 'd' => [ null, null, true ] ],
$this->getEntityDiffChangedAspects()->getSiteLinkChanges()
);
}
@@ -121,7 +121,7 @@
'labelChanges' => [ 'a', '1' ],
'descriptionChanges' => [ 'b', '2' ],
'statementChanges' => [ 'c', '3' ],
- 'siteLinkChanges' => [ 'd' => true ],
+ 'siteLinkChanges' => [ 'd' => [ null, null, true ] ],
'otherChanges' => true,
];
--
To view, visit https://gerrit.wikimedia.org/r/391900
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id185a573c5e0dab5b2cae6e389d5b1018cac2a04
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Ladsgroup <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Hoo man <[email protected]>
Gerrit-Reviewer: Ladsgroup <[email protected]>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <[email protected]>
Gerrit-Reviewer: WMDE-leszek <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
