Skip to content

Commit 68667eb

Browse files
Daniel Kinzlermanicki
Daniel Kinzler
authored and
manicki
committed
Don't use numeric IDs for internal PHP serialization. (#716)
Don't use numeric IDs for internal PHP serialization of Snaks. NOTE: it's unclear if this completely fixes T157442 Bug: T157442
1 parent 4220561 commit 68667eb

8 files changed

Lines changed: 162 additions & 33 deletions

‎phpcs.xml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
<rule ref="Generic.Arrays.DisallowLongArraySyntax" />
88

99
<rule ref="Generic.Classes" />
10-
<rule ref="Generic.CodeAnalysis" />
1110
<rule ref="Generic.ControlStructures" />
1211

1312
<rule ref="Generic.Files.ByteOrderMark" />

‎src/Snak/PropertyValueSnak.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,12 @@ public function getDataValue() {
4848
/**
4949
* @see Serializable::serialize
5050
*
51-
* @since 0.1
51+
* @since 7.0
5252
*
5353
* @return string
5454
*/
5555
public function serialize() {
56-
return serialize( [ $this->propertyId->getNumericId(), $this->dataValue ] );
56+
return serialize( [ $this->propertyId->getSerialization(), $this->dataValue ] );
5757
}
5858

5959
/**
@@ -64,8 +64,14 @@ public function serialize() {
6464
* @param string $serialized
6565
*/
6666
public function unserialize( $serialized ) {
67-
list( $numericId, $dataValue ) = unserialize( $serialized );
68-
$this->__construct( $numericId, $dataValue );
67+
list( $propertyId, $this->dataValue ) = unserialize( $serialized );
68+
69+
if ( is_string( $propertyId ) ) {
70+
$this->propertyId = new PropertyId( $propertyId );
71+
} else {
72+
// Backwards compatibility with the previous serialization format
73+
$this->propertyId = PropertyId::newFromNumber( $propertyId );
74+
}
6975
}
7076

7177
/**

‎src/Snak/SnakObject.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,12 @@ public function equals( $target ) {
9797
/**
9898
* @see Serializable::serialize
9999
*
100-
* @since 0.1
100+
* @since 7.0
101101
*
102102
* @return string
103103
*/
104104
public function serialize() {
105-
return serialize( $this->propertyId->getNumericId() );
105+
return $this->propertyId->getSerialization();
106106
}
107107

108108
/**
@@ -113,7 +113,12 @@ public function serialize() {
113113
* @param string $serialized
114114
*/
115115
public function unserialize( $serialized ) {
116-
$this->propertyId = PropertyId::newFromNumber( unserialize( $serialized ) );
116+
try {
117+
$this->propertyId = new PropertyId( $serialized );
118+
} catch ( InvalidArgumentException $ex ) {
119+
// Backwards compatibility with the previous serialization format
120+
$this->propertyId = PropertyId::newFromNumber( unserialize( $serialized ) );
121+
}
117122
}
118123

119124
}

‎tests/unit/ReferenceListTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -489,8 +489,8 @@ public function testSerializationStability() {
489489
$list->addNewReference( new PropertyNoValueSnak( 1 ) );
490490
$this->assertSame(
491491
"a:1:{i:0;O:28:\"Wikibase\\DataModel\\Reference\":1:{s:35:\"\x00Wikibase\\DataModel\\"
492-
. "Reference\x00snaks\";C:32:\"Wikibase\\DataModel\\Snak\\SnakList\":102:{a:2:{s:4:\""
493-
. 'data";a:1:{i:0;C:43:"Wikibase\\DataModel\\Snak\\PropertyNoValueSnak":4:{i:1;}}s:5'
492+
. "Reference\x00snaks\";C:32:\"Wikibase\\DataModel\\Snak\\SnakList\":100:{a:2:{s:4:\""
493+
. 'data";a:1:{i:0;C:43:"Wikibase\\DataModel\\Snak\\PropertyNoValueSnak":2:{P1}}s:5'
494494
. ':"index";i:0;}}}}',
495495
$list->serialize()
496496
);

‎tests/unit/Snak/DerivedPropertyValueSnakTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public function testHashStability() {
8989
$hash = $snak->getHash();
9090

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

131131
$this->assertEquals(
132-
'a:2:{i:0;i:9001;i:1;C:22:"DataValues\StringValue":2:{bc}}',
132+
'a:2:{i:0;s:5:"P9001";i:1;C:22:"DataValues\StringValue":2:{bc}}',
133133
$snak->serialize()
134134
);
135135
}

‎tests/unit/Snak/PropertyNoValueSnakTest.php‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public function testHashStability() {
7676
$snak = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
7777
$hash = $snak->getHash();
7878

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

@@ -110,15 +110,51 @@ public function notEqualsProvider() {
110110
];
111111
}
112112

113-
public function testSerialize() {
114-
$snak = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
115-
$this->assertSame( 'i:1;', $snak->serialize() );
113+
public function provideDataToSerialize() {
114+
$p2 = new PropertyId( 'P2' );
115+
$p2foo = new PropertyId( 'foo:P2' );
116+
117+
return [
118+
'string' => [
119+
'P2',
120+
new PropertyNoValueSnak( $p2 ),
121+
],
122+
'foreign' => [
123+
'foo:P2',
124+
new PropertyNoValueSnak( $p2foo ),
125+
],
126+
];
116127
}
117128

118-
public function testUnserialize() {
129+
/**
130+
* @dataProvider provideDataToSerialize
131+
*/
132+
public function testSerialize( $expected, Snak $snak ) {
133+
$serialized = $snak->serialize();
134+
$this->assertSame( $expected, $serialized );
135+
136+
$snak2 = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
137+
$snak2->unserialize( $serialized );
138+
$this->assertTrue( $snak->equals( $snak2 ), 'round trip' );
139+
}
140+
141+
public function provideDataToUnserialize() {
142+
$p2 = new PropertyId( 'P2' );
143+
$p2foo = new PropertyId( 'foo:P2' );
144+
145+
return [
146+
'legacy' => [ new PropertyNoValueSnak( $p2 ), 'i:2;' ],
147+
'current' => [ new PropertyNoValueSnak( $p2 ), 'P2' ],
148+
'foreign' => [ new PropertyNoValueSnak( $p2foo ), 'foo:P2' ],
149+
];
150+
}
151+
152+
/**
153+
* @dataProvider provideDataToUnserialize
154+
*/
155+
public function testUnserialize( $expected, $serialized ) {
119156
$snak = new PropertyNoValueSnak( new PropertyId( 'P1' ) );
120-
$snak->unserialize( 'i:2;' );
121-
$expected = new PropertyNoValueSnak( new PropertyId( 'P2' ) );
157+
$snak->unserialize( $serialized );
122158
$this->assertTrue( $snak->equals( $expected ) );
123159
}
124160

‎tests/unit/Snak/PropertySomeValueSnakTest.php‎

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public function testHashStability() {
7676
$snak = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
7777
$hash = $snak->getHash();
7878

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

@@ -110,15 +110,51 @@ public function notEqualsProvider() {
110110
];
111111
}
112112

113-
public function testSerialize() {
114-
$snak = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
115-
$this->assertSame( 'i:1;', $snak->serialize() );
113+
public function provideDataToSerialize() {
114+
$p2 = new PropertyId( 'P2' );
115+
$p2foo = new PropertyId( 'foo:P2' );
116+
117+
return [
118+
'string' => [
119+
'P2',
120+
new PropertySomeValueSnak( $p2 ),
121+
],
122+
'foreign' => [
123+
'foo:P2',
124+
new PropertySomeValueSnak( $p2foo ),
125+
],
126+
];
116127
}
117128

118-
public function testUnserialize() {
129+
/**
130+
* @dataProvider provideDataToSerialize
131+
*/
132+
public function testSerialize( $expected, Snak $snak ) {
133+
$serialized = $snak->serialize();
134+
$this->assertSame( $expected, $serialized );
135+
136+
$snak2 = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
137+
$snak2->unserialize( $serialized );
138+
$this->assertTrue( $snak->equals( $snak2 ), 'round trip' );
139+
}
140+
141+
public function provideDataToUnserialize() {
142+
$p2 = new PropertyId( 'P2' );
143+
$p2foo = new PropertyId( 'foo:P2' );
144+
145+
return [
146+
'legacy' => [ new PropertySomeValueSnak( $p2 ), 'i:2;' ],
147+
'current' => [ new PropertySomeValueSnak( $p2 ), 'P2' ],
148+
'foreign' => [ new PropertySomeValueSnak( $p2foo ), 'foo:P2' ],
149+
];
150+
}
151+
152+
/**
153+
* @dataProvider provideDataToUnserialize
154+
*/
155+
public function testUnserialize( $expected, $serialized ) {
119156
$snak = new PropertySomeValueSnak( new PropertyId( 'P1' ) );
120-
$snak->unserialize( 'i:2;' );
121-
$expected = new PropertySomeValueSnak( new PropertyId( 'P2' ) );
157+
$snak->unserialize( $serialized );
122158
$this->assertTrue( $snak->equals( $expected ) );
123159
}
124160

‎tests/unit/Snak/PropertyValueSnakTest.php‎

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public function testHashStability() {
8888
$hash = $snak->getHash();
8989

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

124-
public function testSerialize() {
125-
$snak = new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'a' ) );
126-
$this->assertSame( 'a:2:{i:0;i:1;i:1;C:22:"DataValues\StringValue":1:{a}}', $snak->serialize() );
124+
public function provideDataToSerialize() {
125+
$p2 = new PropertyId( 'P2' );
126+
$p2foo = new PropertyId( 'foo:P2' );
127+
$value = new StringValue( 'b' );
128+
129+
return [
130+
'string' => [
131+
'a:2:{i:0;s:2:"P2";i:1;C:22:"DataValues\StringValue":1:{b}}',
132+
new PropertyValueSnak( $p2, $value ),
133+
],
134+
'foreign' => [
135+
'a:2:{i:0;s:6:"foo:P2";i:1;C:22:"DataValues\StringValue":1:{b}}',
136+
new PropertyValueSnak( $p2foo, $value ),
137+
],
138+
];
139+
}
140+
141+
/**
142+
* @dataProvider provideDataToSerialize
143+
*/
144+
public function testSerialize( $expected, Snak $snak ) {
145+
$serialized = $snak->serialize();
146+
$this->assertSame( $expected, $serialized );
147+
148+
$snak2 = new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'a' ) );
149+
$snak2->unserialize( $serialized );
150+
$this->assertTrue( $snak->equals( $snak2 ), 'round trip' );
127151
}
128152

129-
public function testUnserialize() {
153+
public function provideDataToUnserialize() {
154+
$p2 = new PropertyId( 'P2' );
155+
$p2foo = new PropertyId( 'foo:P2' );
156+
$value = new StringValue( 'b' );
157+
158+
return [
159+
'legacy' => [
160+
new PropertyValueSnak( $p2, $value ),
161+
'a:2:{i:0;i:2;i:1;C:22:"DataValues\StringValue":1:{b}}'
162+
],
163+
'current' => [
164+
new PropertyValueSnak( $p2, $value ),
165+
'a:2:{i:0;s:2:"P2";i:1;C:22:"DataValues\StringValue":1:{b}}'
166+
],
167+
'foreign' => [
168+
new PropertyValueSnak( $p2foo, $value ),
169+
'a:2:{i:0;s:6:"foo:P2";i:1;C:22:"DataValues\StringValue":1:{b}}'
170+
],
171+
];
172+
}
173+
174+
/**
175+
* @dataProvider provideDataToUnserialize
176+
*/
177+
public function testUnserialize( $expected, $serialized ) {
130178
$snak = new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'a' ) );
131-
$snak->unserialize( 'a:2:{i:0;i:2;i:1;C:22:"DataValues\StringValue":1:{b}}' );
132-
$expected = new PropertyValueSnak( new PropertyId( 'P2' ), new StringValue( 'b' ) );
179+
$snak->unserialize( $serialized );
133180
$this->assertTrue( $snak->equals( $expected ) );
134181
}
135182

0 commit comments

Comments
 (0)