From 3aab5f1f3dab19c7a1cb46ca92541c9591254c30 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= <dziewonski@fastmail.fm>
Date: Sat, 15 Feb 2025 00:19:25 +0100
Subject: [PATCH] RestrictionStore: Remove short-circuit mode when fetching
 cascading sources

Almost every call to isCascadeProtected() (which uses short-circuit
mode) is followed by a call to getCascadeProtectionSources() (which
doesn't), so this attempted optimization (skipping a loop that does
some very cheap operations) actually results in worse performance
in the common case (because the result of the database query can't
be cached in short-circuit mode, and we must query it again), and
it makes the code really annoying to read or modify.

Relevant code: https://codesearch.wmcloud.org/search/?q=getCascadeProtectionSources\(|isCascadeProtected\(&excludeFiles=RestrictionStore.php|HISTORY|tests%2F

Change-Id: Ib9eb6cab28492776d40a10cbfb28e9c1cec8c1d2
(cherry picked from commit f9180c4a36fb8874fc0211f05a1eebaceb67aa0c)
---
 includes/Permissions/RestrictionStore.php | 39 ++++++-----------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/includes/Permissions/RestrictionStore.php b/includes/Permissions/RestrictionStore.php
index 4f7cdd6d799..64f23dbb359 100644
--- a/includes/Permissions/RestrictionStore.php
+++ b/includes/Permissions/RestrictionStore.php
@@ -54,7 +54,6 @@ class RestrictionStore {
 	 *   ?array `create_protection` => value for getCreateProtection
 	 *   bool `cascade` => cascade restrictions on this page to included templates and images?
 	 *   array[] `cascade_sources` => the results of getCascadeProtectionSources
-	 *   bool `has_cascading` => Are cascading restrictions in effect on this page?
 	 * ]
 	 */
 	private $cache = [];
@@ -266,7 +265,7 @@ class RestrictionStore {
 	public function isCascadeProtected( PageIdentity $page ): bool {
 		$page->assertWiki( PageIdentity::LOCAL );
 
-		return $this->getCascadeProtectionSourcesInternal( $page, true );
+		return $this->getCascadeProtectionSourcesInternal( $page )[0] !== [];
 	}
 
 	/**
@@ -531,36 +530,32 @@ class RestrictionStore {
 	public function getCascadeProtectionSources( PageIdentity $page ): array {
 		$page->assertWiki( PageIdentity::LOCAL );
 
-		return $this->getCascadeProtectionSourcesInternal( $page, false );
+		return $this->getCascadeProtectionSourcesInternal( $page );
 	}
 
 	/**
 	 * Cascading protection: Get the source of any cascading restrictions on this page.
 	 *
 	 * @param PageIdentity $page Must be local
-	 * @param bool $shortCircuit If true, just return true or false instead of the actual lists.
-	 * @return array|bool If $shortCircuit is true, return true if there is some cascading
-	 *   protection and false otherwise. Otherwise, same as getCascadeProtectionSources().
+	 * @return array[] Same as getCascadeProtectionSources().
 	 */
 	private function getCascadeProtectionSourcesInternal(
-		PageIdentity $page, bool $shortCircuit = false
-	) {
+		PageIdentity $page
+	): array {
 		if ( !$page->canExist() ) {
-			return $shortCircuit ? false : [ [], [], [], [] ];
+			return [ [], [], [], [] ];
 		}
 
 		$cacheEntry = &$this->cache[CacheKeyHelper::getKeyForPage( $page )];
 
-		if ( !$shortCircuit && isset( $cacheEntry['cascade_sources'] ) ) {
+		if ( isset( $cacheEntry['cascade_sources'] ) ) {
 			return $cacheEntry['cascade_sources'];
-		} elseif ( $shortCircuit && isset( $cacheEntry['has_cascading'] ) ) {
-			return $cacheEntry['has_cascading'];
 		}
 
 		$dbr = $this->loadBalancer->getConnection( DB_REPLICA );
 
 		$baseQuery = $dbr->newSelectQueryBuilder()
-			->select( $shortCircuit ? [ 'pr_expiry' ] : [
+			->select( [
 				'pr_expiry',
 				'pr_page',
 				'page_namespace',
@@ -569,12 +564,9 @@ class RestrictionStore {
 				'pr_level'
 			] )
 			->from( 'page_restrictions' )
+			->join( 'page', null, 'page_id=pr_page' )
 			->where( [ 'pr_cascade' => 1 ] );
 
-		if ( !$shortCircuit ) {
-			$baseQuery->join( 'page', null, 'page_id=pr_page' );
-		}
-
 		$imageQuery = clone $baseQuery;
 		$imageQuery->join( 'imagelinks', null, 'il_from=pr_page' )
 			->fields( [
@@ -607,14 +599,8 @@ class RestrictionStore {
 		$pageRestrictions = [];
 		$now = wfTimestampNow();
 		foreach ( $res as $row ) {
-
 			$expiry = $dbr->decodeExpiry( $row->pr_expiry );
 			if ( $expiry > $now ) {
-				if ( $shortCircuit ) {
-					$cacheEntry['has_cascading'] = true;
-					return true;
-				}
-
 				if ( $row->type === 'il' ) {
 					$ilSources[$row->pr_page] = new PageIdentityValue( $row->pr_page,
 					$row->page_namespace, $row->page_title, PageIdentity::LOCAL );
@@ -638,14 +624,9 @@ class RestrictionStore {
 
 		$sources = array_replace( $tlSources, $ilSources );
 
-		$cacheEntry['has_cascading'] = (bool)$sources;
 		$cacheEntry['cascade_sources'] = [ $sources, $pageRestrictions, $tlSources, $ilSources ];
 
-		if ( $shortCircuit ) {
-			return false;
-		}
-
-		return [ $sources, $pageRestrictions, $tlSources, $ilSources ];
+		return $cacheEntry['cascade_sources'];
 	}
 
 	/**