externalstore: Fix insert*() return docs and remove redundant checks

ExternalStorage::insertWithFallback is reported to return false on
failure, but it doesn't. It has a single exit point, and return value is
checked with strlen(), so actually it can only return the URL or throw.
Thus, update any related doc and remove a redundant check from code
calling insertToDefault.

Change-Id: Ic95c3aed19118b987aef105f8077d55558f39127

diff --git a/includes/Storage/SqlBlobStore.php b/includes/Storage/SqlBlobStore.php
index 82410cc..467a8ac 100644 (file)
--- a/includes/Storage/SqlBlobStore.php
+++ b/includes/Storage/SqlBlobStore.php
@@ -220,9 +220,6 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
                        if ( $this->useExternalStore ) {
                                // Store and get the URL
                                $data = ExternalStore::insertToDefault( $data );
-                               if ( !$data ) {
-                                       throw new BlobAccessException( "Failed to store text to external storage" );
-                               }
                                if ( $flags ) {
                                        $flags .= ',';
                                }

diff --git a/includes/externalstore/ExternalStore.php b/includes/externalstore/ExternalStore.php
index 9cf8e15..76f20f0 100644 (file)
--- a/includes/externalstore/ExternalStore.php
+++ b/includes/externalstore/ExternalStore.php
@@ -159,7 +159,7 @@ class ExternalStore {
         *
         * @param string $data
         * @param array $params Map of ExternalStoreMedium::__construct context parameters
-        * @return string|bool The URL of the stored data item, or false on error
+        * @return string The URL of the stored data item
         * @throws MWException
         */
        public static function insertToDefault( $data, array $params = [] ) {
@@ -177,7 +177,7 @@ class ExternalStore {
         * @param array $tryStores Refer to $wgDefaultExternalStore
         * @param string $data
         * @param array $params Map of ExternalStoreMedium::__construct context parameters
-        * @return string|bool The URL of the stored data item, or false on error
+        * @return string The URL of the stored data item
         * @throws MWException
         */
        public static function insertWithFallback( array $tryStores, $data, array $params = [] ) {
@@ -245,7 +245,7 @@ class ExternalStore {
        /**
         * @param string $data
         * @param string $wiki
-        * @return string|bool The URL of the stored data item, or false on error
+        * @return string The URL of the stored data item
         * @throws MWException
         */
        public static function insertToForeignDefault( $data, $wiki ) {

diff --git a/includes/externalstore/ExternalStoreDB.php b/includes/externalstore/ExternalStoreDB.php
index cac16b6..15bc3e0 100644 (file)
--- a/includes/externalstore/ExternalStoreDB.php
+++ b/includes/externalstore/ExternalStoreDB.php
@@ -92,6 +92,9 @@ class ExternalStoreDB extends ExternalStoreMedium {
                return $ret;
        }
 
+       /**
+        * @inheritDoc
+        */
        public function store( $location, $data ) {
                $dbw = $this->getMaster( $location );
                $dbw->insert( $this->getTable( $dbw ),
@@ -105,6 +108,9 @@ class ExternalStoreDB extends ExternalStoreMedium {
                return "DB://$location/$id";
        }
 
+       /**
+        * @inheritDoc
+        */
        public function isReadOnly( $location ) {
                $lb = $this->getLoadBalancer( $location );
                $domainId = $this->getDomainId( $lb->getServerInfo( $lb->getWriterIndex() ) );

diff --git a/includes/externalstore/ExternalStoreMwstore.php b/includes/externalstore/ExternalStoreMwstore.php
index 30c742d..7414f23 100644 (file)
--- a/includes/externalstore/ExternalStoreMwstore.php
+++ b/includes/externalstore/ExternalStoreMwstore.php
@@ -73,29 +73,32 @@ class ExternalStoreMwstore extends ExternalStoreMedium {
                return $blobs;
        }
 
+       /**
+        * @inheritDoc
+        */
        public function store( $backend, $data ) {
                $be = FileBackendGroup::singleton()->get( $backend );
-               if ( $be instanceof FileBackend ) {
-                       // Get three random base 36 characters to act as shard directories
-                       $rand = Wikimedia\base_convert( mt_rand( 0, 46655 ), 10, 36, 3 );
-                       // Make sure ID is roughly lexicographically increasing for performance
-                       $id = str_pad( UIDGenerator::newTimestampedUID128( 32 ), 26, '0', STR_PAD_LEFT );
-                       // Segregate items by wiki ID for the sake of bookkeeping
-                       // @FIXME: this does not include the domain for b/c but it ideally should
-                       $wiki = $this->params['wiki'] ?? wfWikiID();
+               // Get three random base 36 characters to act as shard directories
+               $rand = Wikimedia\base_convert( mt_rand( 0, 46655 ), 10, 36, 3 );
+               // Make sure ID is roughly lexicographically increasing for performance
+               $id = str_pad( UIDGenerator::newTimestampedUID128( 32 ), 26, '0', STR_PAD_LEFT );
+               // Segregate items by wiki ID for the sake of bookkeeping
+               // @FIXME: this does not include the domain for b/c but it ideally should
+               $wiki = $this->params['wiki'] ?? wfWikiID();
 
-                       $url = $be->getContainerStoragePath( 'data' ) . '/' . rawurlencode( $wiki );
-                       $url .= ( $be instanceof FSFileBackend )
-                               ? "/{$rand[0]}/{$rand[1]}/{$rand[2]}/{$id}" // keep directories small
-                               : "/{$rand[0]}/{$rand[1]}/{$id}"; // container sharding is only 2-levels
+               $url = $be->getContainerStoragePath( 'data' ) . '/' . rawurlencode( $wiki );
+               $url .= ( $be instanceof FSFileBackend )
+                       ? "/{$rand[0]}/{$rand[1]}/{$rand[2]}/{$id}" // keep directories small
+                       : "/{$rand[0]}/{$rand[1]}/{$id}"; // container sharding is only 2-levels
 
-                       $be->prepare( [ 'dir' => dirname( $url ), 'noAccess' => 1, 'noListing' => 1 ] );
-                       if ( $be->create( [ 'dst' => $url, 'content' => $data ] )->isOK() ) {
-                               return $url;
-                       }
-               }
+               $be->prepare( [ 'dir' => dirname( $url ), 'noAccess' => 1, 'noListing' => 1 ] );
+               $status = $be->create( [ 'dst' => $url, 'content' => $data ] );
 
-               return false;
+               if ( $status->isOK() ) {
+                       return $url;
+               } else {
+                       throw new MWException( __METHOD__ . ": operation failed: $status" );
+               }
        }
 
        public function isReadOnly( $backend ) {

diff --git a/maintenance/migrateArchiveText.php b/maintenance/migrateArchiveText.php
index b2b14cb..71fff56 100644 (file)
--- a/maintenance/migrateArchiveText.php
+++ b/maintenance/migrateArchiveText.php
@@ -98,9 +98,6 @@ class MigrateArchiveText extends LoggedUpdateMaintenance {
 
                                                if ( $wgDefaultExternalStore ) {
                                                        $data = ExternalStore::insertToDefault( $data );
-                                                       if ( !$data ) {
-                                                               throw new MWException( "Unable to store text to external storage" );
-                                                       }
                                                        if ( $flags ) {
                                                                $flags .= ',';
                                                        }