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

eliminate OC_Template::printErrorPage in database classes, fixes #12182 #12186

Merged
merged 3 commits into from
Nov 15, 2014
Merged

eliminate OC_Template::printErrorPage in database classes, fixes #12182 #12186

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ed678b
eliminate OC_Template::printErrorPage in database classes, fixes #12182
blizzz Nov 14, 2014
08205c6
errors are already logged
blizzz Nov 14, 2014
74ffda8
do not output DB information, and do not set header
blizzz Nov 14, 2014
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
3 changes: 2 additions & 1 deletion lib/private/db/adapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public function fixupStatement($statement) {
* insert the @input values when they do not exist yet
* @param string $table name
* @param array $input key->value pair, key has to be sanitized properly
* @throws \OC\HintException
* @return int count of inserted rows
*/
public function insertIfNotExist($table, $input) {
Expand Down Expand Up @@ -71,7 +72,7 @@ public function insertIfNotExist($table, $input) {
$entry .= 'Offending command was: ' . $query.'<br />';
\OC_Log::write('core', $entry, \OC_Log::FATAL);
error_log('DB error: ' . $entry);
Copy link
Member

Choose a reason for hiding this comment

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

we can also kill error_log() - would be written to the log twice then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

\OC_Template::printErrorPage( $entry );
throw new \OC\HintException($entry);
}
}
}
4 changes: 2 additions & 2 deletions lib/private/db/adaptersqlite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public function insertIfNotExist($table, $input) {
$entry .= 'Offending command was: ' . $query . '<br />';
\OC_Log::write('core', $entry, \OC_Log::FATAL);
error_log('DB error: '.$entry);
\OC_Template::printErrorPage( $entry );
throw new \OC\HintException($entry);
}

if ($stmt->fetchColumn() === '0') {
Expand All @@ -61,7 +61,7 @@ public function insertIfNotExist($table, $input) {
$entry .= 'Offending command was: ' . $query.'<br />';
\OC_Log::write('core', $entry, \OC_Log::FATAL);
error_log('DB error: ' . $entry);
\OC_Template::printErrorPage( $entry );
throw new \OC\HintException($entry);
}

return $result;
Expand Down
7 changes: 3 additions & 4 deletions lib/private/db/statementwrapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function execute($input=array()) {
} else {
$result = $this->statement->execute();
}

if ($result === false) {
return false;
}
Expand Down Expand Up @@ -161,11 +161,10 @@ private function tryFixSubstringLastArgumentDataForMSSQL($input) {
// send http status 503
header('HTTP/1.1 503 Service Temporarily Unavailable');
header('Status: 503 Service Temporarily Unavailable');
OC_Template::printErrorPage('Failed to connect to database');
die ($entry);
throw new \OC\HintException($entry);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too sure about this one - HintException will translate to http status code 500 - not 503.
The header() calls will be overwritten later.

We should add an httpstatus code to our exception classes - defaulting to 500

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, generic \Exception will end up in 500, HintException in 503, see https://github.com/owncloud/core/blob/master/index.php#L37. Actually, we should get rid of the header() here as well in that case.

Copy link
Member

Choose a reason for hiding this comment

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

no, generic \Exception will end up in 500, HintException in 503

I see - thx - it's not really obvious that HintException will result in 503 - but this is a different discussion 😉

Copy link
Member

Choose a reason for hiding this comment

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

Actually, we should get rid of the header() here as well in that case.

yes

}
}

/**
* provide an alias for fetch
*
Expand Down