Skip to content

Commit

Permalink
Don't use numeric IDs for internal PHP serialization. (#716)
Browse files Browse the repository at this point in the history 
Don't use numeric IDs for internal PHP serialization of Snaks.

NOTE: it's unclear if this completely fixes T157442

Bug: T157442
  • Loading branch information
@manicki
Daniel Kinzler authored and manicki committed Feb 10, 2017
1 parent 4220561 commit 68667eb
Show file tree
Hide file tree
Showing 8 changed files with 162 additions and 33 deletions.
1 change: 0 additions & 1 deletion phpcs.xml
View file
Expand Up @@ -7,7 +7,6 @@
<rule ref="Generic.Arrays.DisallowLongArraySyntax" />

<rule ref="Generic.Classes" />
<rule ref="Generic.CodeAnalysis" />
<rule ref="Generic.ControlStructures" />

<rule ref="Generic.Files.ByteOrderMark" />
Expand Down
14 changes: 10 additions & 4 deletions src/Snak/PropertyValueSnak.php
View file
Expand Up @@ -48,12 +48,12 @@ public function getDataValue() {
/**
* @see Serializable::serialize
*
* @since 0.1
* @since 7.0
*
* @return string
*/
public function serialize() {
return serialize( [ $this->propertyId->getNumericId(), $this->dataValue ] );
return serialize( [ $this->propertyId->getSerialization(), $this->dataValue ] );
}

/**
Expand All @@ -64,8 +64,14 @@ public function serialize() {
* @param string $serialized
*/
public function unserialize( $serialized ) {
list( $numericId, $dataValue ) = unserialize( $serialized );
$this->__construct( $numericId, $dataValue );
list( $propertyId, $this->dataValue ) = unserialize( $serialized );

if ( is_string( $propertyId ) ) {
$this->propertyId = new PropertyId( $propertyId );
} else {
// Backwards compatibility with the previous serialization format
$this->propertyId = PropertyId::newFromNumber( $propertyId );
}
}

/**
Expand Down
11 changes: 8 additions & 3 deletions src/Snak/SnakObject.php
View file
Expand Up @@ -97,12 +97,12 @@ public function equals( $target ) {
/**
* @see Serializable::serialize
*
* @since 0.1
* @since 7.0
*
* @return string
*/
public function serialize() {
return serialize( $this->propertyId->getNumericId() );
return $this->propertyId->getSerialization();
}

/**
Expand All @@ -113,7 +113,12 @@ public function serialize() {
* @param string $serialized
*/
public function unserialize( $serialized ) {
$this->propertyId = PropertyId::newFromNumber( unserialize( $serialized ) );
try {
$this->propertyId = new PropertyId( $serialized );
} catch ( InvalidArgumentException $ex ) {
// Backwards compatibility with the previous serialization format
$this->propertyId = PropertyId::newFromNumber( unserialize( $serialized ) );
}
}

}
4 changes: 2 additions & 2 deletions tests/unit/ReferenceListTest.php
View file
Expand Up @@ -489,8 +489,8 @@ public function testSerializationStability() {
$list->addNewReference( new PropertyNoValueSnak( 1 ) );
$this->assertSame(
"a:1:{i:0;O:28:\"Wikibase\\DataModel\\Reference\":1:{s:35:\"\x00Wikibase\\DataModel\\"
. "Reference\x00snaks\";C:32:\"Wikibase\\DataModel\\Snak\\SnakList\":102:{a:2:{s:4:\""
. 'data";a:1:{i:0;C:43:"Wikibase\\DataModel\\Snak\\PropertyNoValueSnak":4:{i:1;}}s:5'
. "Reference\x00snaks\";C:32:\"Wikibase\\DataModel\\Snak\\SnakList\":100:{a:2:{s:4:\""
. 'data";a:1:{i:0;C:43:"Wikibase\\DataModel\\Snak\\PropertyNoValueSnak":2:{P1}}s:5'
. ':"index";i:0;}}}}',
$list->serialize()
);
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/Snak/DerivedPropertyValueSnakTest.php
View file
Expand Up @@ -89,7 +89,7 @@ public function testHashStability() {
$hash = $snak->getHash();

// @codingStandardsIgnoreStart
$expected = sha1( 'C:48:"Wikibase\DataModel\Snak\DerivedPropertyValueSnak":53:{a:2:{i:0;i:1;i:1;C:22:"DataValues\StringValue":1:{a}}}' );
$expected = sha1( 'C:48:"Wikibase\DataModel\Snak\DerivedPropertyValueSnak":58:{a:2:{i:0;s:2:"P1";i:1;C:22:"DataValues\StringValue":1:{a}}}' );
// @codingStandardsIgnoreEnd
$this->assertSame( $expected, $hash );
}
Expand Down Expand Up @@ -129,7 +129,7 @@ public function testSerializationDoesNotContainDerivedValues() {
);

$this->assertEquals(
'a:2:{i:0;i:9001;i:1;C:22:"DataValues\StringValue":2:{bc}}',
'a:2:{i:0;s:5:"P9001";i:1;C:22:"DataValues\StringValue":2:{bc}}',
$snak->serialize()
);
}
Expand Down
50 changes: 43 additions & 7 deletions tests/unit/Snak/PropertyNoValueSnakTest.php
View file
Expand Up @@ -76,7 +76,7 @@ public function testHashStability() {
$snak = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
$hash = $snak->getHash();

$expected = sha1( 'C:43:"Wikibase\DataModel\Snak\PropertyNoValueSnak":4:{i:1;}' );
$expected = sha1( 'C:43:"Wikibase\DataModel\Snak\PropertyNoValueSnak":2:{P1}' );
$this->assertSame( $expected, $hash );
}

