Skip to content

Fix critical StatementListPatcher regressions #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JonasKress merged 1 commit into from
May 12, 2016
Merged

Fix critical StatementListPatcher regressions #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 44 additions & 5 deletions src/Diff/Internal/StatementListPatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,21 @@ public function patchStatementList( StatementList $statements, Diff $patch ) {
switch ( true ) {
case $diffOp instanceof DiffOpAdd:
/** @var DiffOpAdd $diffOp */
$statements->addStatement( $diffOp->getNewValue() );
/** @var Statement $statement */
$statement = $diffOp->getNewValue();
$guid = $statement->getGuid();
if ( $statements->getFirstStatementWithGuid( $guid ) === null ) {
$statements->addStatement( $statement );
}
break;

case $diffOp instanceof DiffOpChange:
/** @var DiffOpChange $diffOp */
/** @var Statement $statement */
$statement = $diffOp->getOldValue();
$statements->removeStatementsWithGuid( $statement->getGuid() );
$statements->addStatement( $diffOp->getNewValue() );
/** @var Statement $oldStatement */
/** @var Statement $newStatement */
$oldStatement = $diffOp->getOldValue();
$newStatement = $diffOp->getNewValue();
$this->changeStatement( $statements, $oldStatement->getGuid(), $newStatement );
break;

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

/**
* @param StatementList $statements
* @param string|null $oldGuid
* @param Statement $newStatement
*/
private function changeStatement( StatementList $statements, $oldGuid, Statement $newStatement ) {
$replacements = array();

foreach ( $statements->toArray() as $statement ) {
$guid = $statement->getGuid();

// Collect all elements starting from the first with the same GUID
if ( $replacements !== array() ) {
$guid === null
? $replacements[] = $statement
: $replacements[$guid] = $statement;
} elseif ( $guid === $oldGuid ) {
$guid === null
? $replacements[] = $newStatement
: $replacements[$guid] = $newStatement;
}
}

// Remove all starting from the one that should be replaced
foreach ( $replacements as $guid => $statement ) {
$statements->removeStatementsWithGuid( is_int( $guid ) ? null : $guid );
}
// Re-add all starting from the new one
foreach ( $replacements as $statement ) {
$statements->addStatement( $statement );
}
}

/**
* @deprecated since 3.6, use patchStatementList instead
*
Expand Down
128 changes: 128 additions & 0 deletions tests/unit/Diff/Internal/StatementListPatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,134 @@ public function testPatchStatementList(
$this->assertEquals( $expected, $statements );
}

/**
* @SuppressWarnings(PHPMD.ExcessiveMethodLength)
*/
public function statementOrderProvider() {
$statement1 = new Statement( new PropertyNoValueSnak( 1 ), null, null, 's1' );
$statement2 = new Statement( new PropertyNoValueSnak( 2 ), null, null, 's2' );
$statement3 = new Statement( new PropertyNoValueSnak( 3 ), null, null, 's3' );

return array(
'Simple associative add' => array(
new StatementList(),
new Diff( array(
's1' => new DiffOpAdd( $statement1 ),
), true ),
array( 's1' )
),
'Simple non-associative add' => array(
new StatementList(),
new Diff( array(
's1' => new DiffOpAdd( $statement1 ),
), false ),
array( 's1' )
),
'Simple associative remove' => array(
new StatementList( $statement1 ),
new Diff( array(
's1' => new DiffOpRemove( $statement1 ),
), true ),
array()
),
'Simple non-associative remove' => array(
new StatementList( $statement1 ),
new Diff( array(
's1' => new DiffOpRemove( $statement1 ),
), false ),
array()
),

// Change operations
'Remove and add' => array(
new StatementList( $statement1 ),
new Diff( array(
's1' => new DiffOpRemove( $statement1 ),
's2' => new DiffOpAdd( $statement2 ),
) ),
array( 's2' )
),
'Add and remove' => array(
new StatementList( $statement1 ),
new Diff( array(
's2' => new DiffOpAdd( $statement2 ),
's1' => new DiffOpRemove( $statement1 ),
) ),
array( 's2' )
),
'Simple associative replace' => array(
new StatementList( $statement1 ),
new Diff( array(
's1' => new DiffOpChange( $statement1, $statement2 ),
), true ),
array( 's2' )
),
'Simple non-associative replace' => array(
new StatementList( $statement1 ),
new Diff( array(
's1' => new DiffOpChange( $statement1, $statement2 ),
), false ),
array( 's2' )
),
'Replacing first element retains order' => array(
new StatementList( $statement1, $statement2 ),
new Diff( array(
's1' => new DiffOpChange( $statement1, $statement3 ),
) ),
array( 's3', 's2' )
),
'Replacing last element retains order' => array(
new StatementList( $statement1, $statement2 ),
new Diff( array(
's2' => new DiffOpChange( $statement2, $statement3 ),
) ),
array( 's1', 's3' )
),

// No-ops
'Empty diff' => array(
new StatementList( $statement1 ),
new Diff(),
array( 's1' )
),
'Adding existing element is no-op' => array(
new StatementList( $statement1 ),
new Diff( array(
's1' => new DiffOpAdd( $statement1 ),
) ),
array( 's1' )
),
'Removing non-existing element is no-op' => array(
new StatementList( $statement1 ),
new Diff( array(
's2' => new DiffOpRemove( $statement2 ),
) ),
array( 's1' )
),
'Replacing non-existing element is no-op' => array(
new StatementList( $statement1 ),
new Diff( array(
's2' => new DiffOpChange( $statement2, $statement3 ),
) ),
array( 's1' )
),
);
}

/**
* @dataProvider statementOrderProvider
*/
public function testStatementOrder( StatementList $statements, Diff $patch, array $expectedGuids ) {
$patcher = new StatementListPatcher();
$patchedStatements = $patcher->getPatchedStatementList( $statements, $patch );

$guids = array();
foreach ( $patchedStatements->toArray() as $statement ) {
$guids[] = $statement->getGuid();
}
$this->assertSame( $expectedGuids, $guids );
}

public function testGivenEmptyDiff_listIsReturnedAsIs() {
$statements = new StatementList();

Expand Down