We must have a generic full_message method in the Koha::Exceptions base class to display all the info we have about the exception. The current state is bad as we are displaying the "description" of the exception but it does not contain useful information.
Created attachment 129329 [details] [review] Bug 29857: Make the exception classes inherit from the base class We have a base class Koha::Exceptions::Exception but it's not used in most of our exception classes
Created attachment 129330 [details] [review] Bug 29857: Provide a generic full_message method for our exceptions We pass useful information when throwing exceptions, but we don't display them! We must display all the details we need when the exception is stringified. Test plan: Use the following snippet: use Koha::Exceptions::Elasticsearch; Koha::Exceptions::Elasticsearch::BadResponse->throw( type => "a good type", details => "and some details" ); Execute it without and with the patch.
[Bad response received when submitting request to Elasticsearch] vs Exception 'Koha::Exceptions::Elasticsearch::BadResponse' thrown 'Bad response received when submitting request to Elasticsearch' with type => a good type, details => and some details
This seems a solid improvement to me.. honestly I've found our exceptions confusing to work with.. creating new ones and all the classes all over the place.. this works towards making them all consistent at least.
What are the next steps here, discussion wise. I'm on board with the change myself and would happily sign off as is.. were there any particular points you wanted to raise/question Jonathan?
Ready for signoff then :)
Created attachment 130007 [details] [review] Bug 29857: Make the exception classes inherit from the base class We have a base class Koha::Exceptions::Exception but it's not used in most of our exception classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130008 [details] [review] Bug 29857: Provide a generic full_message method for our exceptions We pass useful information when throwing exceptions, but we don't display them! We must display all the details we need when the exception is stringified. Test plan: Use the following snippet: use Koha::Exceptions::Elasticsearch; Koha::Exceptions::Elasticsearch::BadResponse->throw( type => "a good type", details => "and some details" ); Execute it without and with the patch. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130009 [details] [review] Bug 29857: Rename base class as Koha::Exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130010 [details] [review] Bug 29857: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130011 [details] [review] Bug 29857: Fix behavior This patch makes: - passed parameters be rendered alphabetically to allow testing. - only the defined parameters be rendered in the string - the method honor the fact that $self->message should be returned if passed to the constructor (i.e. if the developer called Koha::Exceptions::MyException->throw( $string ) they will expect $string to be rendered. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/Exceptions.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130012 [details] [review] Bug 29857: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130013 [details] [review] Bug 29857: Fix behavior This patch makes: - passed parameters be rendered alphabetically to allow testing. - only the defined parameters be rendered in the string - the method honor the fact that $self->message should be returned if passed to the constructor (i.e. if the developer called Koha::Exceptions::MyException->throw( $string ) they will expect $string to be rendered. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/Exceptions.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I renamed the 'base' exception class to Koha::Exception as it is something I wanted to do for a long time already. Beware the package/file name doesn't need to match the exception name in Exception::Class, so we could've left Koha::Exceptions::Exception and still use Koha::Exception->throw if we named it like that. But... this is certainly better :-D I tried to highlight the expected behaviors with the tests and found some glitches, that are fixed on the 'Fix behavior' patch. Hope you agree with the proposed follow-ups.
Created attachment 130014 [details] [review] Bug 29857: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130015 [details] [review] Bug 29857: Fix behavior This patch makes: - passed parameters be rendered alphabetically to allow testing. - only the defined parameters be rendered in the string - the method honor the fact that $self->message should be returned if passed to the constructor (i.e. if the developer called Koha::Exceptions::MyException->throw( $string ) they will expect $string to be rendered. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/Exceptions.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Dude, this is a great set of follow-ups. Will work through them properly and test it tomorrow
Looking here
(In reply to Jonathan Druart from comment #0) > We must have a generic full_message method in the Koha::Exceptions base > class to display all the info we have about the exception. > The current state is bad as we are displaying the "description" of the > exception but it does not contain useful information. Exception 'Koha::Exception::TestMR' thrown 'Something went wrong!' with status => x, type => HASH(0x5653eb753278) How useful is HASH(0x5653eb753278) ?
Looks quite good to me. I am wondering if we should remove the confusion between singular and plural completely now. We have a base class Koha::Exception. And we could define subclasses in Koha/Exception/ like Koha/Exception/Generic.pm to contain those now in Exceptions.pm. And the rest already in Exceptions/. What do you think?
Created attachment 130156 [details] [review] Bug 29857: (CONCEPT) Switch to singular, add a load method The load method calls the Class::Exception import. Allowing you to skip a use Class::Exception everywhere, and define 'nicer' exception lists?
(In reply to Marcel de Rooy from comment #21) > Created attachment 130156 [details] [review] [review] > Bug 29857: (CONCEPT) Switch to singular, add a load method > > The load method calls the Class::Exception import. Allowing you > to skip a use Class::Exception everywhere, and define 'nicer' > exception lists? Any thoughts ?
Created attachment 130157 [details] [review] Bug 29857: (QA follow-up) Add license
Created attachment 130158 [details] [review] Bug 29857: (CONCEPT) Switch to singular, add a load method The load method calls the Class::Exception import. Allowing you to skip a use Class::Exception everywhere, and define 'nicer' exception lists?
IMO we are going too far here. Please provide patches on follow-up bug reports instead. I don't understand why the "Virtualshelves exceptions" part is in Koha::Exception::Generic.
(In reply to Marcel de Rooy from comment #19) > (In reply to Jonathan Druart from comment #0) > > We must have a generic full_message method in the Koha::Exceptions base > > class to display all the info we have about the exception. > > The current state is bad as we are displaying the "description" of the > > exception but it does not contain useful information. > > Exception 'Koha::Exception::TestMR' thrown 'Something went wrong!' with > status => x, type => HASH(0x5653eb753278) > > How useful is HASH(0x5653eb753278) ? We should call Dumper if it's a struct.
(In reply to Jonathan Druart from comment #26) > (In reply to Marcel de Rooy from comment #19) > > (In reply to Jonathan Druart from comment #0) > > > We must have a generic full_message method in the Koha::Exceptions base > > > class to display all the info we have about the exception. > > > The current state is bad as we are displaying the "description" of the > > > exception but it does not contain useful information. > > > > Exception 'Koha::Exception::TestMR' thrown 'Something went wrong!' with > > status => x, type => HASH(0x5653eb753278) > > > > How useful is HASH(0x5653eb753278) ? > > We should call Dumper if it's a struct. Marcel, did you find an example in the code or it's theoretical?
(In reply to Jonathan Druart from comment #27) > (In reply to Jonathan Druart from comment #26) > > (In reply to Marcel de Rooy from comment #19) > > > (In reply to Jonathan Druart from comment #0) > > > > We must have a generic full_message method in the Koha::Exceptions base > > > > class to display all the info we have about the exception. > > > > The current state is bad as we are displaying the "description" of the > > > > exception but it does not contain useful information. > > > > > > Exception 'Koha::Exception::TestMR' thrown 'Something went wrong!' with > > > status => x, type => HASH(0x5653eb753278) > > > > > > How useful is HASH(0x5653eb753278) ? > > > > We should call Dumper if it's a struct. > > Marcel, did you find an example in the code or it's theoretical? I do not have a real example. So I am not sure about that. But it isn't far fetched if we are starting to use this more. Dumper could do it indeed.
(In reply to Jonathan Druart from comment #25) > IMO we are going too far here. Please provide patches on follow-up bug > reports instead. > > I don't understand why the "Virtualshelves exceptions" part is in > Koha::Exception::Generic. Because it is now in Exceptions.pm where it does not belong either :)
(In reply to Jonathan Druart from comment #25) > IMO we are going too far here. Please provide patches on follow-up bug > reports instead. Heh. I see this happen all the time :) As to singular, I think we do half of the work here and then leave it. And we should try more to finish what we started? But you can remove the concept patch. Not sure if it will reach another report.
Created attachment 130355 [details] [review] Bug 29857: (QA follow-up) Add license Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Marcel de Rooy from comment #22) > (In reply to Marcel de Rooy from comment #21) > > Created attachment 130156 [details] [review] [review] [review] > > Bug 29857: (CONCEPT) Switch to singular, add a load method > > > > The load method calls the Class::Exception import. Allowing you > > to skip a use Class::Exception everywhere, and define 'nicer' > > exception lists? > > Any thoughts ? I like it. It deserves its own bug IMHO.
(In reply to Jonathan Druart from comment #25) > I don't understand why the "Virtualshelves exceptions" part is in > Koha::Exception::Generic. I filed bug 30057 and submitted patches. Test plz
Created attachment 131112 [details] [review] Bug 29857: Make the exception classes inherit from the base class We have a base class Koha::Exceptions::Exception but it's not used in most of our exception classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131113 [details] [review] Bug 29857: Provide a generic full_message method for our exceptions We pass useful information when throwing exceptions, but we don't display them! We must display all the details we need when the exception is stringified. Test plan: Use the following snippet: use Koha::Exceptions::Elasticsearch; Koha::Exceptions::Elasticsearch::BadResponse->throw( type => "a good type", details => "and some details" ); Execute it without and with the patch. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131114 [details] [review] Bug 29857: Rename base class as Koha::Exception Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131115 [details] [review] Bug 29857: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131116 [details] [review] Bug 29857: Fix behavior This patch makes: - passed parameters be rendered alphabetically to allow testing. - only the defined parameters be rendered in the string - the method honor the fact that $self->message should be returned if passed to the constructor (i.e. if the developer called Koha::Exceptions::MyException->throw( $string ) they will expect $string to be rendered. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/Exceptions.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131117 [details] [review] Bug 29857: (QA follow-up) Add license Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Glad to see the next stage put onto it's own bug... The code here works well and clear things up a lot. QA scripts are happy, tests passing. Passing QA
Created attachment 131232 [details] [review] Bug 29857: (QA follow-up) Fix unit test Object.t Unit test failling with : # Failed test 'Exception message correct' # at /kohadevbox/koha/t/db_dependent/Koha/Object.t line 335. # got: 'Exception 'Koha::Exception' thrown 'Asked to embed items but its return value doesn't implement to_api' # ' # expected: 'Asked to embed items but its return value doesn't implement to_api' # Looks like you failed 1 test of 29. Fixed by using $@->message like in other places. prove t/db_dependent/Koha/Object.t
There are other fails in test suite but I push master, we will fix them
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Created attachment 131240 [details] [review] Bug 29857: (follow-up) Fix Barcode_transform_hooks.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131241 [details] [review] Bug 29857: (follow-up) Fix t/Koha/Plugins/Tab.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131242 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/Koha/Biblio.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131243 [details] [review] Bug 29857: (follow-up) Fix t/Koha/Object/Message.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131244 [details] [review] Bug 29857: (follow-up) Fix t/Koha/Email.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131256 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/Auth.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 131258 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/Koha/Patron/Modifications.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Just in case: I'm submitting the fixes one by one just in case I need to leave and cannot complete the fixes. But this could just be pushed on a single commit.
(In reply to Tomás Cohen Arazi from comment #51) > Just in case: I'm submitting the fixes one by one just in case I need to > leave and cannot complete the fixes. But this could just be pushed on a > single commit. These one line patches read so beautifully ;)
Created attachment 131287 [details] [review] Bug 29857: (follow-up) Fix t/Koha/REST/Plugin/Query.t
Created attachment 131288 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/Koha/Plugins/Plugins.t
Created attachment 131289 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/Koha/Club/Hold.t
Created attachment 131290 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/Koha/BackgroundJobs/BatchUpdateBiblio.t
Created attachment 131291 [details] [review] Bug 29857: (follow-up) Fix t/db_dependent/api/v1/transfer_limits.t
Follow-ups pushed to master : > git log --oneline --no-decorate --grep=29857 be74dd2edf Bug 29857: (follow-up) Fix t/db_dependent/api/v1/transfer_limits.t 94220261f2 Bug 29857: (follow-up) Fix t/db_dependent/Koha/BackgroundJobs/BatchUpdateBiblio.t b5560af25e Bug 29857: (follow-up) Fix t/db_dependent/Koha/Club/Hold.t 6ccf04661a Bug 29857: (follow-up) Fix t/db_dependent/Koha/Plugins/Plugins.t e93de8268e Bug 29857: (follow-up) Fix t/Koha/REST/Plugin/Query.t 13a49f860f Bug 29857: (follow-up) Fix t/db_dependent/Koha/Patron/Modifications.t d406051dcb Bug 29857: (follow-up) Fix t/db_dependent/Auth.t 8d2c00fb02 Bug 29857: (follow-up) Fix t/Koha/Email.t e568e8ef5d Bug 29857: (follow-up) Fix t/Koha/Object/Message.t 42c592f3ad Bug 29857: (follow-up) Fix t/db_dependent/Koha/Biblio.t bcd98589c1 Bug 29857: (follow-up) Fix t/Koha/Plugins/Tab.t 651a2228aa Bug 29857: (follow-up) Fix Barcode_transform_hooks.t c287121471 Bug 29857: (QA follow-up) Fix unit test Object.t 6ece7185b0 Bug 29857: (QA follow-up) Add license 3951145cf7 Bug 29857: Fix behavior c10640e8a7 Bug 29857: Unit tests 2b9fae5445 Bug 29857: Rename base class as Koha::Exception 7429f28cfc Bug 29857: Provide a generic full_message method for our exceptions 42096deea4 Bug 29857: Make the exception classes inherit from the base class