@tsteur opened this Issue on October 30th 2014 Owner

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.

@mnapoli commented on October 30th 2014 Member

:+1: 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 commented on October 30th 2014 Owner

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.

@tsteur commented on November 2nd 2014 Owner

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 commented on November 3rd 2014 Owner

@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 commented on November 4th 2014 Owner

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

@tsteur commented on October 28th 2015 Owner

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

@mattab commented on June 19th 2017 Owner

@tsteur is this issue still valid?

Powered by GitHub Issue Mirror