Skip to content

Commit c0676df

Browse files
JonasKressJonasKress
committed
Merge pull request #141 from wmde/statementPatcherPatch
Fix critical StatementListPatcher regressions
2 parents 7247aa0 + 77ce368 commit c0676df

2 files changed

Lines changed: 172 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: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,134 @@ public function testPatchStatementList(
106106
$this->assertEquals( $expected, $statements );
107107
}
108108

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

0 commit comments

Comments
 (0)