Revision split __construct method

This makes the logic much easier to follow and each type of
construction easier to see.
This also emphisises the testing of construction being split
into row object vs array

Change-Id: Ie3aa6ec4c026f0249ccd438903fec27fcd266b67

diff --git a/includes/Revision.php b/includes/Revision.php
index 8855441..dd3ee78 100644 (file)
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -571,168 +571,184 @@ class Revision implements IDBAccessObject {
         * @throws MWException
         * @access private
         */
-       function __construct( $row ) {
+       public function __construct( $row ) {
                if ( is_object( $row ) ) {
-                       $this->mId = intval( $row->rev_id );
-                       $this->mPage = intval( $row->rev_page );
-                       $this->mTextId = intval( $row->rev_text_id );
-                       $this->mComment = CommentStore::newKey( 'rev_comment' )
-                               // Legacy because $row probably came from self::selectFields()
-                               ->getCommentLegacy( wfGetDB( DB_REPLICA ), $row, true )->text;
-                       $this->mUser = intval( $row->rev_user );
-                       $this->mMinorEdit = intval( $row->rev_minor_edit );
-                       $this->mTimestamp = $row->rev_timestamp;
-                       $this->mDeleted = intval( $row->rev_deleted );
-
-                       if ( !isset( $row->rev_parent_id ) ) {
-                               $this->mParentId = null;
-                       } else {
-                               $this->mParentId = intval( $row->rev_parent_id );
-                       }
-
-                       if ( !isset( $row->rev_len ) ) {
-                               $this->mSize = null;
-                       } else {
-                               $this->mSize = intval( $row->rev_len );
-                       }
-
-                       if ( !isset( $row->rev_sha1 ) ) {
-                               $this->mSha1 = null;
-                       } else {
-                               $this->mSha1 = $row->rev_sha1;
-                       }
-
-                       if ( isset( $row->page_latest ) ) {
-                               $this->mCurrent = ( $row->rev_id == $row->page_latest );
-                               $this->mTitle = Title::newFromRow( $row );
-                       } else {
-                               $this->mCurrent = false;
-                               $this->mTitle = null;
-                       }
-
-                       if ( !isset( $row->rev_content_model ) ) {
-                               $this->mContentModel = null; # determine on demand if needed
-                       } else {
-                               $this->mContentModel = strval( $row->rev_content_model );
-                       }
+                       $this->constructFromDbRowObject( $row );
+               } elseif ( is_array( $row ) ) {
+                       $this->constructFromRowArray( $row );
+               } else {
+                       throw new MWException( 'Revision constructor passed invalid row format.' );
+               }
+               $this->mUnpatrolled = null;
+       }
 
-                       if ( !isset( $row->rev_content_format ) ) {
-                               $this->mContentFormat = null; # determine on demand if needed
-                       } else {
-                               $this->mContentFormat = strval( $row->rev_content_format );
-                       }
+       /**
+        * @param object $row
+        */
+       private function constructFromDbRowObject( $row ) {
+               $this->mId = intval( $row->rev_id );
+               $this->mPage = intval( $row->rev_page );
+               $this->mTextId = intval( $row->rev_text_id );
+               $this->mComment = CommentStore::newKey( 'rev_comment' )
+                       // Legacy because $row probably came from self::selectFields()
+                       ->getCommentLegacy( wfGetDB( DB_REPLICA ), $row, true )->text;
+               $this->mUser = intval( $row->rev_user );
+               $this->mMinorEdit = intval( $row->rev_minor_edit );
+               $this->mTimestamp = $row->rev_timestamp;
+               $this->mDeleted = intval( $row->rev_deleted );
+
+               if ( !isset( $row->rev_parent_id ) ) {
+                       $this->mParentId = null;
+               } else {
+                       $this->mParentId = intval( $row->rev_parent_id );
+               }
 
-                       // Lazy extraction...
-                       $this->mText = null;
-                       if ( isset( $row->old_text ) ) {
-                               $this->mTextRow = $row;
-                       } else {
-                               // 'text' table row entry will be lazy-loaded
-                               $this->mTextRow = null;
-                       }
+               if ( !isset( $row->rev_len ) ) {
+                       $this->mSize = null;
+               } else {
+                       $this->mSize = intval( $row->rev_len );
+               }
 
-                       // Use user_name for users and rev_user_text for IPs...
-                       $this->mUserText = null; // lazy load if left null
-                       if ( $this->mUser == 0 ) {
-                               $this->mUserText = $row->rev_user_text; // IP user
-                       } elseif ( isset( $row->user_name ) ) {
-                               $this->mUserText = $row->user_name; // logged-in user
-                       }
-                       $this->mOrigUserText = $row->rev_user_text;
-               } elseif ( is_array( $row ) ) {
-                       // Build a new revision to be saved...
-                       global $wgUser; // ugh
+               if ( !isset( $row->rev_sha1 ) ) {
+                       $this->mSha1 = null;
+               } else {
+                       $this->mSha1 = $row->rev_sha1;
+               }
 
-                       # if we have a content object, use it to set the model and type
-                       if ( !empty( $row['content'] ) ) {
-                               if ( !( $row['content'] instanceof Content ) ) {
-                                       throw new MWException( '`content` field must contain a Content object.' );
-                               }
+               if ( isset( $row->page_latest ) ) {
+                       $this->mCurrent = ( $row->rev_id == $row->page_latest );
+                       $this->mTitle = Title::newFromRow( $row );
+               } else {
+                       $this->mCurrent = false;
+                       $this->mTitle = null;
+               }
 
-                               // @todo when is that set? test with external store setup! check out insertOn() [dk]
-                               if ( !empty( $row['text_id'] ) ) {
-                                       throw new MWException( "Text already stored in external store (id {$row['text_id']}), " .
-                                               "can't serialize content object" );
-                               }
+               if ( !isset( $row->rev_content_model ) ) {
+                       $this->mContentModel = null; # determine on demand if needed
+               } else {
+                       $this->mContentModel = strval( $row->rev_content_model );
+               }
 
-                               $row['content_model'] = $row['content']->getModel();
-                               # note: mContentFormat is initializes later accordingly
-                               # note: content is serialized later in this method!
-                               # also set text to null?
-                       }
+               if ( !isset( $row->rev_content_format ) ) {
+                       $this->mContentFormat = null; # determine on demand if needed
+               } else {
+                       $this->mContentFormat = strval( $row->rev_content_format );
+               }
 
-                       $this->mId = isset( $row['id'] ) ? intval( $row['id'] ) : null;
-                       $this->mPage = isset( $row['page'] ) ? intval( $row['page'] ) : null;
-                       $this->mTextId = isset( $row['text_id'] ) ? intval( $row['text_id'] ) : null;
-                       $this->mUserText = isset( $row['user_text'] )
-                               ? strval( $row['user_text'] ) : $wgUser->getName();
-                       $this->mUser = isset( $row['user'] ) ? intval( $row['user'] ) : $wgUser->getId();
-                       $this->mMinorEdit = isset( $row['minor_edit'] ) ? intval( $row['minor_edit'] ) : 0;
-                       $this->mTimestamp = isset( $row['timestamp'] )
-                               ? strval( $row['timestamp'] ) : wfTimestampNow();
-                       $this->mDeleted = isset( $row['deleted'] ) ? intval( $row['deleted'] ) : 0;
-                       $this->mSize = isset( $row['len'] ) ? intval( $row['len'] ) : null;
-                       $this->mParentId = isset( $row['parent_id'] ) ? intval( $row['parent_id'] ) : null;
-                       $this->mSha1 = isset( $row['sha1'] ) ? strval( $row['sha1'] ) : null;
-
-                       $this->mContentModel = isset( $row['content_model'] )
-                               ? strval( $row['content_model'] ) : null;
-                       $this->mContentFormat = isset( $row['content_format'] )
-                               ? strval( $row['content_format'] ) : null;
-
-                       // Enforce spacing trimming on supplied text
-                       $this->mComment = isset( $row['comment'] ) ? trim( strval( $row['comment'] ) ) : null;
-                       $this->mText = isset( $row['text'] ) ? rtrim( strval( $row['text'] ) ) : null;
+               // Lazy extraction...
+               $this->mText = null;
+               if ( isset( $row->old_text ) ) {
+                       $this->mTextRow = $row;
+               } else {
+                       // 'text' table row entry will be lazy-loaded
                        $this->mTextRow = null;
+               }
 
-                       $this->mTitle = isset( $row['title'] ) ? $row['title'] : null;
-
-                       // if we have a Content object, override mText and mContentModel
-                       if ( !empty( $row['content'] ) ) {
-                               $handler = $this->getContentHandler();
-                               $this->mContent = $row['content'];
+               // Use user_name for users and rev_user_text for IPs...
+               $this->mUserText = null; // lazy load if left null
+               if ( $this->mUser == 0 ) {
+                       $this->mUserText = $row->rev_user_text; // IP user
+               } elseif ( isset( $row->user_name ) ) {
+                       $this->mUserText = $row->user_name; // logged-in user
+               }
+               $this->mOrigUserText = $row->rev_user_text;
+       }
 
-                               $this->mContentModel = $this->mContent->getModel();
-                               $this->mContentHandler = null;
+       /**
+        * @param array $row
+        *
+        * @throws MWException
+        */
+       private function constructFromRowArray( array $row ) {
+               // Build a new revision to be saved...
+               global $wgUser; // ugh
 
-                               $this->mText = $handler->serializeContent( $row['content'], $this->getContentFormat() );
-                       } elseif ( $this->mText !== null ) {
-                               $handler = $this->getContentHandler();
-                               $this->mContent = $handler->unserializeContent( $this->mText );
+               # if we have a content object, use it to set the model and type
+               if ( !empty( $row['content'] ) ) {
+                       if ( !( $row['content'] instanceof Content ) ) {
+                               throw new MWException( '`content` field must contain a Content object.' );
                        }
 
-                       // If we have a Title object, make sure it is consistent with mPage.
-                       if ( $this->mTitle && $this->mTitle->exists() ) {
-                               if ( $this->mPage === null ) {
-                                       // if the page ID wasn't known, set it now
-                                       $this->mPage = $this->mTitle->getArticleID();
-                               } elseif ( $this->mTitle->getArticleID() !== $this->mPage ) {
-                                       // Got different page IDs. This may be legit (e.g. during undeletion),
-                                       // but it seems worth mentioning it in the log.
-                                       wfDebug( "Page ID " . $this->mPage . " mismatches the ID " .
-                                               $this->mTitle->getArticleID() . " provided by the Title object." );
-                               }
+                       // @todo when is that set? test with external store setup! check out insertOn() [dk]
+                       if ( !empty( $row['text_id'] ) ) {
+                               throw new MWException( "Text already stored in external store (id {$row['text_id']}), " .
+                                       "can't serialize content object" );
                        }
 
-                       $this->mCurrent = false;
+                       $row['content_model'] = $row['content']->getModel();
+                       # note: mContentFormat is initializes later accordingly
+                       # note: content is serialized later in this method!
+                       # also set text to null?
+               }
+
+               $this->mId = isset( $row['id'] ) ? intval( $row['id'] ) : null;
+               $this->mPage = isset( $row['page'] ) ? intval( $row['page'] ) : null;
+               $this->mTextId = isset( $row['text_id'] ) ? intval( $row['text_id'] ) : null;
+               $this->mUserText = isset( $row['user_text'] )
+                       ? strval( $row['user_text'] ) : $wgUser->getName();
+               $this->mUser = isset( $row['user'] ) ? intval( $row['user'] ) : $wgUser->getId();
+               $this->mMinorEdit = isset( $row['minor_edit'] ) ? intval( $row['minor_edit'] ) : 0;
+               $this->mTimestamp = isset( $row['timestamp'] )
+                       ? strval( $row['timestamp'] ) : wfTimestampNow();
+               $this->mDeleted = isset( $row['deleted'] ) ? intval( $row['deleted'] ) : 0;
+               $this->mSize = isset( $row['len'] ) ? intval( $row['len'] ) : null;
+               $this->mParentId = isset( $row['parent_id'] ) ? intval( $row['parent_id'] ) : null;
+               $this->mSha1 = isset( $row['sha1'] ) ? strval( $row['sha1'] ) : null;
+
+               $this->mContentModel = isset( $row['content_model'] )
+                       ? strval( $row['content_model'] ) : null;
+               $this->mContentFormat = isset( $row['content_format'] )
+                       ? strval( $row['content_format'] ) : null;
+
+               // Enforce spacing trimming on supplied text
+               $this->mComment = isset( $row['comment'] ) ? trim( strval( $row['comment'] ) ) : null;
+               $this->mText = isset( $row['text'] ) ? rtrim( strval( $row['text'] ) ) : null;
+               $this->mTextRow = null;
+
+               $this->mTitle = isset( $row['title'] ) ? $row['title'] : null;
+
+               // if we have a Content object, override mText and mContentModel
+               if ( !empty( $row['content'] ) ) {
+                       $handler = $this->getContentHandler();
+                       $this->mContent = $row['content'];
 
-                       // If we still have no length, see it we have the text to figure it out
-                       if ( !$this->mSize && $this->mContent !== null ) {
-                               $this->mSize = $this->mContent->getSize();
-                       }
+                       $this->mContentModel = $this->mContent->getModel();
+                       $this->mContentHandler = null;
 
-                       // Same for sha1
-                       if ( $this->mSha1 === null ) {
-                               $this->mSha1 = $this->mText === null ? null : self::base36Sha1( $this->mText );
+                       $this->mText = $handler->serializeContent( $row['content'], $this->getContentFormat() );
+               } elseif ( $this->mText !== null ) {
+                       $handler = $this->getContentHandler();
+                       $this->mContent = $handler->unserializeContent( $this->mText );
+               }
+
+               // If we have a Title object, make sure it is consistent with mPage.
+               if ( $this->mTitle && $this->mTitle->exists() ) {
+                       if ( $this->mPage === null ) {
+                               // if the page ID wasn't known, set it now
+                               $this->mPage = $this->mTitle->getArticleID();
+                       } elseif ( $this->mTitle->getArticleID() !== $this->mPage ) {
+                               // Got different page IDs. This may be legit (e.g. during undeletion),
+                               // but it seems worth mentioning it in the log.
+                               wfDebug( "Page ID " . $this->mPage . " mismatches the ID " .
+                                       $this->mTitle->getArticleID() . " provided by the Title object." );
                        }
+               }
 
-                       // force lazy init
-                       $this->getContentModel();
-                       $this->getContentFormat();
-               } else {
-                       throw new MWException( 'Revision constructor passed invalid row format.' );
+               $this->mCurrent = false;
+
+               // If we still have no length, see it we have the text to figure it out
+               if ( !$this->mSize && $this->mContent !== null ) {
+                       $this->mSize = $this->mContent->getSize();
                }
-               $this->mUnpatrolled = null;
+
+               // Same for sha1
+               if ( $this->mSha1 === null ) {
+                       $this->mSha1 = $this->mText === null ? null : self::base36Sha1( $this->mText );
+               }
+
+               // force lazy init
+               $this->getContentModel();
+               $this->getContentFormat();
        }
 
        /**

diff --git a/tests/phpunit/includes/RevisionTest.php b/tests/phpunit/includes/RevisionTest.php
index 39f7e5c..ef4d127 100644 (file)
--- a/tests/phpunit/includes/RevisionTest.php
+++ b/tests/phpunit/includes/RevisionTest.php
@@ -57,7 +57,7 @@ class RevisionTest extends MediaWikiTestCase {
                parent::tearDown();
        }
 
-       public function provideConstruct() {
+       public function provideConstructFromArray() {
                yield 'with text' => [
                        [
                                'text' => 'hello world.',
@@ -72,16 +72,16 @@ class RevisionTest extends MediaWikiTestCase {
        }
 
        /**
-        * @dataProvider provideConstruct
+        * @dataProvider provideConstructFromArray
         */
-       public function testConstruct( $rowArray ) {
+       public function testConstructFromArray( $rowArray ) {
                $rev = new Revision( $rowArray );
                $this->assertNotNull( $rev->getContent(), 'no content object available' );
                $this->assertEquals( CONTENT_MODEL_JAVASCRIPT, $rev->getContent()->getModel() );
                $this->assertEquals( CONTENT_MODEL_JAVASCRIPT, $rev->getContentModel() );
        }
 
-       public function provideConstructThrowsExceptions() {
+       public function provideConstructFromArrayThrowsExceptions() {
                yield 'content and text_id both not empty' => [
                        [
                                'content' => new WikitextContent( 'GOAT' ),
@@ -105,9 +105,9 @@ class RevisionTest extends MediaWikiTestCase {
        }
 
        /**
-        * @dataProvider provideConstructThrowsExceptions
+        * @dataProvider provideConstructFromArrayThrowsExceptions
         */
-       public function testConstructThrowsExceptions( $rowArray, Exception $expectedException ) {
+       public function testConstructFromArrayThrowsExceptions( $rowArray, Exception $expectedException ) {
                $this->setExpectedException(
                        get_class( $expectedException ),
                        $expectedException->getMessage(),