From 6af306afa8f5e526afc2f131e2434200cd509737 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Gerg=C5=91=20Tisza?= <tgr.huwiki@gmail.com>
Date: Tue, 8 Sep 2015 14:59:45 -0700
Subject: [PATCH] Rewrite OutputPage::addVaryHeader

Rewrite OutputPage::addVaryHeader which had a very confusing
structure.

There is one breaking change: the $option argument was declared
as array|null, but the function accepted everything and showed
inconsistent behavior; e.g.

    $op->addVaryHeader( 'Foo', 'bar' )

resulted in 'X-Vary-Options: Foo;bar' but

    $op->addVaryHeader( 'Foo' )
    $op->addVaryHeader( 'Foo', 'bar' )

resulted in 'X-Vary-Options: Foo'. With the patch, non-array
arguments (other than null) result in an error.

Change-Id: Id31d95fe27b01b00ec8a1d7a3996275fc0aacf3c
---
 includes/OutputPage.php                   | 15 ++---
 tests/phpunit/includes/OutputPageTest.php | 80 +++++++++++++++++++++++
 2 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index 75dbd4c342..12651d72c0 100644
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -2015,17 +2015,14 @@ class OutputPage extends ContextSource {
 	 *  - "list-contains=$XXX" varies on whether the header value as a
 	 *    comma-separated list contains $XXX as one of the list items.
 	 */
-	public function addVaryHeader( $header, $option = null ) {
+	public function addVaryHeader( $header, array $option = null ) {
 		if ( !array_key_exists( $header, $this->mVaryHeader ) ) {
-			$this->mVaryHeader[$header] = (array)$option;
-		} elseif ( is_array( $option ) ) {
-			if ( is_array( $this->mVaryHeader[$header] ) ) {
-				$this->mVaryHeader[$header] = array_merge( $this->mVaryHeader[$header], $option );
-			} else {
-				$this->mVaryHeader[$header] = $option;
-			}
+			$this->mVaryHeader[$header] = array();
+		}
+		if ( !is_array( $option ) ) {
+			$option = array();
 		}
-		$this->mVaryHeader[$header] = array_unique( (array)$this->mVaryHeader[$header] );
+		$this->mVaryHeader[$header] = array_unique( array_merge( $this->mVaryHeader[$header], $option ) );
 	}
 
 	/**
diff --git a/tests/phpunit/includes/OutputPageTest.php b/tests/phpunit/includes/OutputPageTest.php
index ee2b278d4c..f0d905e599 100644
--- a/tests/phpunit/includes/OutputPageTest.php
+++ b/tests/phpunit/includes/OutputPageTest.php
@@ -259,6 +259,86 @@ class OutputPageTest extends MediaWikiTestCase {
 		$actualHtml = implode( "\n", $links['html'] );
 		$this->assertEquals( $expectedHtml, $actualHtml );
 	}
+
+	/**
+	 * @dataProvider provideVaryHeaders
+	 * @covers OutputPage::addVaryHeader
+	 * @covers OutputPage::getVaryHeader
+	 * @covers OutputPage::getXVO
+	 */
+	public function testVaryHeaders( $calls, $vary, $xvo ) {
+		// get rid of default Vary fields
+		$outputPage = $this->getMockBuilder( 'OutputPage' )
+			->setConstructorArgs( array( new RequestContext() ) )
+			->setMethods( array( 'getCacheVaryCookies' ) )
+			->getMock();
+		$outputPage->expects( $this->any() )
+			->method( 'getCacheVaryCookies' )
+			->will( $this->returnValue( array() ) );
+		TestingAccessWrapper::newFromObject( $outputPage )->mVaryHeader = array();
+
+		foreach ( $calls as $call ) {
+			call_user_func_array( array( $outputPage, 'addVaryHeader' ), $call );
+		}
+		$this->assertEquals( $vary, $outputPage->getVaryHeader(), 'Vary:' );
+		$this->assertEquals( $xvo, $outputPage->getXVO(), 'X-Vary-Options:' );
+	}
+
+	public function provideVaryHeaders() {
+		// note: getXVO() automatically adds Vary: Cookie
+		return array(
+			array( // single header
+				array(
+					array( 'Cookie' ),
+				),
+				'Vary: Cookie',
+				'X-Vary-Options: Cookie',
+			),
+			array( // non-unique headers
+				array(
+					array( 'Cookie' ),
+					array( 'Accept-Language' ),
+					array( 'Cookie' ),
+				),
+				'Vary: Cookie, Accept-Language',
+				'X-Vary-Options: Cookie,Accept-Language',
+			),
+			array( // two headers with single options
+				array(
+					array( 'Cookie', array( 'string-contains=phpsessid' ) ),
+					array( 'Accept-Language', array( 'string-contains=en' ) ),
+				),
+				'Vary: Cookie, Accept-Language',
+				'X-Vary-Options: Cookie;string-contains=phpsessid,Accept-Language;string-contains=en',
+			),
+			array( // one header with multiple options
+				array(
+					array( 'Cookie', array( 'string-contains=phpsessid', 'string-contains=userId' ) ),
+				),
+				'Vary: Cookie',
+				'X-Vary-Options: Cookie;string-contains=phpsessid;string-contains=userId',
+			),
+			array( // Duplicate option
+				array(
+					array( 'Cookie', array( 'string-contains=phpsessid' ) ),
+					array( 'Cookie', array( 'string-contains=phpsessid' ) ),
+					array( 'Accept-Language', array( 'string-contains=en', 'string-contains=en' ) ),
+
+
+				),
+				'Vary: Cookie, Accept-Language',
+				'X-Vary-Options: Cookie;string-contains=phpsessid,Accept-Language;string-contains=en',
+			),
+			array( // Same header, different options
+				array(
+					array( 'Cookie', array( 'string-contains=phpsessid' ) ),
+					array( 'Cookie', array( 'string-contains=userId' ) ),
+				),
+				'Vary: Cookie',
+				'X-Vary-Options: Cookie;string-contains=phpsessid;string-contains=userId',
+			),
+		);
+	}
 }
 
 /**
-- 
2.20.1