-
Notifications
You must be signed in to change notification settings - Fork 4.2k
PHPExcel_Exception exists as class but is never used #78
Comments
Currently, work is underway to replace all thrown Exceptions with PHPExcel specific Exception classes. This is a work in progress, but most of the readers and writers throw PHPExcel_Reader_Exception and PHPExcel_Writer_Exception already. |
Happy to hear about that. |
More than happy to accept offers of help.... it's as simple as wading through the code replacing every thrown Exception so that it throws the appropriate PHPExcel_xxx_Exception. I believe I've done the Reader and Writer Exceptions, and the Calc Engine has its own PHPExcel_Calculation_Exception, so all other thrown Exceptions should be PHPExcel_Exception. There's a few places in the code where Exceptions are caught, but these can largely be left simply catching Exception |
Perfect. I'll take care of that and implement a clean Exception architecture. |
Yes, the APIDocs have got a bit out of date in places, and that's also a case of simply trawling through the code to fix all the docblocks, but it's a long, drawn-out chore checking every method that's called by a method to see if an Exception is thrown somewhere down the call stack |
…ception to be able to catch all PHPExcel specific Exceptions in one block
I've changed the inheritance of the PHPExcel_*_Exception Exceptions, they now all extend from PHPExcel_Exception. Now we can fetch PHPExcel_Exception instead of the base Exception. |
Why are there Exceptions thrown in a catch bock of an Exceptions? Example: https://gist.github.com/4112606 |
…ready for testing
Thanks for all these Exception management changes.... I've had to merge them manually, unfortunately; but I just need to check for completeness and consistency (especially those in PHPExcel_Shared_*) and I should be in a position to commit them all tonight |
Your're welcome. I think i replaced all PHPExcel Exceptions. All the Exceptions inherit from a Base PHPExcel_Exception so users are able to catch just PHPExcel Exceptions. They should not need to catch a Graph, Writer or whatever Exception explicit. I did not yet run the tests, but if've tested it in my Tool/FW and it was running without errors. Originally i planned to extend the tests and merge them in the actual head, but because i changed so many files that i got constantly new conflicts and stopped trying. ( I got an idea how sisyphos must have felt ) Maybe i should have done it in smaller pieces and send several pull requests. I'm lacking the experience how to interact with "non continuous integration models". I have several proposal for architecture improvements. |
Sorry, didn't manage to get the merge done tonight - too many interventions, so I didn't get my own final testing complete. A full CI environment would be greate, and we use Travis, but don't yet have a comprehensive set of tests for significant portions of the code, and many of the tests we do have don't yet run cleanly.... it's a work in progress. If you're interested in joining the development team, we can always use fresh blood. There's times when the mind gets too close minded with regard to the existing architecture after working on it for so long. And I'm busy with two large tasks at the moment (the switch from SimpleXML to XMLReader, and a complete rewrite of the calculation engine) so there's a lot of areas being neglected while my focus is on those. |
Done |
You have a custom Exception PHPExcel_Exception, but all classes still use the Exception class.
As PHPExcel_Exception extends Exception it would not break existing code if you just replace all "Exception"s with PHPExcel_Exception.
Sometimes it's a little annoying not to be able to differentiate between a PHPExcel_Exception and other Exceptions.
The text was updated successfully, but these errors were encountered: