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
Comments
👍 awesome
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:
(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 |
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. |
I think we should work on this fairly soon or at least refactor out |
@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 |
I am going to create a pull request for it instead of creating an issue which is not needed |
Maybe something from here can be interesting: http://rosstuck.com/formatting-exception-messages/ |
@tsteur is this issue still valid? |
See discussion in https://github.com/piwik/piwik/pull/6541/files#diff-842c76803d19071217437497af04afd5R615
Problem are exceptions like
HtmlMessageException
in classes likeFilechecks
https://github.com/piwik/piwik/pull/6541/files#diff-ec1435896b71140d801a0b5d8d2ca125R107 andSession
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
orDirectoriesNotWritable
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:
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.
The text was updated successfully, but these errors were encountered: