Skip to content

Commit 2d419ed

Browse files
author
Daniel Kinzler
committed
Merge pull request #626 from wmde/deepCloning
Implement __clone to fully support deep cloning
2 parents 605a1da + 70fd226 commit 2d419ed

10 files changed

Lines changed: 191 additions & 4 deletions

File tree

‎RELEASE-NOTES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
## Version 5.1 (dev)
44

5+
* `Item::copy` and `Property::copy` do not clone immutable objects any more, saving time and memory
56
* Deprecated `FingerprintHolder` and `StatementListHolder`
67

78
## Version 5.0.2 (2016-02-23)

‎src/Entity/Entity.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ public function setAllAliases( array $aliasLists ) {
301301
}
302302

303303
/**
304-
* Returns a deep copy of the entity.
304+
* @see EntityDocument::copy
305305
*
306306
* @since 0.1
307307
*

‎src/Entity/EntityDocument.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,9 @@ public function equals( $target );
7070

7171
/**
7272
* Returns a deep clone of the entity. The clone must be equal in all details, including the id.
73+
* No change done to the clone is allowed to interfere with the original object. Only properties
74+
* containing immutable objects are allowed to (and should) reference the original object.
75+
*
7376
* Since EntityDocuments are not immutable (at least the id can be set) the method is not
7477
* allowed to return $this.
7578
*

‎src/Entity/Item.php‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,19 @@ public function equals( $target ) {
327327
* @return self
328328
*/
329329
public function copy() {
330-
return unserialize( serialize( $this ) );
330+
return clone $this;
331+
}
332+
333+
/**
334+
* @see http://php.net/manual/en/language.oop5.cloning.php
335+
*
336+
* @since 5.1
337+
*/
338+
public function __clone() {
339+
$this->fingerprint = clone $this->fingerprint;
340+
// SiteLinkList is mutable, but SiteLink is not. No deeper cloning necesarry.
341+
$this->siteLinks = clone $this->siteLinks;
342+
$this->statements = clone $this->statements;
331343
}
332344

333345
}

‎src/Entity/Property.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,17 @@ public function setStatements( StatementList $statements ) {
259259
* @return self
260260
*/
261261
public function copy() {
262-
return unserialize( serialize( $this ) );
262+
return clone $this;
263+
}
264+
265+
/**
266+
* @see http://php.net/manual/en/language.oop5.cloning.php
267+
*
268+
* @since 5.1
269+
*/
270+
public function __clone() {
271+
$this->fingerprint = clone $this->fingerprint;
272+
$this->statements = clone $this->statements;
263273
}
264274

265275
}

‎src/Statement/Statement.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,4 +298,14 @@ public function equals( $target ) {
298298
&& $this->references->equals( $target->references );
299299
}
300300

301+
/**
302+
* @see http://php.net/manual/en/language.oop5.cloning.php
303+
*
304+
* @since 5.1
305+
*/
306+
public function __clone() {
307+
$this->qualifiers = clone $this->qualifiers;
308+
$this->references = clone $this->references;
309+
}
310+
301311
}

‎src/Statement/StatementList.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,4 +325,15 @@ public function filter( StatementFilter $filter ) {
325325
return $statementList;
326326
}
327327

328+
/**
329+
* @see http://php.net/manual/en/language.oop5.cloning.php
330+
*
331+
* @since 5.1
332+
*/
333+
public function __clone() {
334+
foreach ( $this->statements as &$statement ) {
335+
$statement = clone $statement;
336+
}
337+
}
338+
328339
}

‎src/Term/Fingerprint.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,4 +278,17 @@ public function setAliasGroups( AliasGroupList $groups ) {
278278
$this->aliasGroups = $groups;
279279
}
280280

281+
/**
282+
* @see http://php.net/manual/en/language.oop5.cloning.php
283+
*
284+
* @since 5.1
285+
*/
286+
public function __clone() {
287+
// TermList is mutable, but Term is not. No deeper cloning necesarry.
288+
$this->labels = clone $this->labels;
289+
$this->descriptions = clone $this->descriptions;
290+
// AliasGroupList is mutable, but AliasGroup is not. No deeper cloning necesarry.
291+
$this->aliasGroups = clone $this->aliasGroups;
292+
}
293+
281294
}

