From 779df10df7746aa42da4066ba32c0c187e5c8652 Mon Sep 17 00:00:00 2001 From: Lucas Werkmeister Date: Wed, 2 Nov 2022 13:33:42 +0100 Subject: [PATCH] Fix Site::getPath() + MediaWikiSite::getFileUrl() confusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Site::getPath() has returned null since change I08ffa6a970 (commit a00337c3f8), almost ten years ago; I think on the whole it’s safer to fix the prose documentation and keep the @return and actual returned value as they are, rather than the other way around. MediaWikiSite::getPageUrl() compared the $filePath to false instead of null, so the comparison would always be true; but on closer inspection, I think this code was supposed to check $path, not $filePath, in the first place. The only reason this didn’t come up so far is that all known callers pass in a custom path (specifically, all non-test callers pass in 'api.php'), so the $path = false default was never used. To avoid a deprecation warning in PHP 8.1 when passing a null $filePath into str_replace(), throw in that case instead – the function isn’t documented to return string|null, and none of the callers seem prepared to handle a null return as far as I can tell, so throwing seems better than returning null. Tests that produce a null $filePath need to be fixed to complete the Site object (compare Wikibase change If20688411e). Bug: T319219 Change-Id: I5ed7e169c9753486c765fd816d1b21016c5c1def Depends-On: If20688411e3f0108b0c0ba3b91e5cc264b70a208 --- includes/site/MediaWikiSite.php | 6 +++++- includes/site/Site.php | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/includes/site/MediaWikiSite.php b/includes/site/MediaWikiSite.php index a0d957e0cb4..10ebb7a2354 100644 --- a/includes/site/MediaWikiSite.php +++ b/includes/site/MediaWikiSite.php @@ -208,11 +208,15 @@ class MediaWikiSite extends Site { * @param string|bool $path * * @return string + * @throws MWException If the file path cannot be determined. */ public function getFileUrl( $path = false ) { $filePath = $this->getPath( self::PATH_FILE ); + if ( $filePath === null ) { + throw new MWException( "PATH_FILE for site {$this->getGlobalId()} not known" ); + } - if ( $filePath !== false ) { + if ( $path !== false ) { $filePath = str_replace( '$1', $path, $filePath ); } diff --git a/includes/site/Site.php b/includes/site/Site.php index f93ed15dd52..32e1a058896 100644 --- a/includes/site/Site.php +++ b/includes/site/Site.php @@ -615,7 +615,7 @@ class Site implements Serializable { } /** - * Returns the path of the provided type or false if there is no such path. + * Returns the path of the provided type or null if there is no such path. * * @since 1.21 *