Bug 21022 - Exceptions should skip stringifying if message manually passed
Summary: Exceptions should skip stringifying if message manually passed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 20590
Blocks: 20287
  Show dependency treegraph
 
Reported: 2018-06-29 14:02 UTC by Tomás Cohen Arazi
Modified: 2019-10-14 19:56 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21022: Unit tests (1.12 KB, patch)
2018-06-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21022: Use passed message if present (1.33 KB, patch)
2018-06-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21022: (follow-up) Move overloaded full_message method to subclass (3.59 KB, patch)
2018-06-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21022: Unit tests (1.18 KB, patch)
2018-07-12 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21022: Use passed message if present (1.39 KB, patch)
2018-07-12 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21022: (follow-up) Move overloaded full_message method to subclass (3.65 KB, patch)
2018-07-12 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21022: Unit tests (1.25 KB, patch)
2018-07-12 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21022: Use passed message if present (1.46 KB, patch)
2018-07-12 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21022: (follow-up) Move overloaded full_message method to subclass (3.73 KB, patch)
2018-07-12 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2018-06-29 14:02:06 UTC
Bug 20590 introduced a way to create more useful and testable exception messages. But we should still be allowed to pass manual messages, because we might not have all the information we need to build the stringified version like it proposed. Notably on Koha::Object->store, in which we parse the DBI warning.
Comment 1 Tomás Cohen Arazi 2018-06-29 14:18:02 UTC
Created attachment 76571 [details] [review]
Bug 21022: Unit tests
Comment 2 Tomás Cohen Arazi 2018-06-29 14:18:09 UTC
Created attachment 76572 [details] [review]
Bug 21022: Use passed message if present

This patch makes 'full_message' use the passed exception message instead
of trying to build it from the parameters. This is particularly useful
for some situations in which we don't have all the information but would
like to keep using the same exception.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
- Sign off
Comment 3 Tomás Cohen Arazi 2018-06-29 14:18:19 UTC
Created attachment 76573 [details] [review]
Bug 21022: (follow-up) Move overloaded full_message method to subclass

We shouldn't have it overloaded on the base class, as it could get huge
and difficult to find things, and read.

This patch moves things to Koha::Exceptions::Object. We should overload
the full_message method on each exceptions class, as needed.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests still pass!
- Sign off :-D
Comment 4 Martin Renvoize 2018-07-12 12:33:57 UTC
Created attachment 76889 [details] [review]
Bug 21022: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2018-07-12 12:34:00 UTC
Created attachment 76890 [details] [review]
Bug 21022: Use passed message if present

This patch makes 'full_message' use the passed exception message instead
of trying to build it from the parameters. This is particularly useful
for some situations in which we don't have all the information but would
like to keep using the same exception.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
- Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2018-07-12 12:34:03 UTC
Created attachment 76891 [details] [review]
Bug 21022: (follow-up) Move overloaded full_message method to subclass

We shouldn't have it overloaded on the base class, as it could get huge
and difficult to find things, and read.

This patch moves things to Koha::Exceptions::Object. We should overload
the full_message method on each exceptions class, as needed.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests still pass!
- Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2018-07-12 12:34:36 UTC
Seems sane, code is reasonable and well structured, signing off :)
Comment 8 Jonathan Druart 2018-07-12 14:40:42 UTC
Created attachment 76893 [details] [review]
Bug 21022: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2018-07-12 14:40:46 UTC
Created attachment 76894 [details] [review]
Bug 21022: Use passed message if present

This patch makes 'full_message' use the passed exception message instead
of trying to build it from the parameters. This is particularly useful
for some situations in which we don't have all the information but would
like to keep using the same exception.

To test:
- Apply this patchset
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests pass!
- Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2018-07-12 14:40:50 UTC
Created attachment 76895 [details] [review]
Bug 21022: (follow-up) Move overloaded full_message method to subclass

We shouldn't have it overloaded on the base class, as it could get huge
and difficult to find things, and read.

This patch moves things to Koha::Exceptions::Object. We should overload
the full_message method on each exceptions class, as needed.

To test:
- Run:
  $ kshell
 k$ prove t/Koha/Exceptions.t
=> SUCCESS: Tests still pass!
- Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Nick Clemens 2018-07-13 13:49:43 UTC
Awesome work all!

Pushed to master for 18.11
Comment 12 Martin Renvoize 2018-07-15 20:45:17 UTC
Pushed to 18.05.x for 18.05.02
Comment 13 Fridolin SOMERS 2018-07-30 14:28:13 UTC
Depends on Bug 20590 not in 17.11.x