‎tests/unit/Entity/ItemTest.php‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@
44

55
use Wikibase\DataModel\Entity\Item;
66
use Wikibase\DataModel\Entity\ItemId;
7-
use Wikibase\DataModel\Entity\PropertyId;
87
use Wikibase\DataModel\SiteLink;
98
use Wikibase\DataModel\Snak\PropertyNoValueSnak;
9+
use Wikibase\DataModel\Snak\PropertySomeValueSnak;
1010
use Wikibase\DataModel\Statement\Statement;
1111
use Wikibase\DataModel\Statement\StatementList;
1212

@@ -368,4 +368,71 @@ public function testNotEquals( Item $firstItem, Item $secondItem ) {
368368
$this->assertFalse( $secondItem->equals( $firstItem ) );
369369
}
370370

371+
public function cloneProvider() {
372+
$item = new Item( new ItemId( 'Q1' ) );
373+
$item->setLabel( 'en', 'original' );
374+
$item->getStatements()->addNewStatement( new PropertyNoValueSnak( 1 ) );
375+
$item->getSiteLinkList()->addNewSiteLink( 'enwiki', 'Original' );
376+
377+
return array(
378+
'copy' => array( $item, $item->copy() ),
379+
'native clone' => array( $item, clone $item ),
380+
);
381+
}
382+
383+
/**
384+
* @dataProvider cloneProvider
385+
*/
386+
public function testCloneIsEqualButNotIdentical( Item $original, Item $clone ) {
387+
$this->assertNotSame( $original, $clone );
388+
$this->assertTrue( $original->equals( $clone ) );
389+
$this->assertSame(
390+
$original->getId(),
391+
$clone->getId(),
392+
'id is immutable and must not be cloned'
393+
);
394+
395+
// The clone must not reference the same mutable objects
396+
$this->assertNotSame( $original->getFingerprint(), $clone->getFingerprint() );
397+
$this->assertNotSame( $original->getStatements(), $clone->getStatements() );
398+
$this->assertNotSame(
399+
$original->getStatements()->getFirstStatementWithGuid( null ),
400+
$clone->getStatements()->getFirstStatementWithGuid( null )
401+
);
402+
$this->assertNotSame( $original->getSiteLinkList(), $clone->getSiteLinkList() );
403+
$this->assertSame(
404+
$original->getSiteLinkList()->getBySiteId( 'enwiki' ),
405+
$clone->getSiteLinkList()->getBySiteId( 'enwiki' ),
406+
'SiteLink is immutable and must not be cloned'
407+
);
408+
}
409+
410+
/**
411+
* @dataProvider cloneProvider
412+
*/
413+
public function testOriginalDoesNotChangeWithClone( Item $original, Item $clone ) {
414+
$originalStatement = $original->getStatements()->getFirstStatementWithGuid( null );
415+
$clonedStatement = $clone->getStatements()->getFirstStatementWithGuid( null );
416+
417+
$clone->setLabel( 'en', 'clone' );
418+
$clone->setDescription( 'en', 'clone' );
419+
$clone->setAliases( 'en', array( 'clone' ) );
420+
$clonedStatement->setGuid( 'clone' );
421+
$clonedStatement->setMainSnak( new PropertySomeValueSnak( 666 ) );
422+
$clonedStatement->setRank( Statement::RANK_DEPRECATED );
423+
$clonedStatement->getQualifiers()->addSnak( new PropertyNoValueSnak( 1 ) );
424+
$clonedStatement->getReferences()->addNewReference( new PropertyNoValueSnak( 1 ) );
425+
$clone->getSiteLinkList()->removeLinkWithSiteId( 'enwiki' );
426+
427+
$this->assertSame( 'original', $original->getFingerprint()->getLabel( 'en' )->getText() );
428+
$this->assertFalse( $original->getFingerprint()->hasDescription( 'en' ) );
429+
$this->assertFalse( $original->getFingerprint()->hasAliasGroup( 'en' ) );
430+
$this->assertNull( $originalStatement->getGuid() );
431+
$this->assertSame( 'novalue', $originalStatement->getMainSnak()->getType() );
432+
$this->assertSame( Statement::RANK_NORMAL, $originalStatement->getRank() );
433+
$this->assertTrue( $originalStatement->getQualifiers()->isEmpty() );
434+
$this->assertTrue( $originalStatement->getReferences()->isEmpty() );
435+
$this->assertFalse( $original->getSiteLinkList()->isEmpty() );
436+
}
437+
371438
}

