Fix field names and behavior in SlotRecord.
authordaniel <daniel.kinzler@wikimedia.de>
Tue, 6 Mar 2018 18:46:13 +0000 (19:46 +0100)
committerdaniel <daniel.kinzler@wikimedia.de>
Thu, 8 Mar 2018 11:53:25 +0000 (12:53 +0100)
The field names used in SlotRecord got out of sync with the changes made
to the database schema. Nobody noticed, because these fields are not yet
written to or read from the database.

This change also rewrites SlotRecordTest and adds several sanity checks
to SlotRecord, in preparation of the introduction of slot_origin.

Change-Id: Ifdf040174705bf88104b8c922c9d6d1120d48f3a

includes/Storage/RevisionStore.php
includes/Storage/SlotRecord.php
tests/phpunit/includes/Storage/MutableRevisionRecordTest.php
tests/phpunit/includes/Storage/RevisionStoreDbTest.php
tests/phpunit/includes/Storage/SlotRecordTest.php

index e00deef..98ad287 100644 (file)
@@ -455,7 +455,7 @@ class RevisionStore
                        $dbw->insert( 'ip_changes', $ipcRow, __METHOD__ );
                }
 
-               $newSlot = SlotRecord::newSaved( $row['rev_id'], $blobAddress, $slot );
+               $newSlot = SlotRecord::newSaved( $row['rev_id'], $textId, $blobAddress, $slot );
                $slots = new RevisionSlots( [ 'main' => $newSlot ] );
 
                $rev = new RevisionStoreRecord(
@@ -751,6 +751,10 @@ class RevisionStore
        private function emulateMainSlot_1_29( $row, $queryFlags, Title $title ) {
                $mainSlotRow = new stdClass();
                $mainSlotRow->role_name = 'main';
+               $mainSlotRow->model_name = null;
+               $mainSlotRow->slot_revision_id = null;
+               $mainSlotRow->content_address = null;
+               $mainSlotRow->slot_content_id = null;
 
                $content = null;
                $blobData = null;
@@ -763,7 +767,8 @@ class RevisionStore
                        }
 
                        if ( isset( $row->rev_text_id ) && $row->rev_text_id > 0 ) {
-                               $mainSlotRow->cont_address = 'tt:' . $row->rev_text_id;
+                               $mainSlotRow->slot_content_id = $row->rev_text_id;
+                               $mainSlotRow->content_address = 'tt:' . $row->rev_text_id;
                        }
 
                        if ( isset( $row->old_text ) ) {
@@ -776,10 +781,10 @@ class RevisionStore
                                $blobFlags = ( $row->old_flags === null ) ? '' : $row->old_flags;
                        }
 
-                       $mainSlotRow->slot_revision = intval( $row->rev_id );
+                       $mainSlotRow->slot_revision_id = intval( $row->rev_id );
 
-                       $mainSlotRow->cont_size = isset( $row->rev_len ) ? intval( $row->rev_len ) : null;
-                       $mainSlotRow->cont_sha1 = isset( $row->rev_sha1 ) ? strval( $row->rev_sha1 ) : null;
+                       $mainSlotRow->content_size = isset( $row->rev_len ) ? intval( $row->rev_len ) : null;
+                       $mainSlotRow->content_sha1 = isset( $row->rev_sha1 ) ? strval( $row->rev_sha1 ) : null;
                        $mainSlotRow->model_name = isset( $row->rev_content_model )
                                ? strval( $row->rev_content_model )
                                : null;
@@ -788,13 +793,16 @@ class RevisionStore
                                ? strval( $row->rev_content_format )
                                : null;
                } elseif ( is_array( $row ) ) {
-                       $mainSlotRow->slot_revision = isset( $row['id'] ) ? intval( $row['id'] ) : null;
+                       $mainSlotRow->slot_revision_id = isset( $row['id'] ) ? intval( $row['id'] ) : null;
 
-                       $mainSlotRow->cont_address = isset( $row['text_id'] )
+                       $mainSlotRow->slot_content_id = isset( $row['text_id'] )
+                               ? intval( $row['text_id'] )
+                               : null;
+                       $mainSlotRow->content_address = isset( $row['text_id'] )
                                ? 'tt:' . intval( $row['text_id'] )
                                : null;
-                       $mainSlotRow->cont_size = isset( $row['len'] ) ? intval( $row['len'] ) : null;
-                       $mainSlotRow->cont_sha1 = isset( $row['sha1'] ) ? strval( $row['sha1'] ) : null;
+                       $mainSlotRow->content_size = isset( $row['len'] ) ? intval( $row['len'] ) : null;
+                       $mainSlotRow->content_sha1 = isset( $row['sha1'] ) ? strval( $row['sha1'] ) : null;
 
                        $mainSlotRow->model_name = isset( $row['content_model'] )
                                ? strval( $row['content_model'] ) : null;  // XXX: must be a string!
@@ -853,6 +861,7 @@ class RevisionStore
                        };
                }
 
