https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Update skelton.pl to show |Update skeleton.pl to show |an example use of coding |an example use of coding |guideline SQL14 |guideline SQL14 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171338|0 |1 is obsolete| | --- Comment #2 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 171383 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171383&action=edit Bug 37895: Update skelton.pl to show an example use of coding guideline SQL14 Guideline SQL14 recommends using try/finally blocks to report database modification errors. It would be helpful to update skeleton.pl to use this methodology. Test Plan: 1) Apply this patch 2) Copy skeleton.pl to test.pl 3) Add a bad query to the the $dbh->do({}) line 4) Run updatedatabase.pl 5) Note the error message is displayed 6) Change the bad query to a good query 7) Run updatedatabase.pl 8) Note the success message is displayed! Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi, I had a go at updating the wiki page, but got a bit stuck on the second example: If the column already exists, we might want to do a say_info "nothing happened", if it exists a say_success and if it doesn't work a say_failure. Maybe someone else could have a look once we have this sorted? https://wiki.koha-community.org/wiki/Database_updates Adding info on the checks for existence of tables, columns etc. might also be a good addition to the skeleton.pl. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- I feel like that is somewhat less readable than what was been proposed in the patch but I am open to changes you feel that is better. should read as I feel like that is somewhat less readable than what has been proposed in the patch but I am open to changes if you feel that is better. /me hangs head in shame -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- I generally write the "happy path" in the try block, like this: try { $dbh->do(q{}); say_success( $out, "Database modification was successful!" ); } catch { say_failure( $out, "Database modification failed with errors: $_" ); } I think it's more readable than the SQL14 example: less code, less indentation, and you can read the whole "happy path" in one go. But maybe there's a catch (ah!) somewhere with this approach ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> ---

try { $dbh->do(q{}); say_success( $out, "Database modification was successful!" ); } catch { say_failure( $out, "Database modification failed with errors: $_" ); }

^ this I like -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Kyle M Hall from comment #5)

