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.

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

Use exceptions instead of calls to Piwik_ExitWithMessage #6541

Merged
merged 5 commits into from Oct 28, 2014

Conversation

diosmosis
Copy link
Member

As title. Also includes refactoring Piwik_ExitWithMessage so no echo-ing or exit-ing is done by the function (function is renamed). Includes modification to Log.php so it will work even if config is empty. Finally, includes event that allows modification of error page contents.

Thoughts and reviews are welcome (cc @mattab @mnapoli @tsteur ).

Tests I did:

  • check config.php exceptions result in correct error page
  • Filechecks.php (check changed code results in same behavior)
  • FrontController::init() (check changed code results in same behavior)
  • Log succeeds when no config available.
  • Controller.php (check changed code results in same behavior)
  • minimum php version warning (check error message displayed the same)
  • CorePluginsAdmin controller (check changed code results in same behavior)
  • installation (check works)

@@ -81,7 +81,7 @@ public static function getDatabaseConfig($dbConfig = null)
*/
Piwik::postEvent('Db.getDatabaseConfig', array(&$dbConfig));

$dbConfig['profiler'] = $config->Debug['enable_sql_profiler'];
$dbConfig['profiler'] = @$config->Debug['enable_sql_profiler'];
Copy link
Member

Choose a reason for hiding this comment

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

why silent fail here?

Copy link
Member Author

Choose a reason for hiding this comment

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

when config fails to be created there is no information in the config structure, but this code path is still executed (because of FrontController::__destruct). Code needs to default value to false in this case.

@mattab
Copy link
Member

mattab commented Oct 28, 2014

Nice, less technical debt and code that makes more sense now 👍

diosmosis pushed a commit that referenced this pull request Oct 28, 2014
Use exceptions instead of calls to Piwik_ExitWithMessage. Also includes refactoring Piwik_ExitWithMessage so no echo-ing or exit-ing is done by the function (function is renamed), modification to Log.php so it will work even if config is empty, and, includes event that allows modification of error page contents.
@diosmosis diosmosis merged commit 52f47a9 into master Oct 28, 2014
@diosmosis diosmosis deleted the remove_exitwithmessage branch October 28, 2014 05:52
@mnapoli
Copy link
Contributor

mnapoli commented Oct 28, 2014

Awesome!

BTW maybe the branch can be deleted now? nevermind I misread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants