From fe4ab927719f7a34770b7397e59036ee17fcfbbd Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 12 Jan 2018 14:15:12 +0100 Subject: [PATCH] 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 --- apps/workflowengine/lib/Check/FileSize.php | 4 ++-- core/Controller/ClientFlowLoginController.php | 2 +- core/Controller/CssController.php | 2 +- core/Controller/JsController.php | 2 +- lib/private/AppFramework/Http/Request.php | 5 ++--- lib/private/L10N/Factory.php | 2 +- lib/private/Log/File.php | 5 ++++- lib/private/Memcache/APCu.php | 4 ++-- .../Controller/ClientFlowLoginControllerTest.php | 8 ++++++++ tests/lib/L10N/FactoryTest.php | 15 +++++++++++---- 10 files changed, 33 insertions(+), 16 deletions(-) diff --git a/apps/workflowengine/lib/Check/FileSize.php b/apps/workflowengine/lib/Check/FileSize.php index 1744793dec72e..7e48f0f603820 100644 --- 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 index 0e7fbf892b60b..23bd42a0f18f8 100644 --- 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 index 95a41f8dd150c..43a4f453b0ee8 100644 --- 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 index f02948e47a023..670ca9972576b 100644 --- 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 index 77ecb02165ba5..975c4255d5acd 100644 --- 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 index e277a00e653d5..8268be05d4b0a 100644 --- 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 index b6a208ad68a37..290f7897c9dbd 100644 --- 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 index 70f0d73d2d483..3e96acdecb733 100644 --- 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 index 0e04853822333..cfc61eccbfab9 100644 --- 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 index 602967b162620..4f31784337a45 100644 --- 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')