From 5719815f3bd9886f495980fb7ecee1d97b8ef50a Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 29 Aug 2019 01:14:24 -0700 Subject: [PATCH] filebackend: add idiom constant to FileBackend for null results Change-Id: I65f043e87d82192c13d3627fb45ef222f0290bf8 --- includes/libs/filebackend/FSFileBackend.php | 8 +-- includes/libs/filebackend/FileBackend.php | 56 +++++++++++++------ .../libs/filebackend/FileBackendStore.php | 10 ++-- .../libs/filebackend/MemoryFileBackend.php | 2 +- .../libs/filebackend/SwiftFileBackend.php | 14 ++--- 5 files changed, 56 insertions(+), 34 deletions(-) diff --git a/includes/libs/filebackend/FSFileBackend.php b/includes/libs/filebackend/FSFileBackend.php index c05dc286c8..c1a796f75a 100644 --- a/includes/libs/filebackend/FSFileBackend.php +++ b/includes/libs/filebackend/FSFileBackend.php @@ -593,7 +593,7 @@ class FSFileBackend extends FileBackendStore { } elseif ( !$hadError ) { return false; // file does not exist } else { - return null; // failure + return self::UNKNOWN; // failure } } @@ -610,7 +610,7 @@ class FSFileBackend extends FileBackendStore { $exists = is_dir( $dir ); $hadError = $this->untrapWarnings(); - return $hadError ? null : $exists; + return $hadError ? self::UNKNOWN : $exists; } /** @@ -632,7 +632,7 @@ class FSFileBackend extends FileBackendStore { } elseif ( !is_readable( $dir ) ) { $this->logger->warning( __METHOD__ . "() given directory is unreadable: '$dir'\n" ); - return null; // bad permissions? + return self::UNKNOWN; // bad permissions? } return new FSFileBackendDirList( $dir, $params ); @@ -657,7 +657,7 @@ class FSFileBackend extends FileBackendStore { } elseif ( !is_readable( $dir ) ) { $this->logger->warning( __METHOD__ . "() given directory is unreadable: '$dir'\n" ); - return null; // bad permissions? + return self::UNKNOWN; // bad permissions? } return new FSFileBackendFileList( $dir, $params ); diff --git a/includes/libs/filebackend/FileBackend.php b/includes/libs/filebackend/FileBackend.php index 4cacb7af63..72200a82e3 100644 --- a/includes/libs/filebackend/FileBackend.php +++ b/includes/libs/filebackend/FileBackend.php @@ -131,6 +131,9 @@ abstract class FileBackend implements LoggerAwareInterface { const ATTR_METADATA = 2; // files can be stored with metadata key/values const ATTR_UNICODE_PATHS = 4; // files can have Unicode paths (not just ASCII) + /** @var null Idiom for "could not determine due to I/O errors" */ + const UNKNOWN = null; + /** * Create a new backend instance from configuration. * This should only be called from within FileBackendGroup. @@ -209,7 +212,8 @@ abstract class FileBackend implements LoggerAwareInterface { } /** - * Get the unique backend name. + * Get the unique backend name + * * We may have multiple different backends of the same type. * For example, we can have two Swift backends using different proxies. * @@ -231,6 +235,7 @@ abstract class FileBackend implements LoggerAwareInterface { /** * Alias to getDomainId() + * * @return string * @since 1.20 * @deprecated Since 1.34 Use getDomainId() @@ -1164,21 +1169,34 @@ abstract class FileBackend implements LoggerAwareInterface { abstract public function getFileHttpUrl( array $params ); /** - * Check if a directory exists at a given storage path. - * Backends using key/value stores will check if the path is a - * virtual directory, meaning there are files under the given directory. + * Check if a directory exists at a given storage path + * + * For backends using key/value stores, a directory is said to exist whenever + * there exist any files with paths using the given directory path as a prefix + * followed by a forward slash. For example, if there is a file called + * "mwstore://backend/container/dir/path.svg" then directories are said to exist + * at "mwstore://backend/container" and "mwstore://backend/container/dir". These + * can be thought of as "virtual" directories. + * + * Backends that directly use a filesystem layer might enumerate empty directories. + * The clean() method should always be used when files are deleted or moved if this + * is a concern. This is a trade-off to avoid write amplication/contention on file + * changes or read amplification when calling this method. * * Storage backends with eventual consistency might return stale data. * + * @see FileBackend::clean() + * * @param array $params Parameters include: * - dir : storage directory - * @return bool|null Returns null on failure + * @return bool|null Whether a directory exists or null on failure * @since 1.20 */ abstract public function directoryExists( array $params ); /** - * Get an iterator to list *all* directories under a storage directory. + * Get an iterator to list *all* directories under a storage directory + * * If the directory is of the form "mwstore://backend/container", * then all directories in the container will be listed. * If the directory is of form "mwstore://backend/container/dir", @@ -1189,10 +1207,12 @@ abstract class FileBackend implements LoggerAwareInterface { * * Failures during iteration can result in FileBackendError exceptions (since 1.22). * + * @see FileBackend::directoryExists() + * * @param array $params Parameters include: * - dir : storage directory * - topOnly : only return direct child dirs of the directory - * @return Traversable|array|null Returns null on failure + * @return Traversable|array|null Directory list enumerator null on failure * @since 1.20 */ abstract public function getDirectoryList( array $params ); @@ -1205,9 +1225,11 @@ abstract class FileBackend implements LoggerAwareInterface { * * Failures during iteration can result in FileBackendError exceptions (since 1.22). * + * @see FileBackend::directoryExists() + * * @param array $params Parameters include: * - dir : storage directory - * @return Traversable|array|null Returns null on failure + * @return Traversable|array|null Directory list enumerator or null on failure * @since 1.20 */ final public function getTopDirectoryList( array $params ) { @@ -1215,12 +1237,12 @@ abstract class FileBackend implements LoggerAwareInterface { } /** - * Get an iterator to list *all* stored files under a storage directory. - * If the directory is of the form "mwstore://backend/container", - * then all files in the container will be listed. - * If the directory is of form "mwstore://backend/container/dir", - * then all files under that directory will be listed. - * Results will be storage paths relative to the given directory. + * Get an iterator to list *all* stored files under a storage directory + * + * If the directory is of the form "mwstore://backend/container", then all + * files in the container will be listed. If the directory is of form + * "mwstore://backend/container/dir", then all files under that directory will + * be listed. Results will be storage paths relative to the given directory. * * Storage backends with eventual consistency might return stale data. * @@ -1230,7 +1252,7 @@ abstract class FileBackend implements LoggerAwareInterface { * - dir : storage directory * - topOnly : only return direct child files of the directory (since 1.20) * - adviseStat : set to true if stat requests will be made on the files (since 1.22) - * @return Traversable|array|null Returns null on failure + * @return Traversable|array|null File list enumerator or null on failure */ abstract public function getFileList( array $params ); @@ -1245,7 +1267,7 @@ abstract class FileBackend implements LoggerAwareInterface { * @param array $params Parameters include: * - dir : storage directory * - adviseStat : set to true if stat requests will be made on the files (since 1.22) - * @return Traversable|array|null Returns null on failure + * @return Traversable|array|null File list enumerator or null on failure * @since 1.20 */ final public function getTopFileList( array $params ) { @@ -1283,7 +1305,7 @@ abstract class FileBackend implements LoggerAwareInterface { * @param array $params Parameters include: * - srcs : list of source storage paths * - latest : use the latest available data - * @return bool All requests proceeded without I/O errors (since 1.24) + * @return bool Whether all requests proceeded without I/O errors (since 1.24) * @since 1.23 */ abstract public function preloadFileStat( array $params ); diff --git a/includes/libs/filebackend/FileBackendStore.php b/includes/libs/filebackend/FileBackendStore.php index aa95ee40bf..9b901dd1d1 100644 --- a/includes/libs/filebackend/FileBackendStore.php +++ b/includes/libs/filebackend/FileBackendStore.php @@ -604,7 +604,7 @@ abstract class FileBackendStore extends FileBackend { $ps = $this->scopedProfileSection( __METHOD__ . "-{$this->name}" ); $stat = $this->getFileStat( $params ); - return ( $stat === null ) ? null : (bool)$stat; // null => failure + return ( $stat === self::UNKNOWN ) ? self::UNKNOWN : (bool)$stat; } final public function getFileTimestamp( array $params ) { @@ -637,7 +637,7 @@ abstract class FileBackendStore extends FileBackend { // cache entries from mass object listings that do not include the SHA-1. In that // case, loading the persistent stat cache will likely yield the SHA-1. if ( - $stat === null || + $stat === self::UNKNOWN || ( $requireSHA1 && is_array( $stat ) && !isset( $stat['sha1'] ) ) ) { $this->primeFileCache( [ $path ] ); // check persistent cache @@ -936,7 +936,7 @@ abstract class FileBackendStore extends FileBackend { $res = true; break; // found one! } elseif ( $exists === null ) { // error? - $res = null; // if we don't find anything, it is indeterminate + $res = self::UNKNOWN; // if we don't find anything, it is indeterminate } } @@ -957,7 +957,7 @@ abstract class FileBackendStore extends FileBackend { final public function getDirectoryList( array $params ) { list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] ); if ( $dir === null ) { // invalid storage path - return null; + return self::UNKNOWN; } if ( $shard !== null ) { // File listing is confined to a single container/shard @@ -987,7 +987,7 @@ abstract class FileBackendStore extends FileBackend { final public function getFileList( array $params ) { list( $fullCont, $dir, $shard ) = $this->resolveStoragePath( $params['dir'] ); if ( $dir === null ) { // invalid storage path - return null; + return self::UNKNOWN; } if ( $shard !== null ) { // File listing is confined to a single container/shard diff --git a/includes/libs/filebackend/MemoryFileBackend.php b/includes/libs/filebackend/MemoryFileBackend.php index f0cbf3bbcd..88b281e380 100644 --- a/includes/libs/filebackend/MemoryFileBackend.php +++ b/includes/libs/filebackend/MemoryFileBackend.php @@ -148,7 +148,7 @@ class MemoryFileBackend extends FileBackendStore { protected function doGetFileStat( array $params ) { $src = $this->resolveHashKey( $params['src'] ); if ( $src === null ) { - return null; + return false; // invalid path } if ( isset( $this->files[$src] ) ) { diff --git a/includes/libs/filebackend/SwiftFileBackend.php b/includes/libs/filebackend/SwiftFileBackend.php index afd1688c1c..e576c6429e 100644 --- a/includes/libs/filebackend/SwiftFileBackend.php +++ b/includes/libs/filebackend/SwiftFileBackend.php @@ -593,7 +593,7 @@ class SwiftFileBackend extends FileBackendStore { $stat = $this->getContainerStat( $fullCont ); if ( is_array( $stat ) ) { return $status; // already there - } elseif ( $stat === null ) { + } elseif ( $stat === self::UNKNOWN ) { $status->fatal( 'backend-fail-internal', $this->name ); $this->logger->error( __METHOD__ . ': cannot get container stat' ); @@ -832,7 +832,7 @@ class SwiftFileBackend extends FileBackendStore { return ( count( $status->value ) ) > 0; } - return null; // error + return self::UNKNOWN; // error } /** @@ -1401,7 +1401,7 @@ class SwiftFileBackend extends FileBackendStore { if ( !$this->containerStatCache->hasField( $container, 'stat' ) ) { $auth = $this->getAuthentication(); if ( !$auth ) { - return null; + return self::UNKNOWN; } list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->http->run( [ @@ -1427,7 +1427,7 @@ class SwiftFileBackend extends FileBackendStore { $this->onError( null, __METHOD__, [ 'cont' => $container ], $rerr, $rcode, $rdesc ); - return null; + return self::UNKNOWN; } } @@ -1599,7 +1599,7 @@ class SwiftFileBackend extends FileBackendStore { $stats[$path] = false; continue; // invalid storage path } elseif ( !$auth ) { - $stats[$path] = null; + $stats[$path] = self::UNKNOWN; continue; } @@ -1609,7 +1609,7 @@ class SwiftFileBackend extends FileBackendStore { $stats[$path] = false; continue; // ok, nothing to do } elseif ( !is_array( $cstat ) ) { - $stats[$path] = null; + $stats[$path] = self::UNKNOWN; continue; } @@ -1642,7 +1642,7 @@ class SwiftFileBackend extends FileBackendStore { } elseif ( $rcode === 404 ) { $stat = false; } else { - $stat = null; + $stat = self::UNKNOWN; $this->onError( null, __METHOD__, $params, $rerr, $rcode, $rdesc ); } $stats[$path] = $stat; -- 2.20.1