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

Better exception handling #6564

Open
tsteur opened this issue Oct 30, 2014 · 7 comments
Open

Better exception handling #6564

tsteur opened this issue Oct 30, 2014 · 7 comments
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.

Comments

@tsteur
Copy link
Member

tsteur commented Oct 30, 2014

See discussion in https://github.com/piwik/piwik/pull/6541/files#diff-842c76803d19071217437497af04afd5R615

Problem are exceptions like HtmlMessageException in classes like Filechecks https://github.com/piwik/piwik/pull/6541/files#diff-ec1435896b71140d801a0b5d8d2ca125R107 and Session https://github.com/piwik/piwik/pull/6541/files#diff-d1904580a82516edb7ede7a2d4ca3a37R135 but kinda also always using the \Exception.

Those classes do multiple things. They handle for instance file checks or sessions and they "render" / "output" things. This prevents the usage of those classes in different contexts (for instance in Console or in API's you do not want to output HTML) and it makes it impossible to treat exceptions in some cases. Also if you call a method you don't know which error actually happened as all of them throw the same exception. Sometimes you maybe want to catch one type of exception but ignore all the others. Instead they should throw exceptions like NoPrivilegesException or DirectoriesNotWritable and the rendering / handling should be done somewhere else outside of the class that throws it. For instance in an "ErrorController" or an event or something else. I'm not sure right now what the best solution is as I'd have to think about it for a while. This is especially important for lower level classes like session and filechecks.

Note:
It is often probably not a good idea to handle it in the controller that is triggering a method. Imagine you are calling any method in any controller and then you want to catch a specific exception like this:

try {
    $model->performAction();
} catch (FileNotWriteableException $e) {
}

Now the controller needs to have the knowledge that at some point deep down there must be something file related but actually the controller should not know this. They are coupled now. Meaning whenever you change something in an underlying layer you'll have to update all controllers etc. This is something we should have in mind when looking for a solution.

It is also important that plugins can render / handle exceptions they throw and that not all error handling is done for instance in one plugin. Otherwise one plugin has the knowledge of all possible exceptions etc and they would be coupled again.

@tsteur tsteur 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. labels Oct 30, 2014
@mnapoli
Copy link
Contributor

mnapoli commented Oct 30, 2014

👍 awesome

Now the controller needs to have the knowledge that at some point deep down there must be something file related but actually the controller should not know this. They are coupled now.

Definitely agree with you, we need to avoid that. A solution could be to wrap sub-exceptions with ones that make sense in the layer we are in:

try {
    $filesystem->writeContent($filename, $content);
} catch (FileNotWriteableException $e) {
    throw new ReportGenerationException('There was an error while generating the report: ' $e->getMessage(), 0, $e);
}

For example in a project using Doctrine you would usually have that:

  • PDO layer: throws PDOException
  • ORM layer: catches PDOException and throws Doctrine\ORM\EntityNotFoundException
  • model layer: catches EntityNotFoundException and throw UserNotFoundException

(this is just a stupid example)

That way each layer only cares about the exceptions of the sub-layer it's directly using. I.e. exceptions are part of the public API of a class.

But of course we don't want to do that everywhere and wrap everything. So we should only do that for exceptions that makes sense to be catched. If I take again my first example, do we really need ReportGenerationException? If there's any exception happening when generating the report, then that's it, maybe we don't need a specific class here.

@tsteur
Copy link
Member Author

tsteur commented Oct 30, 2014

I thought about this as well as I know it from other projects too. Problem is you end up having many exceptions and we currently lack some kind of "layer" in Piwik. I notice this from time to time and would even help in some DI discussions etc. We probably need such "layer" exceptions - at least sometimes.

@mattab mattab added this to the Mid term milestone Oct 31, 2014
@tsteur
Copy link
Member Author

tsteur commented Nov 2, 2014

I think we should work on this fairly soon or at least refactor out HtmlMessageException as a first step this or next release. Could create another issue for this.

@mattab
Copy link
Member

mattab commented Nov 3, 2014

@tsteur Ok for creating new issue since this one is maybe too large. If though you still think this is important feel free to move it to Short term

@tsteur
Copy link
Member Author

tsteur commented Nov 4, 2014

I am going to create a pull request for it instead of creating an issue which is not needed

@tsteur
Copy link
Member Author

tsteur commented Oct 28, 2015

Maybe something from here can be interesting: http://rosstuck.com/formatting-exception-messages/

@mattab mattab modified the milestones: Long term, Mid term Dec 5, 2016
@mattab
Copy link
Member

mattab commented Jun 19, 2017

@tsteur is this issue still valid?

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. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

3 participants