From dfbf5830b235d3b1f7f9c49bb4d3c3727dfd5bab Mon Sep 17 00:00:00 2001
From: Thiemo Kreuz <thiemo.kreuz@wikimedia.de>
Date: Tue, 5 Apr 2022 17:59:25 +0200
Subject: [PATCH] Move "dirty" logic to PoolWorkArticleView subclass that uses
 it

There is only a single subclass that ever does anything with these
two boolean properties. Only 3 states are possible. Pretty much all
of this belongs to the subclass. No other code should have to know
anything about this.

This patch doesn't fully solve the issue but moves code in the
described direction.

Bug: T304813
Change-Id: I70754546f065b03ff04a73307c10f22fbb040810
---
 includes/page/ParserOutputAccess.php          |  7 +++---
 includes/poolcounter/PoolWorkArticleView.php  | 24 -------------------
 .../PoolWorkArticleViewCurrent.php            | 16 +++++++++++--
 .../PoolWorkArticleViewCurrentTest.php        | 12 ++++------
 4 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/includes/page/ParserOutputAccess.php b/includes/page/ParserOutputAccess.php
index 920f4dd2e8e..27347bf4b40 100644
--- a/includes/page/ParserOutputAccess.php
+++ b/includes/page/ParserOutputAccess.php
@@ -309,11 +309,10 @@ class ParserOutputAccess {
 				$classCacheKey = $this->primaryCache->makeParserOutputKey( $page, $parserOptions );
 				$this->localCache[$classCacheKey] = $output;
 			}
-			if ( $work->getIsDirty() ) {
-				$staleReason = $work->getIsFastStale()
-					? 'view-pool-contention' : 'view-pool-overload';
+			if ( $work instanceof PoolWorkArticleViewCurrent && $work->getDirtyWarning() ) {
 				$status->warning( 'view-pool-dirty-output' );
-				$status->warning( $staleReason );
+				// @phan-suppress-next-line PhanTypeMismatchArgumentNullable Null check is above
+				$status->warning( $work->getDirtyWarning() );
 			}
 		}
 
diff --git a/includes/poolcounter/PoolWorkArticleView.php b/includes/poolcounter/PoolWorkArticleView.php
index 390910d5665..22482cef2ef 100644
--- a/includes/poolcounter/PoolWorkArticleView.php
+++ b/includes/poolcounter/PoolWorkArticleView.php
@@ -45,12 +45,6 @@ class PoolWorkArticleView extends PoolCounterWork {
 	/** @var ParserOutput|bool */
 	protected $parserOutput = false;
 
-	/** @var bool */
-	protected $isDirty = false;
-
-	/** @var bool */
-	protected $isFast = false;
-
 	/** @var Status|bool */
 	protected $error = false;
 
@@ -87,24 +81,6 @@ class PoolWorkArticleView extends PoolCounterWork {
 		return $this->parserOutput;
 	}
 
-	/**
-	 * Get whether the ParserOutput is a dirty one (i.e. expired)
-	 *
-	 * @return bool
-	 */
-	public function getIsDirty() {
-		return $this->isDirty;
-	}
-
-	/**
-	 * Get whether the ParserOutput was retrieved in fast stale mode
-	 *
-	 * @return bool
-	 */
-	public function getIsFastStale() {
-		return $this->isFast;
-	}
-
 	/**
 	 * Get a Status object in case of error or false otherwise
 	 *
diff --git a/includes/poolcounter/PoolWorkArticleViewCurrent.php b/includes/poolcounter/PoolWorkArticleViewCurrent.php
index 8e85941b76c..caecbde5b6c 100644
--- a/includes/poolcounter/PoolWorkArticleViewCurrent.php
+++ b/includes/poolcounter/PoolWorkArticleViewCurrent.php
@@ -47,6 +47,9 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 	/** @var WikiPageFactory */
 	private $wikiPageFactory;
 
+	/** @var string|null */
+	private $dirtyWarning = null;
+
 	/**
 	 * @param string $workKey
 	 * @param PageRecord $page
@@ -170,8 +173,17 @@ class PoolWorkArticleViewCurrent extends PoolWorkArticleView {
 		}
 
 		$logger->info( $fast ? 'fast dirty output' : 'dirty output', [ 'workKey' => $this->workKey ] );
-		$this->isDirty = true;
-		$this->isFast = $fast;
+		$this->dirtyWarning = $fast ? 'view-pool-contention' : 'view-pool-overload';
 		return true;
 	}
+
+	/**
+	 * Get whether the ParserOutput is a dirty one (i.e. expired)
+	 *
+	 * @return string|null
+	 */
+	public function getDirtyWarning(): ?string {
+		return $this->dirtyWarning;
+	}
+
 }
diff --git a/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php b/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php
index 793f4a59d84..3a3b9a6d2f6 100644
--- a/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php
+++ b/tests/phpunit/includes/poolcounter/PoolWorkArticleViewCurrentTest.php
@@ -129,13 +129,11 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 
 		$this->assertFalse( $work->fallback( true ) );
 		$this->assertFalse( $work->getParserOutput() );
-		$this->assertFalse( $work->getIsDirty() );
-		$this->assertFalse( $work->getIsFastStale() );
+		$this->assertNull( $work->getDirtyWarning() );
 
 		$this->assertTrue( $work->fallback( false ) );
 		$this->assertInstanceOf( ParserOutput::class, $work->getParserOutput() );
-		$this->assertTrue( $work->getIsDirty() );
-		$this->assertFalse( $work->getIsFastStale() );
+		$this->assertSame( 'view-pool-overload', $work->getDirtyWarning() );
 	}
 
 	public function testFallbackFromMoreRecentParserCache() {
@@ -159,13 +157,11 @@ class PoolWorkArticleViewCurrentTest extends PoolWorkArticleViewTest {
 
 		$this->assertTrue( $work->fallback( true ) );
 		$this->assertInstanceOf( ParserOutput::class, $work->getParserOutput() );
-		$this->assertTrue( $work->getIsDirty() );
-		$this->assertTrue( $work->getIsFastStale() );
+		$this->assertSame( 'view-pool-contention', $work->getDirtyWarning() );
 
 		$this->assertTrue( $work->fallback( false ) );
 		$this->assertInstanceOf( ParserOutput::class, $work->getParserOutput() );
-		$this->assertTrue( $work->getIsDirty() );
-		$this->assertFalse( $work->getIsFastStale() );
+		$this->assertSame( 'view-pool-overload', $work->getDirtyWarning() );
 	}
 
 }