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

Separated error and exception handling from logging #6842

Merged
merged 3 commits into from Dec 14, 2014

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Dec 11, 2014

This is an implementation of the changes I suggested in #6813: separating logging and exception handling.

I'm opening it as a separate pull request so that we can discuss it here (wether we want this or not) and I can post screenshots without flooding the other pull request.

Summary

  • serious errors and uncatched exceptions are not handled by the logger anymore (because they are errors which stop the page, this is not logging, this is error handling)
  • the error handler logs warnings and notices into the logger, and turns errors into exceptions (ErrorException)
  • the exception handler catches all uncatched exceptions (including ErrorException) and display them to the user (HTML or CLI)
  • the "screen" backend now logs message in HTML notification boxes (using the Notification class): it doesn't hijack/break HTML/JSON output anymore
  • I've normalized exceptions/errors shown to the user in HTML (wether they are catched by the FrontController or not)

Screenshots

Uncatched exception in a controller/API/…

Same as before:

capture d ecran 2014-12-11 a 14 42 04

Uncatched exception outside of the dispatch loop

Before:

capture d ecran 2014-12-11 a 14 46 20

Now:

capture d ecran 2014-12-11 a 14 47 50

Error

Was:

capture d ecran 2014-12-11 a 14 45 40

Now looks like this:

capture d ecran 2014-12-11 a 14 43 38

Fatal error

Same as before:

capture d ecran 2014-12-11 a 14 44 37

Custom message logged when screen logging is enabled

Example code:

Log::warning('This is a warning');
Log::error('This is an error');

Before:

capture d ecran 2014-12-11 a 14 51 26

(the lines of log where added at the top of the page)

Now:

capture d ecran 2014-12-15 a 11 08 23

(we use the notification API)

… logging

- the error handler logs warnings and notices, and turns errors into exceptions (`ErrorException`)
- the exception handler catches all uncatched exceptions and display them to the user (HTML or CLI)
- the "screen" logging backend has been removed
- I've normalized exceptions/errors shown to the user in HTML (wether they are catched by the FrontController or not)
@mnapoli mnapoli added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. and removed Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. labels Dec 11, 2014
@mnapoli mnapoli self-assigned this Dec 11, 2014
@mnapoli mnapoli added this to the Piwik 2.11.0 milestone Dec 11, 2014
…L notifications

The "screen" backend (WebNotificationHandler) now logs to HTML notification boxes using the `Notification` api.
I've re-set the default log level to WARN, and logged PHP notices/warnings to "warning" so that on a default install they are shown in the page in a yellow/warning notification box.

For the record, the default log level had previously been changed from WARN to ERROR because screen logging was previously messing the HTML/JSON output (which was breaking archiving). With the logger refactoring this is no longer a problem.
@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 14, 2014

I've updated the PR:

  • the screen backend now logs messages to HTML notifications (I've updated the last screenshot above to show an example): so PHP notices/warnings are displayed that way
  • the screen backend still exist, except it doesn't break HTML/JSON output anymore (it's a nice HTML notification box)
  • errors and uncatched exceptions still stop the page and display the error page

I've reviewed with @mattab, I'm merging this in the Logger refactoring branch. If you have any more comment, feel free to add them here or on #6813.

Cannot add tests for the screen backend because I would need to mock the session, which is not possible right now because it's all static stuff.
mnapoli added a commit that referenced this pull request Dec 14, 2014
Separated error and exception handling from logging
@mnapoli mnapoli merged commit 95014cd into log-refactoring-1 Dec 14, 2014
@mnapoli mnapoli deleted the log-refactoring-2 branch December 14, 2014 22:45
@mattab
Copy link
Member

mattab commented Dec 14, 2014

Looks good @mnapoli though i have feedback:

  • can you display /reuse the message "Please report this issue in the forums and check it was not reported before etc. [...]" ? as it helps people understand what this notification means and what their next step should be (ie. report to us so we can fix it and improve Piwik for everyone)

@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 15, 2014

@mattab Where/in which case should I display that?

@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 15, 2014

FYI it was displayed for errors (the yellow box), and it's still displayed for those errors

@mattab
Copy link
Member

mattab commented Dec 15, 2014

@mnapoli For php notice messages for example the message to invite users to report in forum should also be displayed (it was displayed before even for PHP notice)

mnapoli added a commit that referenced this pull request Dec 15, 2014
@mnapoli
Copy link
Contributor Author

mnapoli commented Dec 15, 2014

@mattab done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants