From 2c3dafeedc1178f2a57109b3e86a146d25878823 Mon Sep 17 00:00:00 2001 From: Kevin Day Date: Sat, 3 Feb 2018 15:09:47 -0600 Subject: [PATCH] Update: partial improvements to error presentation, including passing error_message to now reserved ':{error_message}' replacement parameter Process and pass the error message to the string. It seems that this was not happening. There are a few other issues: - arrays were being passed, increasing complexity (unnecessarily). - use preg_match() instead of direct string comparisons on postgresql error messages to be consistent. - ':{failure_reasons} array is no longer neded with this change. While reviewing and making these changes, I noticed that the entire error message process is a bit inconsistent. I suspect that I had changed how I was processing errors along the process. More work needs to be done, but that is being put off to later. --- common/base/classes/base_database.php | 14 +++--- .../base/classes/base_error_messages_english.php | 50 +++++++++++----------- .../base/classes/base_error_messages_japanese.php | 43 +++++++++---------- common/standard/classes/standard_index.php | 2 +- common/standard/paths/u/user_login.php | 24 ++++------- 5 files changed, 65 insertions(+), 68 deletions(-) diff --git a/common/base/classes/base_database.php b/common/base/classes/base_database.php index 93a93aa..f17eaf3 100644 --- a/common/base/classes/base_database.php +++ b/common/base/classes/base_database.php @@ -908,19 +908,21 @@ class c_base_database extends c_base_return { $warnings = $handle_warnings->get_warnings(); unset($handle_warnings); - $failure_reasons = []; + $false = c_base_return_error::s_false(); if ($warnings instanceof c_base_return_array) { $failure_reasons = $warnings->get_value_exact(); + foreach ($failure_reasons as $key => $failure_reason) { + $failure = c_base_error::s_log($failure_reason['message'], ['arguments' => [':{database_name}' => $this->connection_string->get_database()->get_value_exact(), ':{error_message}' => $failure_reason['message'], ':{function_name}' => __CLASS__ . '->' . __FUNCTION__]], i_base_error_messages::POSTGRESQL_CONNECTION_FAILURE); + $false->set_error($failure); + } + unset($failure); + unset($failure_reason); } unset($warnings); - $error = c_base_error::s_log(NULL, ['arguments' => [':{database_name}' => $this->connection_string->get_database()->get_value_exact(), ':{failure_reasons}' => $failure_reasons, ':{function_name}' => __CLASS__ . '->' . __FUNCTION__]], i_base_error_messages::POSTGRESQL_CONNECTION_FAILURE); - unset($failure_reasons); - - return c_base_return_error::s_false($error); + return $false; } unset($handle_warnings); - unset($warnings); $this->database = $database; unset($database); diff --git a/common/base/classes/base_error_messages_english.php b/common/base/classes/base_error_messages_english.php index 5288c28..0497d7e 100644 --- a/common/base/classes/base_error_messages_english.php +++ b/common/base/classes/base_error_messages_english.php @@ -71,21 +71,32 @@ final class c_base_error_messages_english implements i_base_error_messages { return c_base_return_string::s_new($message); } + // replace the reserved ':{error_message}', with the message assigned to the error object. + $error_message = $error->get_message(); + if (!is_string($error_message)) { + $error_message = ''; + } + + if ($html) { + $processed_message = preg_replace('/:{error_message}/i', '
' . htmlspecialchars($error_message, ENT_HTML5 | ENT_COMPAT | ENT_DISALLOWED | ENT_SUBSTITUTE, 'UTF-8') . '
', $message); + } + else { + $processed_message = preg_replace('/:{error_message}/i', $error_message, $message); + } + unset($error_message); + + if (is_string($processed_message)) { + $message = $processed_message; + } + unset($processed_message); + $details = $error->get_details(); + if (isset($details['arguments']) && is_array($details['arguments'])) { if ($html) { + foreach ($details['arguments'] as $detail_name => $detail_value) { - if (is_array($detail_value)) { - // @fixme: re-write as necessary to handle multiple values. - $detail_value = reset($detail_value); - if (isset($detail_value['message']) && is_string($detail_value['message'])) { - $detail_value = $detail_value['message']; - } - else { - $detail_value = ''; - } - } - else if (!is_string($detail_value)) { + if (!is_string($detail_value)) { $detail_value = ''; } @@ -100,17 +111,7 @@ final class c_base_error_messages_english implements i_base_error_messages { } else { foreach ($details['arguments'] as $detail_name => $detail_value) { - if (is_array($detail_value)) { - // @fixme: re-write as necessary to handle multiple values. - $detail_value = reset($detail_value); - if (isset($detail_value['message']) && is_string($detail_value['message'])) { - $detail_value = $detail_value['message']; - } - else { - $detail_value = ''; - } - } - else if (!is_string($detail_value)) { + if (!is_string($detail_value)) { $detail_value = ''; } @@ -124,6 +125,7 @@ final class c_base_error_messages_english implements i_base_error_messages { unset($detail_name); unset($detail_value); unset($details); + if ($html) { return c_base_return_string::s_new('
' . $message . '
'); } @@ -284,7 +286,7 @@ final class c_base_error_messages_english implements i_base_error_messages { } elseif ($code === static::POSTGRESQL_CONNECTION_FAILURE) { if ($arguments === TRUE) { - return c_base_return_string::s_new('Failed to connect to the database, :{database_name}, reasons: :{failure_reasons}' . (is_null($function_name_string) ? '' : ',') . $function_name_string . '.'); + return c_base_return_string::s_new('Failed to connect to the database, :{database_name}, reasons: :{error_message}' . (is_null($function_name_string) ? '' : ',') . $function_name_string . '.'); } else { return c_base_return_string::s_new('Failed to connect to the database.'); @@ -292,7 +294,7 @@ final class c_base_error_messages_english implements i_base_error_messages { } elseif ($code === static::POSTGRESQL_NO_ACCOUNT) { if ($arguments === TRUE) { - return c_base_return_string::s_new('Database access denied: the account :{database_account} does not exist or does not have the required access' . $function_name_string . '.'); + return c_base_return_string::s_new('Database access denied: the account :{database_account} does not exist or does not have the required access, reasons: :{error_message}' . $function_name_string . '.'); } else { return c_base_return_string::s_new('Database access denied: the account does not exist or does not have the required access.'); diff --git a/common/base/classes/base_error_messages_japanese.php b/common/base/classes/base_error_messages_japanese.php index fe96b6c..cf2e21b 100644 --- a/common/base/classes/base_error_messages_japanese.php +++ b/common/base/classes/base_error_messages_japanese.php @@ -76,21 +76,30 @@ final class c_base_error_messages_japanese implements i_base_error_messages { return c_base_return_string::s_new($message); } + // replace the reserved ':{error_message}', with the message assigned to the error object. + $error_message = $error->get_message(); + if (!is_string($error_message)) { + $error_message = ''; + } + + if ($html) { + $processed_message = preg_replace('/:{error_message}/i', '
' . htmlspecialchars($error_message, ENT_HTML5 | ENT_COMPAT | ENT_DISALLOWED | ENT_SUBSTITUTE, 'UTF-8') . '
', $message); + } + else { + $processed_message = preg_replace('/:{error_message}/i', $error_message, $message); + } + unset($error_message); + + if (is_string($processed_message)) { + $message = $processed_message; + } + unset($processed_message); + $details = $error->get_details(); if (isset($details['arguments']) && is_array($details['arguments'])) { if ($html) { foreach ($details['arguments'] as $detail_name => $detail_value) { - if (is_array($detail_value)) { - // @fixme: re-write as necessary to handle multiple values. - $detail_value = reset($detail_value); - if (isset($detail_value['message']) && is_string($detail_value['message'])) { - $detail_value = $detail_value['message']; - } - else { - $detail_value = ''; - } - } - else if (!is_string($detail_value)) { + if (!is_string($detail_value)) { $detail_value = ''; } @@ -105,17 +114,7 @@ final class c_base_error_messages_japanese implements i_base_error_messages { } else { foreach ($details['arguments'] as $detail_name => $detail_value) { - if (is_array($detail_value)) { - // @fixme: re-write as necessary to handle multiple values. - $detail_value = reset($detail_value); - if (isset($detail_value['message']) && is_string($detail_value['message'])) { - $detail_value = $detail_value['message']; - } - else { - $detail_value = ''; - } - } - else if (!is_string($detail_value)) { + if (!is_string($detail_value)) { $detail_value = ''; } diff --git a/common/standard/classes/standard_index.php b/common/standard/classes/standard_index.php index 58b8c93..3d037f1 100644 --- a/common/standard/classes/standard_index.php +++ b/common/standard/classes/standard_index.php @@ -534,7 +534,7 @@ class c_standard_index extends c_base_return { if (c_base_return::s_has_error($result)) { if ($user_name == $this->settings['database_user_public']) { $error_message = $result->get_error(0)->get_message(); - if ($error_message == 'pg_connect(): Unable to connect to PostgreSQL server: fe_sendauth: no password supplied') { + if (preg_match('/fe_sendauth: no password supplied/i', $error_message) > 0) { $error = c_base_error::s_log('Unable to connect to database with public account (message = ' . $error_message . ').', ['arguments' => [':{database_account}' => $account_name, ':{function_name}' => __CLASS__ . '->' . __FUNCTION__]], i_base_error_messages::POSTGRESQL_NO_ACCOUNT); return c_base_return_error::s_false($error); } diff --git a/common/standard/paths/u/user_login.php b/common/standard/paths/u/user_login.php index 3124ff9..5b2124a 100644 --- a/common/standard/paths/u/user_login.php +++ b/common/standard/paths/u/user_login.php @@ -383,38 +383,32 @@ class c_standard_path_user_login extends c_standard_path { $error = reset($errors); unset($errors); - $details = $error->get_details(); + $error_messsage = $error->get_message(); unset($error); - if (isset($details['arguments'][':{failure_reasons}'][0]['message'])) { + if (is_string($error_message)) { // in the case where the database cannot be connected to, do not attempt to ensure user account. - if (preg_match('/could not connect to server: connection refused/i', $details['arguments'][':{failure_reasons}'][0]['message']) > 0) { + if (preg_match('/could not connect to server: connection refused/i', $error_message) > 0) { // @todo: separate messages for admin users and public users. - #foreach ($details['arguments'][':{failure_reasons}'] as $error_message) { - # $error_messages[] = $error_message; - #} - #unset($error_message); - unset($details); $problems[] = c_base_form_problem::s_create_error(NULL, 'Unable to login, cannot connect to the database.'); return $problems; } - elseif (preg_match('/no pg_hba\.conf entry for host/i', $details['arguments'][':{failure_reasons}'][0]['message']) > 0) { + elseif (preg_match('/no pg_hba\.conf entry for host/i', $error_message) > 0) { // the account either does note exist or is not authorized. // it is a pity that postgresql doesn't differentiate the two. $access_denied = TRUE; } - elseif (preg_match('/password authentication failed for user /i', $details['arguments'][':{failure_reasons}'][0]['message']) > 0) { + elseif (preg_match('/password authentication failed for user /i', $error_message) > 0) { $access_denied = TRUE; } else { - $problems[] = c_base_form_problem::s_create_error(NULL, 'Unable to login, reason: ' . $details['arguments'][':{failure_reasons}'][0]['message'] . '.'); - unset($details); + $problems[] = c_base_form_problem::s_create_error(NULL, 'Unable to login, reason: ' . $error_message . '.'); return $problems; } } - unset($details); + unset($error_message); if ($access_denied) { // it is possible the user name might not exist, so try to auto-create the username if the username does not exist. @@ -521,8 +515,8 @@ class c_standard_path_user_login extends c_standard_path { unset($error); // @todo: not just database errors, but also session create errors need to be checked. - if (isset($details['arguments'][':{failure_reasons}'][0]['message']) && is_string($details['arguments'][':{failure_reasons}'][0]['message'])) { - $problems[] = c_base_form_problem::s_create_error(NULL, 'Unable to login, ' . $details['arguments'][':{failure_reasons}'][0]['message']); + if (isset($details['arguments'][':{error_message}'][0]['message']) && is_string($details['arguments'][':{error_message}'][0]['message'])) { + $problems[] = c_base_form_problem::s_create_error(NULL, 'Unable to login, ' . $details['arguments'][':{error_message}'][0]['message']); } else { // here the reason for failure is unknown. -- 1.8.3.1