(In reply to Julian Maurice from comment #3)

...

Shouldn't we add `use Try::Tiny` at the beginning ?

It doesn't seem to be needed as we include it farther upstream from where we execute atomic updates. Should I include it explicitly in updatedatabase.pl just to have it there?

You're right. It is included from C4::Installer which is where we load atomicupdate files so it should be ok. I think the example is still missing something though: what to do when we have an error. In the example, we print the error and continue with the rest of the update. Shouldn't we stop at the first error ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 171660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171660&action=edit Bug 37895: (QA follow-up) Exit in the failure block -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #15 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- I hate to say this after the work that's gone into this, but I think we're missing some important code history here... See bug 25078 - error handling is already baked into Installer.pm, to abort the update if a dbrev fails (or allow a --force parameter to keep going anyway), and to make sure the output is displayed correctly regardless of whether the update is being run from the command line or from the web installer. Testing these patches as-is, I'm getting no output at all to the console on a bad dbrev - the update subs in Installer.pm collect all of the messages and pass them along to the console after the dbrev is finished, and exit(1) bails out of the whole script before that happens. The web installer does display the error messages, but they have added cruft with the patches - an extra layer on the stack trace, and a bunch of array references. Replacing exit(1) with a die statement works better, but die prints its own error message, so there's no real benefit to having both say_failure and die. I don't think we want to include a try/catch in the dbrevs themselves, except in cases where special error handling is actually needed. Otherwise, it's just clashing with the error handling upstream. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #17 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- (In reply to Julian Maurice from comment #16)

So... should SQL14 be removed ? Why was it added by the way ? The wiki doesn't explain why it's recommended.

We have some notes about it here - https://koha-hedgedoc.servers.llownd.net/Development_IRC_meeting_31_July_202... It started with discussion about making a coding guideline for using the new "say" functions to produce colored outputs in dbrevs, and evolved into error reporting. I think no one remembered/realized at the time that we do have some error handling and reporting upstream. Yes, I do think SQL14 should be removed (or replaced with something else - I think we were generally in favor of the colored outputs guideline). I'll put a note on it linking to this discussion for now, and add it onto the agenda for the next dev meeting. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #19 from Emily Lamancusa (emlam) <emily.lamancusa@montgomerycountymd.gov> --- Yeah, there's definitely room for developer choice here! My main concern with having try/catch in skeleton.pl (which I'll admit I haven't articulated well) is that if we don't catch the exception at all, the upstream methods will catch and handle it, but if we catch it "wrong", we can actively break the upstream error handling. (e.g. exit(1) breaks both error and success reporting in CLI; catching an error that should stop the database upgrade and forgetting to rethrow means it will not stop the upgrade anymore; etc) Since a "wrong" try/catch can be worse than no try/catch, I think it's better for maintainability if developers make the active choice to catch the exception when they do have a reason to do special handling, rather than have the try/catch in the template by default. I'd certainly agree with keeping both templates on the wiki as a "simple mode" vs "advanced mode" choice for developers, though! Or even both in the template if you think there's a benefit to doing it that way. Either way, we should also document the behavior when the exception isn't caught. :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com --- Comment #20 from Jonathan Druart <jonathan.druart@gmail.com> --- I've lost track of this, sorry. I am not sure it's INVALID. Why not iterating again? Why not simply rethrowing the exception? use Modern::Perl; use Try::Tiny; use Koha::Installer::Output qw(say_warning say_failure say_success say_info); return { bug_number => "12345", description => "Do stuffs with biblios", up => sub { my ($args) = @_; my ( $dbh, $out ) = @$args{qw(dbh out)}; try { $dbh->do(q{SELECT COUNT(*) FROM biblio;}); say_success( $out, "Retrieved number of biblios"); } catch { say_failure( $out, "Cannot get count of biblios" ); $_->rethrow; }; try { $dbh->do(q{ALTER TABLE biblio ADD COLUMN biblionumber INT(11);}); say_success( $out, "Added biblio.biblionumber"); } catch { say_failure( $out, "Cannot add column biblio.biblionumber" ); $_->rethrow; }; say_success( $out, "Never reached"); }, }; This will work: $ updatedatabase DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_12345.pl [09:18:16]: Bug 12345 - Do stuffs with biblios Retrieved number of biblios Cannot add column biblio.biblionumber ERROR - C4::Installer::try {...} (): DBI Exception: DBD::mysql::db do failed: Duplicate column name 'biblionumber' at /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_12345.pl line 26 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |In Discussion -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #22 from Jonathan Druart <jonathan.druart@gmail.com> --- The UI does not like it however: https://snipboard.io/IGiKgM.jpg Wondering if it's not broken in main already... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #21)

I've read the previous again, but I still think we should have a good example of try catch in the skeleton file. We could also enforce the existing of rethrow in the catch block for db_revs. SQL14 have been removed, so try/catch is not mandatory. If one chooses to use it, there must be a good reason for it and I think we shouldn't enforce any behavior in the catch block. It could be ok for an SQL query to fail in that case.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #25 from Jonathan Druart <jonathan.druart@gmail.com> --- Also some (all?) say_failure statements will never been reached as DBI has RaiseError turned on, so installer/data/mysql/db_revs/240600029.pl 34 if ( $dbh->do("$creation_date_statement AFTER `end_date`") ) { 35 say_success( $out, q{Added column 'bookings.creation_date'} ); 36 } else { 37 say_failure( $out, q{Failed to add column 'bookings.creation_date': } . $dbh->errstr ); 38 } This is useless code if not better written (with try-catch + rethrow). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 --- Comment #27 from Jonathan Druart <jonathan.druart@gmail.com> --- I've opened bug 38394 to remove the existing try/catch and say_failure statements. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |ASSIGNED Keywords| |rel_25_05_candidate Assignee|kyle@bywatersolutions.com |jonathan.druart@gmail.com Depends on| |38394 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38394 [Bug 38394] Remove try/catch and say_failures for 24.11 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37895 Bug 37895 depends on bug 38394, which changed state. Bug 38394 Summary: Remove try/catch and say_failures for 24.11 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38394 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.