Skip to content

Commit 54e54a9

Browse files
thiemowmdethiemowmde
committed
Fix critical StatementListPatcher regressions
1 parent 16dfc52 commit 54e54a9

2 files changed

Lines changed: 169 additions & 5 deletions

File tree

‎src/Diff/Internal/StatementListPatcher.php‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,21 @@ public function patchStatementList( StatementList $statements, Diff $patch ) {
4040
switch ( true ) {
4141
case $diffOp instanceof DiffOpAdd:
4242
/** @var DiffOpAdd $diffOp */
43-
$statements->addStatement( $diffOp->getNewValue() );
43+
/** @var Statement $statement */
44+
$statement = $diffOp->getNewValue();
45+
$guid = $statement->getGuid();
46+
if ( $statements->getFirstStatementWithGuid( $guid ) === null ) {
47+
$statements->addStatement( $statement );
48+
}
4449
break;
4550

4651
case $diffOp instanceof DiffOpChange:
4752
/** @var DiffOpChange $diffOp */
48-
/** @var Statement $statement */
49-
$statement = $diffOp->getOldValue();
50-
$statements->removeStatementsWithGuid( $statement->getGuid() );
51-
$statements->addStatement( $diffOp->getNewValue() );
53+
/** @var Statement $oldStatement */
54+
/** @var Statement $newStatement */
55+
$oldStatement = $diffOp->getOldValue();
56+
$newStatement = $diffOp->getNewValue();
57+
$this->changeStatement( $statements, $oldStatement->getGuid(), $newStatement );
5258
break;
5359

5460
case $diffOp instanceof DiffOpRemove:
@@ -64,6 +70,39 @@ public function patchStatementList( StatementList $statements, Diff $patch ) {
6470
}
6571
}
6672

73+
/**
74+
* @param StatementList $statements
75+
* @param string|null $oldGuid
76+
* @param Statement $newStatement
77+
*/
78+
private function changeStatement( StatementList $statements, $oldGuid, Statement $newStatement ) {
79+
$replacements = array();
80+
81+
foreach ( $statements->toArray() as $statement ) {
82+
$guid = $statement->getGuid();
83+
84+
// Collect all elements starting from the first with the same GUID
85+
if ( $replacements !== array() ) {
86+
$guid === null
87+
? $replacements[] = $statement
88+
: $replacements[$guid] = $statement;
89+
} elseif ( $guid === $oldGuid ) {
90+
$guid === null
91+
? $replacements[] = $newStatement
92+
: $replacements[$guid] = $newStatement;
93+
}
94+
}
95+
96+
// Remove all starting from the one that should be replaced
97+
foreach ( $replacements as $guid => $statement ) {
98+
$statements->removeStatementsWithGuid( is_int( $guid ) ? null : $guid );
99+
}
100+
// Re-add all starting from the new one
101+
foreach ( $replacements as $statement ) {
102+
$statements->addStatement( $statement );
103+
}
104+
}
105+
67106
/**
68107
* @deprecated since 3.6, use patchStatementList instead
69108
*

‎tests/unit/Diff/Internal/StatementListPatcherTest.php‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,131 @@ public function testPatchStatementList(
106106
$this->assertEquals( $expected, $statements );
107107
}
108108

109+
public function statementOrderProvider() {
110+
$statement1 = new Statement( new PropertyNoValueSnak( 1 ), null, null, 's1' );
111+
$statement2 = new Statement( new PropertyNoValueSnak( 2 ), null, null, 's2' );
112+
$statement3 = new Statement( new PropertyNoValueSnak( 3 ), null, null, 's3' );
113+
114+
return array(
115+
'Simple associative add' => array(
116+
new StatementList(),
117+
new Diff( array(
118+
's1' => new DiffOpAdd( $statement1 ),
119+
), true ),
120+
array( 's1' )
121+
),
122+
'Simple non-associative add' => array(
123+
new StatementList(),
124+
new Diff( array(
125+
's1' => new DiffOpAdd( $statement1 ),
126+
), false ),
127+
array( 's1' )
128+
),
129+
'Simple associative remove' => array(
130+
new StatementList( $statement1 ),
131+
new Diff( array(
132+
's1' => new DiffOpRemove( $statement1 ),
133+
), true ),
134+
array()
135+
),
136+
'Simple non-associative remove' => array(
137+
new StatementList( $statement1 ),
138+
new Diff( array(
139+
's1' => new DiffOpRemove( $statement1 ),
140+
), false ),
141+
array()
142+
),
143+
144+
// Change operations
145+
'Remove and add' => array(
146+
new StatementList( $statement1 ),
147+
new Diff( array(
148+
's1' => new DiffOpRemove( $statement1 ),
149+
's2' => new DiffOpAdd( $statement2 ),
150+
) ),
151+
array( 's2' )
152+
),
153+
'Add and remove' => array(
154+
new StatementList( $statement1 ),
155+
new Diff( array(
156+
's2' => new DiffOpAdd( $statement2 ),
157+
's1' => new DiffOpRemove( $statement1 ),
158+
) ),
159+
array( 's2' )
160+
),
161+
'Simple associative replace' => array(
162+
new StatementList( $statement1 ),
163+
new Diff( array(
164+
's1' => new DiffOpChange( $statement1, $statement2 ),
165+
), true ),
166+
array( 's2' )
167+
),
168+
'Simple non-associative replace' => array(
169+
new StatementList( $statement1 ),
170+
new Diff( array(
171+
's1' => new DiffOpChange( $statement1, $statement2 ),
172+
), false ),
173+
array( 's2' )
174+
),
175+
'Replacing first element retains order' => array(
176+
new StatementList( $statement1, $statement2 ),
177+
new Diff( array(
178+
's1' => new DiffOpChange( $statement1, $statement3 ),
179+
) ),
180+
array( 's3', 's2' )
181+
),
182+
'Replacing last element retains order' => array(
183+
new StatementList( $statement1, $statement2 ),
184+
new Diff( array(
185+
's2' => new DiffOpChange( $statement2, $statement3 ),
186+
) ),
187+
array( 's1', 's3' )
188+
),
189+
190+
// No-ops
191+
'Empty diff' => array(
192+
new StatementList( $statement1 ),
193+
new Diff(),
194+
array( 's1' )
195+
),
196+
'Adding existing element is no-op' => array(
197+
new StatementList( $statement1 ),
198+
new Diff( array(
199+
's1' => new DiffOpAdd( $statement1 ),
200+
) ),
201+
array( 's1' )
202+
),
203+
'Removing non-existing element is no-op' => array(
204+
new StatementList( $statement1 ),
205+
new Diff( array(
206+
's2' => new DiffOpRemove( $statement2 ),
207+
) ),
208+
array( 's1' )
209+
),
210+
'Replacing non-existing element is no-op' => array(
211+
new StatementList( $statement1 ),
212+
new Diff( array(
213+
's2' => new DiffOpChange( $statement2, $statement3 ),
214+
) ),
215+
array( 's1' )
216+
),
217+
);
218+
}
219+
220+
/**
221+
* @dataProvider statementOrderProvider
222+
*/
223+
public function testStatementOrder( StatementList $statements, Diff $patch, array $expectedGuids ) {
224+
$patcher = new StatementListPatcher();
225+
$patchedStatements = $patcher->getPatchedStatementList( $statements, $patch );
226+
227+
$guids = array();
228+
foreach ( $patchedStatements->toArray() as $statement ) {
229+
$guids[] = $statement->getGuid();
230+
}
231+
$this->assertSame( $expectedGuids, $guids );
232+
}
233+
109234
public function testGivenEmptyDiff_listIsReturnedAsIs() {
110235
$statements = new StatementList();
111236

0 commit comments

Comments
 (0)