Request->getHeader() should always return a string

PHPDoc (of the public API) says that this method returns string but it also returns null, which is not allowed in some method calls. This fixes that behaviour and returns an empty string and fixes all code paths that explicitly checked for null to be still compliant. Found while enabling the strict_typing for lib/private for the PHP7+ migration. Signed-off-by: Morris Jobke <[email protected]>
nextcloud · Jan 17, 2018 · fe4ab92 · fe4ab92
1 parent 22b3280
commit fe4ab92
Show file tree

Hide file tree

Showing 10 changed files with 33 additions and 16 deletions.
diff --git a/apps/workflowengine/lib/Check/FileSize.php b/apps/workflowengine/lib/Check/FileSize.php
@@ -103,13 +103,13 @@ protected function getFileSizeFromHeader() {
 		}
 
 		$size = $this->request->getHeader('OC-Total-Length');
-		if ($size === null) {
+		if ($size === '') {
 			if (in_array($this->request->getMethod(), ['POST', 'PUT'])) {
 				$size = $this->request->getHeader('Content-Length');
 			}
 		}
 
-		if ($size === null) {
+		if ($size === '') {
 			$size = false;
 		}
 

diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php
@@ -115,7 +115,7 @@ public function __construct($appName,
 	 */
 	private function getClientName() {
 		$userAgent = $this->request->getHeader('USER_AGENT');
-		return $userAgent !== null ? $userAgent : 'unknown';
+		return $userAgent !== '' ? $userAgent : 'unknown';
 	}
 
 	/**

diff --git a/core/Controller/CssController.php b/core/Controller/CssController.php
@@ -98,7 +98,7 @@ public function getCss($fileName, $appName) {
 	private function getFile(ISimpleFolder $folder, $fileName, &$gzip) {
 		$encoding = $this->request->getHeader('Accept-Encoding');
 
-		if ($encoding !== null && strpos($encoding, 'gzip') !== false) {
+		if (strpos($encoding, 'gzip') !== false) {
 			try {
 				$gzip = true;
 				return $folder->getFile($fileName . '.gzip'); # Safari doesn't like .gz

diff --git a/core/Controller/JsController.php b/core/Controller/JsController.php
@@ -96,7 +96,7 @@ public function getJs($fileName, $appName) {
 	private function getFile(ISimpleFolder $folder, $fileName, &$gzip) {
 		$encoding = $this->request->getHeader('Accept-Encoding');
 
-		if ($encoding !== null && strpos($encoding, 'gzip') !== false) {
+		if (strpos($encoding, 'gzip') !== false) {
 			try {
 				$gzip = true;
 				return $folder->getFile($fileName . '.gzip'); # Safari doesn't like .gz

diff --git a/lib/private/AppFramework/Http/Request.php b/lib/private/AppFramework/Http/Request.php
@@ -324,7 +324,7 @@ public function getHeader($name) {
 
 		}
 
-		return null;
+		return '';
 	}
 
 	/**
@@ -404,8 +404,7 @@ public function getCookie($key) {
 	protected function getContent() {
 		// If the content can't be parsed into an array then return a stream resource.
 		if ($this->method === 'PUT'
-			&& $this->getHeader('Content-Length') !== 0
-			&& $this->getHeader('Content-Length') !== null
+			&& $this->getHeader('Content-Length') !== '0'
 			&& $this->getHeader('Content-Length') !== ''
 			&& strpos($this->getHeader('Content-Type'), 'application/x-www-form-urlencoded') === false
 			&& strpos($this->getHeader('Content-Type'), 'application/json') === false

diff --git a/lib/private/L10N/Factory.php b/lib/private/L10N/Factory.php
@@ -247,7 +247,7 @@ public function languageExists($app, $lang) {
 	 */
 	private function getLanguageFromRequest($app) {
 		$header = $this->request->getHeader('ACCEPT_LANGUAGE');
-		if ($header) {
+		if ($header !== '') {
 			$available = $this->findAvailableLanguages($app);
 
 			// E.g. make sure that 'de' is before 'de_DE'.

diff --git a/lib/private/Log/File.php b/lib/private/Log/File.php
@@ -105,7 +105,10 @@ public static function write($app, $message, $level) {
 		} else {
 			$user = '--';
 		}
-		$userAgent = $request->getHeader('User-Agent') ?: '--';
+		$userAgent = $request->getHeader('User-Agent');
+		if ($userAgent === '') {
+			$userAgent = '--';
+		}
 		$version = $config->getValue('version', '');
 		$entry = compact(
 			'reqId',

diff --git a/lib/private/Memcache/APCu.php b/lib/private/Memcache/APCu.php
@@ -158,8 +158,8 @@ static public function isAvailable() {
 		} elseif (!\OC::$server->getIniWrapper()->getBool('apc.enable_cli') && \OC::$CLI) {
 			return false;
 		} elseif (
-				version_compare(phpversion('apc'), '4.0.6') === -1 &&
-				version_compare(phpversion('apcu'), '5.1.0') === -1
+				version_compare(phpversion('apc') ?: '0.0.0', '4.0.6') === -1 &&
+				version_compare(phpversion('apcu') ?: '0.0.0', '5.1.0') === -1
 		) {
 			return false;
 		} else {

diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php
@@ -431,6 +431,10 @@ public function testGeneratePasswordWithPassword() {
 			->expects($this->once())
 			->method('getServerHost')
 			->willReturn('example.com');
+		$this->request
+			->expects($this->once())
+			->method('getHeader')
+			->willReturn('');
 
 		$expected = new Http\RedirectResponse('nc://login/server:http://example.com&user:MyLoginName&password:MyGeneratedToken');
 		$this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken'));
@@ -583,6 +587,10 @@ public function testGeneratePasswordWithoutPassword() {
 			->expects($this->once())
 			->method('getServerHost')
 			->willReturn('example.com');
+		$this->request
+			->expects($this->once())
+			->method('getHeader')
+			->willReturn('');
 
 		$expected = new Http\RedirectResponse('nc://login/server:http://example.com&user:MyLoginName&password:MyGeneratedToken');
 		$this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken'));

diff --git a/tests/lib/L10N/FactoryTest.php b/tests/lib/L10N/FactoryTest.php
@@ -55,9 +55,16 @@ public function setUp() {
 
 	/**
 	 * @param array $methods
+	 * @param bool $mockRequestGetHeaderMethod
 	 * @return Factory|\PHPUnit_Framework_MockObject_MockObject
 	 */
-	protected function getFactory(array $methods = []) {
+	protected function getFactory(array $methods = [], $mockRequestGetHeaderMethod = false) {
+		if ($mockRequestGetHeaderMethod) {
+			$this->request->expects($this->any())
+				->method('getHeader')
+				->willReturn('');
+		}
+
 		if (!empty($methods)) {
 			return $this->getMockBuilder(Factory::class)
 				->setConstructorArgs([
@@ -137,7 +144,7 @@ public function testFindLanguageWithNotExistingRequestLanguageAndExistingStoredU
 	}
 
 	public function testFindLanguageWithNotExistingRequestLanguageAndNotExistingStoredUserLanguage() {
-		$factory = $this->getFactory(['languageExists']);
+		$factory = $this->getFactory(['languageExists'], true);
 		$this->invokePrivate($factory, 'requestLanguage', ['de']);
 		$factory->expects($this->at(0))
 				->method('languageExists')
@@ -180,7 +187,7 @@ public function testFindLanguageWithNotExistingRequestLanguageAndNotExistingStor
 	}
 
 	public function testFindLanguageWithNotExistingRequestLanguageAndNotExistingStoredUserLanguageAndNotExistingDefault() {
-		$factory = $this->getFactory(['languageExists']);
+		$factory = $this->getFactory(['languageExists'], true);
 		$this->invokePrivate($factory, 'requestLanguage', ['de']);
 		$factory->expects($this->at(0))
 				->method('languageExists')
@@ -226,7 +233,7 @@ public function testFindLanguageWithNotExistingRequestLanguageAndNotExistingStor
 	}
 
 	public function testFindLanguageWithNotExistingRequestLanguageAndNotExistingStoredUserLanguageAndNotExistingDefaultAndNoAppInScope() {
-		$factory = $this->getFactory(['languageExists']);
+		$factory = $this->getFactory(['languageExists'], true);
 		$this->invokePrivate($factory, 'requestLanguage', ['de']);
 		$factory->expects($this->at(0))
 				->method('languageExists')