Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request->getHeader() should always return a string #7813

Merged
merged 1 commit into from
Jan 17, 2018
+38 −16
Merged

Request->getHeader() should always return a string #7813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/workflowengine/lib/Check/FileSize.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion core/Controller/ClientFlowLoginController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion core/Controller/CssController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion core/Controller/JsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions lib/private/AppFramework/Http/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ public function getHeader($name) {

}

return null;
return '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking public API :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really - the public API says: @return string

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this fixes the public API, because you can see, that many apps use it like it will always return a string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from apps that checked the actual return instead of trusting the docs :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well we should communicate all this properly. But at some point we want to fix the code. I would vote to actually cleanup the code. It is not like we have a million apps that depend on all this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I was just saying that "communicate" is the important word.

I'd really like to use @nextcloud/maintainers or something like that to mention such important cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us collect this in a separate ticket for 14. Because I guess there will be more to come.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7827

}

/**
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/private/L10N/Factory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
Expand Down
5 changes: 4 additions & 1 deletion lib/private/Log/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 2 additions & 2 deletions lib/private/Memcache/APCu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
13 changes: 13 additions & 0 deletions tests/Core/Controller/ClientFlowLoginControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,10 @@ public function testGeneratePasswordWithPassword() {
->expects($this->once())
->method('getServerHost')
->willReturn('example.com');
$this->request
->expects($this->any())
->method('getHeader')
->willReturn('');

$expected = new Http\RedirectResponse('nc://login/server:http://example.com&user:MyLoginName&password:MyGeneratedToken');
$this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken'));
Expand Down Expand Up @@ -583,6 +587,10 @@ public function testGeneratePasswordWithoutPassword() {
->expects($this->once())
->method('getServerHost')
->willReturn('example.com');
$this->request
->expects($this->any())
->method('getHeader')
->willReturn('');

$expected = new Http\RedirectResponse('nc://login/server:http://example.com&user:MyLoginName&password:MyGeneratedToken');
$this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken'));
Expand All @@ -594,6 +602,7 @@ public function dataGeneratePasswordWithHttpsProxy() {
[
['X-Forwarded-Proto', 'http'],
['X-Forwarded-Ssl', 'off'],
['USER_AGENT', ''],
],
'http',
'http',
Expand All @@ -602,6 +611,7 @@ public function dataGeneratePasswordWithHttpsProxy() {
[
['X-Forwarded-Proto', 'http'],
['X-Forwarded-Ssl', 'off'],
['USER_AGENT', ''],
],
'https',
'https',
Expand All @@ -610,6 +620,7 @@ public function dataGeneratePasswordWithHttpsProxy() {
[
['X-Forwarded-Proto', 'https'],
['X-Forwarded-Ssl', 'off'],
['USER_AGENT', ''],
],
'http',
'https',
Expand All @@ -618,6 +629,7 @@ public function dataGeneratePasswordWithHttpsProxy() {
[
['X-Forwarded-Proto', 'https'],
['X-Forwarded-Ssl', 'on'],
['USER_AGENT', ''],
],
'http',
'https',
Expand All @@ -626,6 +638,7 @@ public function dataGeneratePasswordWithHttpsProxy() {
[
['X-Forwarded-Proto', 'http'],
['X-Forwarded-Ssl', 'on'],
['USER_AGENT', ''],
],
'http',
'https',
Expand Down
15 changes: 11 additions & 4 deletions tests/lib/L10N/FactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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')
Expand Down