Expand Down Expand Up @@ -110,15 +110,51 @@ public function notEqualsProvider() {
];
}

public function testSerialize() {
$snak = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
$this->assertSame( 'i:1;', $snak->serialize() );
public function provideDataToSerialize() {
$p2 = new PropertyId( 'P2' );
$p2foo = new PropertyId( 'foo:P2' );

return [
'string' => [
'P2',
new PropertyNoValueSnak( $p2 ),
],
'foreign' => [
'foo:P2',
new PropertyNoValueSnak( $p2foo ),
],
];
}

public function testUnserialize() {
/**
* @dataProvider provideDataToSerialize
*/
public function testSerialize( $expected, Snak $snak ) {
$serialized = $snak->serialize();
$this->assertSame( $expected, $serialized );

$snak2 = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
$snak2->unserialize( $serialized );
$this->assertTrue( $snak->equals( $snak2 ), 'round trip' );
}

public function provideDataToUnserialize() {
$p2 = new PropertyId( 'P2' );
$p2foo = new PropertyId( 'foo:P2' );

return [
'legacy' => [ new PropertyNoValueSnak( $p2 ), 'i:2;' ],
'current' => [ new PropertyNoValueSnak( $p2 ), 'P2' ],
'foreign' => [ new PropertyNoValueSnak( $p2foo ), 'foo:P2' ],
];
}

/**
* @dataProvider provideDataToUnserialize
*/
public function testUnserialize( $expected, $serialized ) {
$snak = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
$snak->unserialize( 'i:2;' );
$expected = new PropertyNoValueSnak( new PropertyId( 'P2' ) );
$snak->unserialize( $serialized );
$this->assertTrue( $snak->equals( $expected ) );
}

Expand Down
50 changes: 43 additions & 7 deletions tests/unit/Snak/PropertySomeValueSnakTest.php
View file
Expand Up @@ -76,7 +76,7 @@ public function testHashStability() {
$snak = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
$hash = $snak->getHash();

$expected = sha1( 'C:45:"Wikibase\DataModel\Snak\PropertySomeValueSnak":4:{i:1;}' );
$expected = sha1( 'C:45:"Wikibase\DataModel\Snak\PropertySomeValueSnak":2:{P1}' );
$this->assertSame( $expected, $hash );
}

Expand Down Expand Up @@ -110,15 +110,51 @@ public function notEqualsProvider() {
];
}

public function testSerialize() {
$snak = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
$this->assertSame( 'i:1;', $snak->serialize() );
public function provideDataToSerialize() {
$p2 = new PropertyId( 'P2' );
$p2foo = new PropertyId( 'foo:P2' );

return [
'string' => [
'P2',
new PropertySomeValueSnak( $p2 ),
],
'foreign' => [
'foo:P2',
new PropertySomeValueSnak( $p2foo ),
],
];
}

public function testUnserialize() {
/**
* @dataProvider provideDataToSerialize
*/
public function testSerialize( $expected, Snak $snak ) {
$serialized = $snak->serialize();
$this->assertSame( $expected, $serialized );

$snak2 = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
$snak2->unserialize( $serialized );
$this->assertTrue( $snak->equals( $snak2 ), 'round trip' );
}

public function provideDataToUnserialize() {
$p2 = new PropertyId( 'P2' );
$p2foo = new PropertyId( 'foo:P2' );

return [
'legacy' => [ new PropertySomeValueSnak( $p2 ), 'i:2;' ],
'current' => [ new PropertySomeValueSnak( $p2 ), 'P2' ],
'foreign' => [ new PropertySomeValueSnak( $p2foo ), 'foo:P2' ],
];
}

/**
* @dataProvider provideDataToUnserialize
*/
public function testUnserialize( $expected, $serialized ) {
$snak = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
$snak->unserialize( 'i:2;' );
$expected = new PropertySomeValueSnak( new PropertyId( 'P2' ) );
$snak->unserialize( $serialized );
$this->assertTrue( $snak->equals( $expected ) );
}

Expand Down
61 changes: 54 additions & 7 deletions tests/unit/Snak/PropertyValueSnakTest.php
View file
Expand Up @@ -88,7 +88,7 @@ public function testHashStability() {
$hash = $snak->getHash();

// @codingStandardsIgnoreStart
$expected = sha1( 'C:41:"Wikibase\DataModel\Snak\PropertyValueSnak":53:{a:2:{i:0;i:1;i:1;C:22:"DataValues\StringValue":1:{a}}}' );
$expected = sha1( 'C:41:"Wikibase\DataModel\Snak\PropertyValueSnak":58:{a:2:{i:0;s:2:"P1";i:1;C:22:"DataValues\StringValue":1:{a}}}' );
// @codingStandardsIgnoreEnd
$this->assertSame( $expected, $hash );
}
Expand Down Expand Up @@ -121,15 +121,62 @@ public function notEqualsProvider() {
];
}

public function testSerialize() {
$snak = new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'a' ) );
$this->assertSame( 'a:2:{i:0;i:1;i:1;C:22:"DataValues\StringValue":1:{a}}', $snak->serialize() );
public function provideDataToSerialize() {
$p2 = new PropertyId( 'P2' );
$p2foo = new PropertyId( 'foo:P2' );
$value = new StringValue( 'b' );

return [
'string' => [
'a:2:{i:0;s:2:"P2";i:1;C:22:"DataValues\StringValue":1:{b}}',
new PropertyValueSnak( $p2, $value ),
],
'foreign' => [
'a:2:{i:0;s:6:"foo:P2";i:1;C:22:"DataValues\StringValue":1:{b}}',
new PropertyValueSnak( $p2foo, $value ),
],
];
}

/**
* @dataProvider provideDataToSerialize
*/
public function testSerialize( $expected, Snak $snak ) {
$serialized = $snak->serialize();
$this->assertSame( $expected, $serialized );

$snak2 = new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'a' ) );
$snak2->unserialize( $serialized );
$this->assertTrue( $snak->equals( $snak2 ), 'round trip' );
}