+               $mainSlotRow->slot_id = $mainSlotRow->slot_revision_id;
                return new SlotRecord( $mainSlotRow, $content );
        }
 
index 8769330..b59d92f 100644 (file)
@@ -23,6 +23,7 @@
 namespace MediaWiki\Storage;
 
 use Content;
+use InvalidArgumentException;
 use LogicException;
 use OutOfBoundsException;
 use Wikimedia\Assert\Assert;
@@ -72,7 +73,8 @@ class SlotRecord {
         * @return SlotRecord
         */
        private static function newDerived( SlotRecord $slot, array $overrides = [] ) {
-               $row = $slot->row;
+               $row = clone $slot->row;
+               $row->slot_id = null; // never copy the row ID!
 
                foreach ( $overrides as $key => $value ) {
                        $row->$key = $value;
@@ -85,6 +87,10 @@ class SlotRecord {
         * Constructs a new SlotRecord for a new revision, inheriting the content of the given SlotRecord
         * of a previous revision.
         *
+        * Note that a SlotRecord constructed this way are intended as prototypes,
+        * to be used wit newSaved(). They are incomplete, so some getters such as
+        * getRevision() will fail.
+        *
         * @param SlotRecord $slot
         *
         * @return SlotRecord
@@ -92,32 +98,35 @@ class SlotRecord {
        public static function newInherited( SlotRecord $slot ) {
                return self::newDerived( $slot, [
                        'slot_inherited' => true,
-                       'slot_revision' => null,
+                       'slot_revision_id' => null,
                ] );
        }
 
        /**
         * Constructs a new Slot from a Content object for a new revision.
         * This is the preferred way to construct a slot for storing Content that
-        * resulted from a user edit.
+        * resulted from a user edit. The slot is assumed to be not inherited.
+        *
+        * Note that a SlotRecord constructed this way are intended as prototypes,
+        * to be used wit newSaved(). They are incomplete, so some getters such as
+        * getAddress() will fail.
         *
         * @param string $role
         * @param Content $content
-        * @param bool $inherited
         *
-        * @return SlotRecord
+        * @return SlotRecord An incomplete proto-slot object, to be used with newSaved() later.
         */
-       public static function newUnsaved( $role, Content $content, $inherited = false ) {
-               Assert::parameterType( 'boolean', $inherited, '$inherited' );
+       public static function newUnsaved( $role, Content $content ) {
                Assert::parameterType( 'string', $role, '$role' );
 
                $row = [
                        'slot_id' => null, // not yet known
-                       'slot_address' => null, // not yet known. need setter?
-                       'slot_revision' => null, // not yet known
-                       'slot_inherited' => $inherited,
-                       'cont_size' => null, // compute later
-                       'cont_sha1' => null, // compute later
+                       'slot_revision_id' => null, // not yet known
+                       'slot_inherited' => 0, // not inherited
+                       'content_size' => null, // compute later
+                       'content_sha1' => null, // compute later
+                       'slot_content_id' => null, // not yet known, will be set in newSaved()
+                       'content_address' => null, // not yet known, will be set in newSaved()
                        'role_name' => $role,
                        'model_name' => $content->getModel(),
                ];
@@ -126,23 +135,64 @@ class SlotRecord {
        }
 
        /**
-        * Constructs a SlotRecord for a newly saved revision, based on the proto-slot that was
-        * supplied to the code that performed the save operation. This adds information that
-        * has only become available during saving, particularly the revision ID and blob address.
-        *
-        * @param int $revisionId
-        * @param string $blobAddress
-        * @param SlotRecord $protoSlot The proto-slot that was provided to the code that then
-        *
-        * @return SlotRecord
+        * Constructs a complete SlotRecord for a newly saved revision, based on the incomplete
+        * proto-slot. This adds information that has only become available during saving,
+        * particularly the revision ID and content address.
+        *
+        * @param int $revisionId the revision the slot is to be associated with (field slot_revision_id).
+        *        If $protoSlot already has a revision, it must be the same.
+        * @param int $contentId the ID of the row in the content table describing the content
+        *        referenced by $contentAddress (field slot_content_id).
+        *        If $protoSlot already has a content ID, it must be the same.
+        * @param string $contentAddress the slot's content address (field content_address).
+        *        If $protoSlot already has an address, it must be the same.
+        * @param SlotRecord $protoSlot The proto-slot that was provided as input for creating a new
+        *        revision. $protoSlot must have a content address if inherited.
+        *
+        * @return SlotRecord If the state of $protoSlot is inappropriate for saving a new revision.
         */
-       public static function newSaved( $revisionId, $blobAddress, SlotRecord $protoSlot ) {
+       public static function newSaved(
+               $revisionId,
+               $contentId,
+               $contentAddress,
+               SlotRecord $protoSlot
+       ) {
                Assert::parameterType( 'integer', $revisionId, '$revisionId' );
-               Assert::parameterType( 'string', $blobAddress, '$blobAddress' );
+               Assert::parameterType( 'integer', $contentId, '$contentId' );
+               Assert::parameterType( 'string', $contentAddress, '$contentAddress' );
+
+               if ( $protoSlot->hasRevision() && $protoSlot->getRevision() !== $revisionId ) {
+                       throw new LogicException(
+                               "Mismatching revision ID $revisionId: "
+                               . "The slot already belongs to revision {$protoSlot->getRevision()}. "
+                               . "Use SlotRecord::newInherited() to re-use content between revisions."
+                       );
+               }
+
+               if ( $protoSlot->hasAddress() && $protoSlot->getAddress() !== $contentAddress ) {
+                       throw new LogicException(
+                               "Mismatching blob address $contentAddress: "
+                               . "The slot already has content at {$protoSlot->getAddress()}."
+                       );
+               }
+
+               if ( $protoSlot->hasAddress() && $protoSlot->getContentId() !== $contentId ) {
+                       throw new LogicException(
+                               "Mismatching content ID $contentId: "
+                               . "The slot already has content row {$protoSlot->getContentId()} associated."
+                       );
+               }
+
+               if ( $protoSlot->isInherited() && !$protoSlot->hasAddress() ) {
+                       throw new InvalidArgumentException(
+                               "An inherited blob should have a content address!"
+                       );
+               }
 
                return self::newDerived( $protoSlot, [
-                       'slot_revision' => $revisionId,
-                       'cont_address' => $blobAddress,
+                       'slot_revision_id' => $revisionId,
+                       'slot_content_id' => $contentId,
+                       'content_address' => $contentAddress,
                ] );
        }
 
@@ -165,6 +215,37 @@ class SlotRecord {
                Assert::parameterType( 'object', $row, '$row' );
                Assert::parameterType( 'Content|callable', $content, '$content' );
 
+               Assert::parameter(
+                       property_exists( $row, 'slot_id' ),
+                       '$row->slot_id',
+                       'must exist'
+               );
+               Assert::parameter(
+                       property_exists( $row, 'slot_revision_id' ),
+                       '$row->slot_revision_id',
+                       'must exist'
+               );
+               Assert::parameter(
+                       property_exists( $row, 'slot_inherited' ),
+                       '$row->slot_inherited',
+                       'must exist'
+               );
+               Assert::parameter(
+                       property_exists( $row, 'slot_content_id' ),
+                       '$row->slot_content_id',
+                       'must exist'
+               );
+               Assert::parameter(
+                       property_exists( $row, 'content_address' ),
+                       '$row->content_address',
+                       'must exist'
+               );
+               Assert::parameter(
+                       property_exists( $row, 'model_name' ),
+                       '$row->model_name',
+                       'must exist'
+               );
+
                $this->row = $row;
                $this->content = $content;
        }
@@ -217,7 +298,8 @@ class SlotRecord {
         * @param string $name
         *
         * @throws OutOfBoundsException
-        * @return mixed Returns the field's value, or null if the field is NULL in the DB row.
+        * @throws IncompleteRevisionException
+        * @return mixed Returns the field's value, never null.
         */
        private function getField( $name ) {
                if ( !isset( $this->row->$name ) ) {
@@ -280,7 +362,7 @@ class SlotRecord {
         * @return int
         */
        public function getRevision() {
-               return $this->getIntField( 'slot_revision' );
+               return $this->getIntField( 'slot_revision_id' );
        }
 
        /**
@@ -300,7 +382,7 @@ class SlotRecord {
         * @return bool
         */
        public function hasAddress() {
-               return $this->hasField( 'cont_address' );
+               return $this->hasField( 'content_address' );
        }
 
        /**
@@ -311,7 +393,7 @@ class SlotRecord {
         * @return bool
         */
        public function hasRevision() {
-               return $this->hasField( 'slot_revision' );
+               return $this->hasField( 'slot_revision_id' );
        }
 
        /**
@@ -330,7 +412,18 @@ class SlotRecord {
         * @return string
         */
        public function getAddress() {
-               return $this->getStringField( 'cont_address' );
+               return $this->getStringField( 'content_address' );
+       }
+
+       /**
+        * Returns the ID of the content meta data row associated with the slot.
+        * This information should be irrelevant to application logic, it is here to allow
+        * the construction of a full row for the revision table.
+        *
+        * @return int
+        */
+       public function getContentId() {
+               return $this->getIntField( 'slot_content_id' );
        }
 
        /**
@@ -340,10 +433,10 @@ class SlotRecord {
         */
        public function getSize() {
                try {
-                       $size = $this->getIntField( 'cont_size' );
+                       $size = $this->getIntField( 'content_size' );
                } catch ( IncompleteRevisionException $ex ) {
                        $size = $this->getContent()->getSize();
-                       $this->setField( 'cont_size', $size );
+                       $this->setField( 'content_size', $size );
                }
 
                return $size;
@@ -356,7 +449,7 @@ class SlotRecord {
         */
        public function getSha1() {
                try {
-                       $sha1 = $this->getStringField( 'cont_sha1' );
+                       $sha1 = $this->getStringField( 'content_sha1' );
                } catch ( IncompleteRevisionException $ex ) {
                        $format = $this->hasField( 'format_name' )
                                ? $this->getStringField( 'format_name' )
@@ -364,7 +457,7 @@ class SlotRecord {
 
                        $data = $this->getContent()->serialize( $format );
                        $sha1 = self::base36Sha1( $data );
-                       $this->setField( 'cont_sha1', $sha1 );
+                       $this->setField( 'content_sha1', $sha1 );
                }
 
                return $sha1;
index 807099f..675201e 100644 (file)
@@ -68,8 +68,8 @@ class MutableRevisionRecordTest extends MediaWikiTestCase {
 
        public function testSimpleSetGetSlot() {
                $record = new MutableRevisionRecord( Title::newFromText( 'Foo' ) );
-               $slot = new SlotRecord(
-                       (object)[ 'role_name' => 'main' ],
+               $slot = SlotRecord::newUnsaved(
+                       'main',
                        new WikitextContent( 'x' )
                );
                $record->setSlot( $slot );
index e81f0af..c713e2c 100644 (file)
@@ -167,8 +167,8 @@ class RevisionStoreDbTest extends MediaWikiTestCase {
                $this->assertEquals( $r1->getWikiId(), $r2->getWikiId() );
                $this->assertEquals( $r1->isMinor(), $r2->isMinor() );
                foreach ( $r1->getSlotRoles() as $role ) {
-                       $this->assertEquals( $r1->getSlot( $role ), $r2->getSlot( $role ) );
-                       $this->assertEquals( $r1->getContent( $role ), $r2->getContent( $role ) );
+                       $this->assertSlotRecordsEqual( $r1->getSlot( $role ), $r2->getSlot( $role ) );
+                       $this->assertTrue( $r1->getContent( $role )->equals( $r2->getContent( $role ) ) );
                }
                foreach ( [
                        RevisionRecord::DELETED_TEXT,
@@ -180,6 +180,29 @@ class RevisionStoreDbTest extends MediaWikiTestCase {
                }
        }
 
+       private function assertSlotRecordsEqual( SlotRecord $s1, SlotRecord $s2 ) {
+               $this->assertSame( $s1->getRole(), $s2->getRole() );
+               $this->assertSame( $s1->getModel(), $s2->getModel() );
+               $this->assertSame( $s1->getFormat(), $s2->getFormat() );
+               $this->assertSame( $s1->getSha1(), $s2->getSha1() );
+               $this->assertSame( $s1->getSize(), $s2->getSize() );
+               $this->assertTrue( $s1->getContent()->equals( $s2->getContent() ) );
+
+               $s1->hasRevision() ? $this->assertSame( $s1->getRevision(), $s2->getRevision() ) : null;
+               $s1->hasAddress() ? $this->assertSame( $s1->hasAddress(), $s2->hasAddress() ) : null;
+       }
+
+       private function assertRevisionCompleteness( RevisionRecord $r ) {
+               foreach ( $r->getSlotRoles() as $role ) {
+                       $this->assertSlotCompleteness( $r, $r->getSlot( $role ) );
+               }
+       }
+
+       private function assertSlotCompleteness( RevisionRecord $r, SlotRecord $slot ) {
+               $this->assertTrue( $slot->hasAddress() );
+               $this->assertSame( $r->getId(), $slot->getRevision() );
+       }
+
        /**
         * @param mixed[] $details
         *
@@ -257,6 +280,7 @@ class RevisionStoreDbTest extends MediaWikiTestCase {
 
                $this->assertLinkTargetsEqual( $title, $return->getPageAsLinkTarget() );
                $this->assertRevisionRecordsEqual( $rev, $return );
+               $this->assertRevisionCompleteness( $return );
        }
 
        /**
index 27fcd0c..ef31315 100644 (file)
@@ -2,10 +2,12 @@
 
 namespace MediaWiki\Tests\Storage;
 
+use InvalidArgumentException;
+use LogicException;
+use MediaWiki\Storage\IncompleteRevisionException;
 use MediaWiki\Storage\SlotRecord;
+use MediaWiki\Storage\SuppressedDataException;
 use MediaWikiTestCase;
-use RuntimeException;
-use Wikimedia\Assert\ParameterTypeException;
 use WikitextContent;
 
 /**
@@ -13,52 +15,88 @@ use WikitextContent;
  */
 class SlotRecordTest extends MediaWikiTestCase {
 
-       public function provideAContent() {
-               yield [ new WikitextContent( 'A' ) ];
-               yield [
-                       function ( SlotRecord $slotRecord ) {
-                               if ( $slotRecord->getAddress() === 'tt:456' ) {
-                                       return new WikitextContent( 'A' );
-                               }
-                               throw new RuntimeException( 'Got Wrong SlotRecord for callback' );
-                       },
-               ];
-       }
-
-       /**
-        * @dataProvider provideAContent
-        */
-       public function testValidConstruction( $content ) {
-               $row = (object)[
-                       'cont_size' => '1',
-                       'cont_sha1' => 'someHash',
-                       'cont_address' => 'tt:456',
-                       'model_name' => 'aModelname',
-                       'slot_revision' => '2',
-                       'format_name' => 'someFormatName',
+       private function makeRow( $data = [] ) {
+               $data = $data + [
+                       'slot_id' => 1234,
+                       'slot_content_id' => 33,
+                       'content_size' => '5',
+                       'content_sha1' => 'someHash',
+                       'content_address' => 'tt:456',
+                       'model_name' => CONTENT_MODEL_WIKITEXT,
+                       'format_name' => CONTENT_FORMAT_WIKITEXT,
+                       'slot_revision_id' => '2',
+                       'slot_inherited' => '1',
                        'role_name' => 'myRole',
-                       'slot_inherited' => '99'
                ];
+               return (object)$data;
+       }
 
-               $record = new SlotRecord( $row, $content );
+       public function testCompleteConstruction() {
+               $row = $this->makeRow();
+               $record = new SlotRecord( $row, new WikitextContent( 'A' ) );
 
+               $this->assertTrue( $record->hasAddress() );
+               $this->assertTrue( $record->hasRevision() );
+               $this->assertTrue( $record->isInherited() );
                $this->assertSame( 'A', $record->getContent()->getNativeData() );
-               $this->assertSame( 1, $record->getSize() );
+               $this->assertSame( 5, $record->getSize() );
                $this->assertSame( 'someHash', $record->getSha1() );
-               $this->assertSame( 'aModelname', $record->getModel() );
+               $this->assertSame( CONTENT_MODEL_WIKITEXT, $record->getModel() );
                $this->assertSame( 2, $record->getRevision() );
                $this->assertSame( 'tt:456', $record->getAddress() );
-               $this->assertSame( 'someFormatName', $record->getFormat() );
+               $this->assertSame( 33, $record->getContentId() );
+               $this->assertSame( CONTENT_FORMAT_WIKITEXT, $record->getFormat() );
                $this->assertSame( 'myRole', $record->getRole() );
+       }
+
+       public function testConstructionDeferred() {
+               $row = $this->makeRow( [
+                       'content_size' => null, // to be computed
+                       'content_sha1' => null, // to be computed
+                       'format_name' => function () {
+                               return CONTENT_FORMAT_WIKITEXT;
+                       },
+                       'slot_inherited' => '0'
+               ] );
+
+               $content = function () {
+                       return new WikitextContent( 'A' );
+               };
+
+               $record = new SlotRecord( $row, $content );
+
                $this->assertTrue( $record->hasAddress() );
                $this->assertTrue( $record->hasRevision() );
-               $this->assertTrue( $record->isInherited() );
+               $this->assertFalse( $record->isInherited() );
+               $this->assertSame( 'A', $record->getContent()->getNativeData() );
+               $this->assertSame( 1, $record->getSize() );
+               $this->assertNotNull( $record->getSha1() );
+               $this->assertSame( CONTENT_MODEL_WIKITEXT, $record->getModel() );
+               $this->assertSame( 2, $record->getRevision() );
+               $this->assertSame( 'tt:456', $record->getAddress() );
+               $this->assertSame( 33, $record->getContentId() );
+               $this->assertSame( CONTENT_FORMAT_WIKITEXT, $record->getFormat() );
+               $this->assertSame( 'myRole', $record->getRole() );
+       }
+
+       public function testNewUnsaved() {
+               $record = SlotRecord::newUnsaved( 'myRole', new WikitextContent( 'A' ) );
+
+               $this->assertFalse( $record->hasAddress() );
+               $this->assertFalse( $record->hasRevision() );
+               $this->assertFalse( $record->isInherited() );
+               $this->assertSame( 'A', $record->getContent()->getNativeData() );
+               $this->assertSame( 1, $record->getSize() );
+               $this->assertNotNull( $record->getSha1() );
+               $this->assertSame( CONTENT_MODEL_WIKITEXT, $record->getModel() );
+               $this->assertSame( 'myRole', $record->getRole() );
        }
 
        public function provideInvalidConstruction() {
                yield 'both null' => [ null, null ];
                yield 'null row' => [ null, new WikitextContent( 'A' ) ];
-               yield 'array row' => [ null, new WikitextContent( 'A' ) ];
+               yield 'array row' => [ [], new WikitextContent( 'A' ) ];
+               yield 'empty row' => [ (object)[], new WikitextContent( 'A' ) ];
                yield 'null content' => [ (object)[], null ];
        }
 
@@ -66,25 +104,168 @@ class SlotRecordTest extends MediaWikiTestCase {
         * @dataProvider provideInvalidConstruction
         */
        public function testInvalidConstruction( $row, $content ) {
-               $this->setExpectedException( ParameterTypeException::class );
+               $this->setExpectedException( InvalidArgumentException::class );
                new SlotRecord( $row, $content );
        }
 
-       public function testHasAddress_false() {
-               $record = new SlotRecord( (object)[], new WikitextContent( 'A' ) );
-               $this->assertFalse( $record->hasAddress() );
+       public function testGetContentId_fails() {
+               $record = SlotRecord::newUnsaved( 'main', new WikitextContent( 'A' ) );
+               $this->setExpectedException( IncompleteRevisionException::class );
+
+               $record->getContentId();
        }
 
-       public function testHasRevision_false() {
-               $record = new SlotRecord( (object)[], new WikitextContent( 'A' ) );
-               $this->assertFalse( $record->hasRevision() );
+       public function testGetAddress_fails() {
+               $record = SlotRecord::newUnsaved( 'main', new WikitextContent( 'A' ) );
+               $this->setExpectedException( IncompleteRevisionException::class );
+
+               $record->getAddress();
        }
 
-       public function testInInherited_false() {
-               // TODO unskip me once fixed.
-               $this->markTestSkipped( 'Should probably return false, needs fixing?' );
-               $record = new SlotRecord( (object)[], new WikitextContent( 'A' ) );
-               $this->assertFalse( $record->isInherited() );
+       public function testGetRevision_fails() {
+               $record = SlotRecord::newUnsaved( 'main', new WikitextContent( 'A' ) );
+               $this->setExpectedException( IncompleteRevisionException::class );
+
+               $record->getRevision();
+       }
+
+       public function provideHashStability() {
+               yield [ '', 'phoiac9h4m842xq45sp7s6u21eteeq1' ];
+               yield [ 'Lorem ipsum', 'hcr5u40uxr81d3nx89nvwzclfz6r9c5' ];
+       }
+
+       /**
+        * @dataProvider provideHashStability
+        */
+       public function testHashStability( $text, $hash ) {
+               // Changing the output of the hash function will break things horribly!
+
+               $this->assertSame( $hash, SlotRecord::base36Sha1( $text ) );
+
+               $record = SlotRecord::newUnsaved( 'main', new WikitextContent( $text ) );
+               $this->assertSame( $hash, $record->getSha1() );
+       }
+
+       public function testNewWithSuppressedContent() {
+               $input = new SlotRecord( $this->makeRow(), new WikitextContent( 'A' ) );
+               $output = SlotRecord::newWithSuppressedContent( $input );
+
+               $this->setExpectedException( SuppressedDataException::class );
+               $output->getContent();
+       }
+
+       public function testNewInherited() {
+               $row = $this->makeRow( [ 'slot_revision_id' => 7, 'slot_inherited' => 0 ] );
+               $parent = new SlotRecord( $row, new WikitextContent( 'A' ) );
+
+               // This would happen while doing an edit, before saving revision meta-data.
+               $inherited = SlotRecord::newInherited( $parent );
+
+               $this->assertSame( $parent->getContentId(), $inherited->getContentId() );
+               $this->assertSame( $parent->getAddress(), $inherited->getAddress() );
+               $this->assertSame( $parent->getContent(), $inherited->getContent() );
+               $this->assertTrue( $inherited->isInherited() );
+               $this->assertFalse( $inherited->hasRevision() );
+
+               // make sure we didn't mess with the internal state of $parent
+               $this->assertFalse( $parent->isInherited() );
+               $this->assertSame( 7, $parent->getRevision() );
+
+               // This would happen while doing an edit, after saving the revision meta-data
+               // and content meta-data.
+               $saved = SlotRecord::newSaved(
+                       10,
+                       $inherited->getContentId(),
+                       $inherited->getAddress(),
+                       $inherited
+               );
+               $this->assertSame( $parent->getContentId(), $saved->getContentId() );
+               $this->assertSame( $parent->getAddress(), $saved->getAddress() );
+               $this->assertSame( $parent->getContent(), $saved->getContent() );
+               $this->assertTrue( $saved->isInherited() );
+               $this->assertTrue( $saved->hasRevision() );
+               $this->assertSame( 10, $saved->getRevision() );
+
+               // make sure we didn't mess with the internal state of $parent or $inherited
+               $this->assertSame( 7, $parent->getRevision() );
+               $this->assertFalse( $inherited->hasRevision() );
+       }
+
+       public function testNewSaved() {
+               // This would happen while doing an edit, before saving revision meta-data.
+               $unsaved = SlotRecord::newUnsaved( 'main', new WikitextContent( 'A' ) );
+
+               // This would happen while doing an edit, after saving the revision meta-data
+               // and content meta-data.
+               $saved = SlotRecord::newSaved( 10, 20, 'theNewAddress', $unsaved );
+               $this->assertFalse( $saved->isInherited() );
+               $this->assertTrue( $saved->hasRevision() );
+               $this->assertTrue( $saved->hasAddress() );
+               $this->assertSame( 'theNewAddress', $saved->getAddress() );
+               $this->assertSame( 20, $saved->getContentId() );
+               $this->assertSame( 'A', $saved->getContent()->getNativeData() );
+               $this->assertSame( 10, $saved->getRevision() );
+
+               // make sure we didn't mess with the internal state of $unsaved
+               $this->assertFalse( $unsaved->hasAddress() );
+               $this->assertFalse( $unsaved->hasRevision() );
+       }
+
+       public function provideNewSaved_LogicException() {
+               $freshRow = $this->makeRow( [
+                       'content_id' => 10,
+                       'content_address' => 'address:1',
+                       'slot_inherited' => 0,
+                       'slot_revision_id' => 1,
+               ] );
+
+               $freshSlot = new SlotRecord( $freshRow, new WikitextContent( 'A' ) );
+               yield 'mismatching address' => [ 1, 10, 'address:BAD', $freshSlot ];
+               yield 'mismatching revision' => [ 5, 10, 'address:1', $freshSlot ];
+               yield 'mismatching content ID' => [ 1, 17, 'address:1', $freshSlot ];
+
+               $inheritedRow = $this->makeRow( [
+                       'content_id' => null,
+                       'content_address' => null,
+                       'slot_inherited' => 1
+               ] );
+
+               $inheritedSlot = new SlotRecord( $inheritedRow, new WikitextContent( 'A' ) );
+               yield 'inherited, but no address' => [ 1, 10, 'address:2', $inheritedSlot ];
+       }
+
+       /**
+        * @dataProvider provideNewSaved_LogicException
+        */
+       public function testNewSaved_LogicException(
+               $revisionId,
+               $contentId,
+               $contentAddress,
+               SlotRecord $protoSlot
+       ) {
+               $this->setExpectedException( LogicException::class );
+               SlotRecord::newSaved( $revisionId, $contentId, $contentAddress, $protoSlot );
+       }
+
+       public function provideNewSaved_InvalidArgumentException() {
+               $unsaved = SlotRecord::newUnsaved( 'main', new WikitextContent( 'A' ) );
+
+               yield 'bad revision id' => [ 'xyzzy', 5, 'address', $unsaved ];
+               yield 'bad content id' => [ 7, 'xyzzy', 'address', $unsaved ];
+               yield 'bad content address' => [ 7, 5, 77, $unsaved ];
+       }
+
+       /**
+        * @dataProvider provideNewSaved_InvalidArgumentException
+        */
+       public function testNewSaved_InvalidArgumentException(
+               $revisionId,
+               $contentId,
+               $contentAddress,
+               SlotRecord $protoSlot
+       ) {
+               $this->setExpectedException( InvalidArgumentException::class );
+               SlotRecord::newSaved( $revisionId, $contentId, $contentAddress, $protoSlot );
        }
 
 }