We should have a Java-style Exception handling mechanism in place. It will make it easier to handle exception across the board. Try::Tiny and Exception::Class seem to be the most bestest approaches.
Created attachment 37827 [details] [review] Bug 13995 - Proper Exception handling try { Koha::Exception::BadSystemPreference->throw(error => 'Syspref DisplayIconsXSLT is not a valid boolean'); } catch { if (blessed($_) && $_->isa('Koha::Exception::BadSystemPreference')) { print $_->as_string(); warn $_->error, "\n", $_->trace->as_string, "\n"; } else { $_->rethrow(); } }
Decided to create separate packages/files for each Exception, so IDE's can more easily pick up package definitions and autocomplete available packages, so we don't have to remember was the Exception named as Koha::Exceptions::NoSystempreference or Koha::Exception::NoSysPref or Koha::Exception::NoSystemPreference
Created attachment 38131 [details] [review] Bug 13995 - Proper Exception handling try { Koha::Exception::BadSystemPreference->throw(error => 'Syspref DisplayIconsXSLT is not a valid boolean'); } catch { if (blessed($_) && $_->isa('Koha::Exception::BadSystemPreference')) { print $_->as_string(); warn $_->error, "\n", $_->trace->as_string, "\n"; } else { $_->rethrow(); } }
Created attachment 40074 [details] [review] Bug 13995 - Proper Exception handling try { Koha::Exception::BadSystemPreference->throw(error => 'Syspref DisplayIconsXSLT is not a valid boolean'); } catch { if (blessed($_) && $_->isa('Koha::Exception::BadSystemPreference')) { print $_->as_string(); warn $_->error, "\n", $_->trace->as_string, "\n"; } else { $_->rethrow(); } }
Created attachment 40579 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 40580 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 40581 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 40717 [details] [review] Bug 13995 - Proper Exception handling
Olli, could you please sent an email on koha-devel about this to explain your choices and get some feedbacks?
Created attachment 41018 [details] [review] Bug 13995 - Proper Exception handling Koha has issues with propagating errors and exceptions from the business layer to the front-end. Using a more sophisticated system of named Exceptions we can send more formal signals towards the user. Catching different types of Exceptions makes for a more concise handling of errors. Also throwing/catching Exceptions is a industry standard and for a good reason. There is no point for Koha to lurk behind. USAGE: try { Koha::Exception::BadSystemPreference->throw(error => 'Syspref DisplayIconsXSLT is not a valid boolean'); } catch { if (blessed($_) && $_->isa('Koha::Exception::BadSystemPreference')) { print $_->as_string(); warn $_->error, "\n", $_->trace->as_string, "\n"; } else { $_->rethrow(); } }
Created attachment 41118 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 41153 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 41249 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 41529 [details] [review] Bug 13995 - Proper Exception handling
Created attachment 42109 [details] [review] Bug 13995 - Koha::Exception squashable
Patch complexity is 'Large' because this change has many architectural connections
Created attachment 42415 [details] Comparing two Exception::Class approaches
Bug 14544 also introduces a Koha::Exceptions module. Has there been any interaction between the authors about the road to follow?
(In reply to Marcel de Rooy from comment #18) > Bug 14544 also introduces a Koha::Exceptions module. > Has there been any interaction between the authors about the road to follow? This patch still applies, but there is no "t/Exceptions.t" test file. I think this should be required for a sign off, in addition to answering Marcel's question, since that bug (14544) has been pushed to master. Changing status to 'In Discussion' (to be doomed for all eternity! -- since that is what most 'In Discussion' bugs do)
(In reply to M. Tompsett from comment #19) > (In reply to Marcel de Rooy from comment #18) > > Bug 14544 also introduces a Koha::Exceptions module. > > Has there been any interaction between the authors about the road to follow? > > This patch still applies, but there is no "t/Exceptions.t" test file. I > think this should be required for a sign off, in addition to answering > Marcel's question, since that bug (14544) has been pushed to master. > Changing status to 'In Discussion' (to be doomed for all eternity! -- since > that is what most 'In Discussion' bugs do) Hi! See the attached opendocument presentation about the pros and cons of the two different approaches. If you like this approach, sign off this patch.
(In reply to Olli-Antti Kivilahti from comment #20) > See the attached opendocument presentation about the pros and cons of the > two different approaches. > > If you like this approach, sign off this patch. While I may prefer the individual file for the reason of reducing potential conflicts, I still can't sign off for the very reason given in comment #19. "This patch still applies, but there is no "t/Exceptions.t" test file. I think this should be required for a sign off" Additionally, it has usually been the case that if one method is used first in Koha that either: a) you do it the way already there, OR b) you refactor the other way AFTER discussion on koha-devel. So, if (b) is the choice, then create a refactor bug, and make this depend on that. And of course, actually discuss this on koha-devel. And either way, there should be a test file.
Based on Comment 21 I think this should be "Failed QA" for now.
Created attachment 60963 [details] [review] Bug 13995 - Proper Exception handling
(In reply to Olli-Antti Kivilahti from comment #23) > Created attachment 60963 [details] [review] [review] > Bug 13995 - Proper Exception handling Is it ready for testing again? What is the test plan?
(In reply to Josef Moravec from comment #24) > (In reply to Olli-Antti Kivilahti from comment #23) > > Created attachment 60963 [details] [review] [review] [review] > > Bug 13995 - Proper Exception handling > > Is it ready for testing again? What is the test plan? Nope. Because the test that Olli-Antti Kivilahti added was only for one of the exceptions added. Still Failed QA, because of inadequate test coverage. Jonathan Druart came across as rather particular on this point in a recent IRC meeting.
This patch doesn't really require unit tests in my opinion. But we didn't agree (explicitly) on the one exception per file approach, and since this patch was posted several exceptions have been added. This patch is of big value because Olli put a lot of time thinking about the exceptions we might need and also how to organize them. This should be used as a basis for new exceptions addition. Maybe organized by type or module as the ones we already have (which should be rearranged too, virtualshelves, etc)
I understand the desire to single file each exception. That makes sense. We can use directory structure to sort them if needed. git bz apply 13995 git diff origin/master | grep "package Koha::Exception::" | sed -e "s#\+package ##g" | sed -e "s#;##g" > ~/new_exceptions git grep "'Koha::Exceptions::" | grep -v "isa\W*=" | grep ^Koha/Exceptions | cut -f2- -d: | sed -e "s#[=>{ ']*##g" > ~/existing_exceptions Looking at new and existing exceptions, we see that there may be some overlap and as Tomás Cohen Arazi pointed out in comment #26, there is likely a need for better consolidation and sorting. So even if I wanted to sign this off, there is still clean up to do here. For this reason, I am leaving it Failed QA. I would be willing to sign off if this: 1) duplicates all the exceptions in Koha/Exceptions 2) would refactor nicely such another bug would merely remove the 's' from Exceptions 3) has test coverage for all the exception files -- if 1 & 2 are met, I'd be happy to write the test coverage just to get sign offs moved forward.
(In reply to Tomás Cohen Arazi from comment #26) > This patch doesn't really require unit tests in my opinion. But we didn't > agree (explicitly) on the one exception per file approach, and since this > patch was posted several exceptions have been added. > > This patch is of big value because Olli put a lot of time thinking about the > exceptions we might need and also how to organize them. > > This should be used as a basis for new exceptions addition. Maybe organized > by type or module as the ones we already have (which should be rearranged > too, virtualshelves, etc) Since the codebase has moved on since, should we still keep this open?