Bug 13995

Summary: Proper Exception handling
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: Architecture, internals, and plumbingAssignee: Olli-Antti Kivilahti <olli-antti.kivilahti>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, josef.moravec, m.de.rooy, mtompset, olli-antti.kivilahti, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14544
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17425
Change sponsored?: --- Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10744, 14012, 14495, 14539, 14540, 13920, 14437, 14698    
Attachments: Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Proper Exception handling
Bug 13995 - Koha::Exception squashable
Comparing two Exception::Class approaches
Bug 13995 - Proper Exception handling

Description Olli-Antti Kivilahti 2015-04-14 14:05:02 UTC
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.
Comment 1 Olli-Antti Kivilahti 2015-04-14 15:14:41 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-04-14 15:21:46 UTC
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
Comment 3 Olli-Antti Kivilahti 2015-04-19 14:42:10 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2015-06-10 13:58:50 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2015-06-24 12:03:04 UTC Comment hidden (obsolete)
Comment 6 Olli-Antti Kivilahti 2015-06-24 12:31:45 UTC Comment hidden (obsolete)
Comment 7 Olli-Antti Kivilahti 2015-06-24 13:07:10 UTC Comment hidden (obsolete)
Comment 8 Olli-Antti Kivilahti 2015-06-29 12:02:25 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-07-02 15:03:32 UTC
Olli, could you please sent an email on koha-devel about this to explain your choices and get some feedbacks?
Comment 10 Olli-Antti Kivilahti 2015-07-16 12:16:49 UTC Comment hidden (obsolete)
Comment 11 Olli-Antti Kivilahti 2015-07-22 11:23:35 UTC Comment hidden (obsolete)
Comment 12 Olli-Antti Kivilahti 2015-07-24 13:59:32 UTC Comment hidden (obsolete)
Comment 13 Olli-Antti Kivilahti 2015-07-31 10:32:16 UTC Comment hidden (obsolete)
Comment 14 Olli-Antti Kivilahti 2015-08-17 11:59:05 UTC
Created attachment 41529 [details] [review]
Bug 13995 - Proper Exception handling
Comment 15 Olli-Antti Kivilahti 2015-08-31 11:04:01 UTC
Created attachment 42109 [details] [review]
Bug 13995 - Koha::Exception squashable
Comment 16 Zeno Tajoli 2015-09-01 08:13:04 UTC
Patch complexity is 'Large' because this change has many architectural connections
Comment 17 Olli-Antti Kivilahti 2015-09-04 17:04:56 UTC
Created attachment 42415 [details]
Comparing two Exception::Class approaches
Comment 18 Marcel de Rooy 2015-10-09 11:43:43 UTC
Bug 14544 also introduces a Koha::Exceptions module.
Has there been any interaction between the authors about the road to follow?
Comment 19 Mark Tompsett 2016-02-05 20:19:39 UTC
(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)
Comment 20 Olli-Antti Kivilahti 2016-02-08 13:44:34 UTC
(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.
Comment 21 Mark Tompsett 2016-02-08 18:27:09 UTC
(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.
Comment 22 Owen Leonard 2016-03-23 12:59:33 UTC
Based on Comment 21 I think this should be "Failed QA" for now.
Comment 23 Olli-Antti Kivilahti 2017-03-09 14:53:00 UTC
Created attachment 60963 [details] [review]
Bug 13995 - Proper Exception handling
Comment 24 Josef Moravec 2017-05-09 08:28:09 UTC
(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?
Comment 25 Mark Tompsett 2018-01-19 03:15:05 UTC
(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.
Comment 26 Tomás Cohen Arazi 2018-01-19 03:27:54 UTC
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)
Comment 27 Mark Tompsett 2018-01-19 18:03:10 UTC
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.
Comment 28 Katrin Fischer 2023-07-28 22:54:33 UTC
(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?