‎tests/unit/Entity/PropertyTest.php‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
use Wikibase\DataModel\Entity\Property;
77
use Wikibase\DataModel\Entity\PropertyId;
88
use Wikibase\DataModel\Snak\PropertyNoValueSnak;
9+
use Wikibase\DataModel\Snak\PropertySomeValueSnak;
10+
use Wikibase\DataModel\Statement\Statement;
911
use Wikibase\DataModel\Statement\StatementList;
1012
use Wikibase\DataModel\Term\Fingerprint;
1113

@@ -229,4 +231,62 @@ public function testPropertyWithStatementsIsNotEmpty() {
229231
$this->assertFalse( $property->isEmpty() );
230232
}
231233

234+
public function cloneProvider() {
235+
$property = new Property( new PropertyId( 'P1' ), null, 'string' );
236+
$property->setLabel( 'en', 'original' );
237+
$property->getStatements()->addNewStatement( new PropertyNoValueSnak( 1 ) );
238+
239+
return array(
240+
'copy' => array( $property, $property->copy() ),
241+
'native clone' => array( $property, clone $property ),
242+
);
243+
}
244+
245+
/**
246+
* @dataProvider cloneProvider
247+
*/
248+
public function testCloneIsEqualButNotIdentical( Property $original, Property $clone ) {
249+
$this->assertNotSame( $original, $clone );
250+
$this->assertTrue( $original->equals( $clone ) );
251+
$this->assertSame(
252+
$original->getId(),
253+
$clone->getId(),
254+
'id is immutable and must not be cloned'
255+
);
256+
257+
// The clone must not reference the same mutable objects
258+
$this->assertNotSame( $original->getFingerprint(), $clone->getFingerprint() );
259+
$this->assertNotSame( $original->getStatements(), $clone->getStatements() );
260+
$this->assertNotSame(
261+
$original->getStatements()->getFirstStatementWithGuid( null ),
262+
$clone->getStatements()->getFirstStatementWithGuid( null )
263+
);
264+
}
265+
266+
/**
267+
* @dataProvider cloneProvider
268+
*/
269+
public function testOriginalDoesNotChangeWithClone( Property $original, Property $clone ) {
270+
$originalStatement = $original->getStatements()->getFirstStatementWithGuid( null );
271+
$clonedStatement = $clone->getStatements()->getFirstStatementWithGuid( null );
272+
273+
$clone->setLabel( 'en', 'clone' );
274+
$clone->setDescription( 'en', 'clone' );
275+
$clone->setAliases( 'en', array( 'clone' ) );
276+
$clonedStatement->setGuid( 'clone' );
277+
$clonedStatement->setMainSnak( new PropertySomeValueSnak( 666 ) );
278+
$clonedStatement->setRank( Statement::RANK_DEPRECATED );
279+
$clonedStatement->getQualifiers()->addSnak( new PropertyNoValueSnak( 1 ) );
280+
$clonedStatement->getReferences()->addNewReference( new PropertyNoValueSnak( 1 ) );
281+
282+
$this->assertSame( 'original', $original->getFingerprint()->getLabel( 'en' )->getText() );
283+
$this->assertFalse( $original->getFingerprint()->hasDescription( 'en' ) );
284+
$this->assertFalse( $original->getFingerprint()->hasAliasGroup( 'en' ) );
285+
$this->assertNull( $originalStatement->getGuid() );
286+
$this->assertSame( 'novalue', $originalStatement->getMainSnak()->getType() );
287+
$this->assertSame( Statement::RANK_NORMAL, $originalStatement->getRank() );
288+
$this->assertTrue( $originalStatement->getQualifiers()->isEmpty() );
289+
$this->assertTrue( $originalStatement->getReferences()->isEmpty() );
290+
}
291+
232292
}

0 commit comments

Comments
 (0)