public function testUnserialize() {
public function provideDataToUnserialize() {
$p2 = new PropertyId( 'P2' );
$p2foo = new PropertyId( 'foo:P2' );
$value = new StringValue( 'b' );

return [
'legacy' => [
new PropertyValueSnak( $p2, $value ),
'a:2:{i:0;i:2;i:1;C:22:"DataValues\StringValue":1:{b}}'
],
'current' => [
new PropertyValueSnak( $p2, $value ),
'a:2:{i:0;s:2:"P2";i:1;C:22:"DataValues\StringValue":1:{b}}'
],
'foreign' => [
new PropertyValueSnak( $p2foo, $value ),
'a:2:{i:0;s:6:"foo:P2";i:1;C:22:"DataValues\StringValue":1:{b}}'
],
];
}

/**
* @dataProvider provideDataToUnserialize
*/
public function testUnserialize( $expected, $serialized ) {
$snak = new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'a' ) );
$snak->unserialize( 'a:2:{i:0;i:2;i:1;C:22:"DataValues\StringValue":1:{b}}' );
$expected = new PropertyValueSnak( new PropertyId( 'P2' ), new StringValue( 'b' ) );
$snak->unserialize( $serialized );
$this->assertTrue( $snak->equals( $expected ) );
}

Expand Down

0 comments on commit 68667eb

Please sign in to comment.