Bug 25026

Yes! I am 100% in favour of turning on RaiseError for the UI. 

For what it's worth, I don't think that it will be a huge drama. 

When running on openSUSE, we have turned on RaiseError on the staff interface for many years now, and while there is an occasional fatal software error, which I try to clean up and upstream, for the most part it's quite stable.

(In reply to David Cook from comment #1)
> Yes! I am 100% in favour of turning on RaiseError for the UI. 
> 
> For what it's worth, I don't think that it will be a huge drama. 
> 
> When running on openSUSE, we have turned on RaiseError on the staff
> interface for many years now, and while there is an occasional fatal
> software error, which I try to clean up and upstream, for the most part it's
> quite stable.

Plus, we could look at moving forward on Bug 26048, so that any errors in the UI are much more user friendly.

Note that Bug 26853 is an example where RaiseError would be really helpful because it would help surface edge cases that otherwise fail with silent errors.

It would be nice to have bug 26048 before we turn this on.

Created attachment 114059 [details] [review]
Bug 25026: Turn on RaiseError

This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that
unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening.
The change goes to the right direction, but we need to know how big is the task
to have a stable release with these parameters correctly set.

We will have a lot of fun once this patch is pushed! :)

Test plan:
0. Does not apply the patch
1. Add, somewhere in mainpage.pl
  C4::Acquisition::NewBasket();
2. restart_all
3. Hit the mainpage
=> Hum... nothing happens! However there is an error in the logs
 DBI Exception: DBD::mysql::db do failed: Column 'booksellerid' cannot be null
No basket has been created!
4. Apply the patch, restart_all
5. Hit the mainpage
=> Here we go, there was something wrong and the user is aware of that!

(In reply to Jonathan Druart from comment #4)
> It would be nice to have bug 26048 before we turn this on.

Very true

diff --git a/populate_db.pl b/populate_db.pl
index 097bbda..7d07aa6 100755
--- a/populate_db.pl
+++ b/populate_db.pl
@@ -92,8 +92,8 @@ if (     $marcflavour ne 'MARC21'
 
 my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
 
-my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM systempreferences|);
-my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM borrowers|);
+my ( $prefs_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM systempreferences|) };
+my ( $patrons_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM borrowers|) };
 if ( $prefs_count or $patrons_count ) {
     die "Database is not empty!";
 }

You will need to apply this to koha-misc4dev first.

Created attachment 114106 [details] [review]
Bug 25026: Turn on RaiseError

This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that
unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening.
The change goes to the right direction, but we need to know how big is the task
to have a stable release with these parameters correctly set.

We will have a lot of fun once this patch is pushed! :)

Test plan:
0. Does not apply the patch
1. Add, somewhere in mainpage.pl
  C4::Acquisition::NewBasket();
2. restart_all
3. Hit the mainpage
=> Hum... nothing happens! However there is an error in the logs
 DBI Exception: DBD::mysql::db do failed: Column 'booksellerid' cannot be null
No basket has been created!
4. Apply the patch, restart_all
5. Hit the mainpage
=> Here we go, there was something wrong and the user is aware of that!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

It works! ﷐[U+1F4A5]﷑﷐[U+1F4A5]﷑﷐[U+1F4A5]﷑

About comment 7 , in koha-testing-docker, I edited misc4dev/populate_db.pl and did reset_all, was that correct?

(In reply to Jonathan Druart from comment #7)
> diff --git a/populate_db.pl b/populate_db.pl
> index 097bbda..7d07aa6 100755
> --- a/populate_db.pl
> +++ b/populate_db.pl
> @@ -92,8 +92,8 @@ if (     $marcflavour ne 'MARC21'
>  
>  my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
>  
> -my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> systempreferences|);
> -my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> borrowers|);
> +my ( $prefs_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> systempreferences|) };
> +my ( $patrons_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> borrowers|) };
>  if ( $prefs_count or $patrons_count ) {
>      die "Database is not empty!";
>  }
> 
> You will need to apply this to koha-misc4dev first.

Can you clarify why we need to do this? Is koha-misc4dev currently broken?

(In reply to Victor Grousset/tuxayo from comment #9)
> It works! ﷐[U+1F4A5]﷑﷐[U+1F4A5]﷑﷐[U+1F4A5]﷑
> 

This could give me my first opportunity to QA! Looking forward to it...

(In reply to David Cook from comment #10)
> (In reply to Jonathan Druart from comment #7)
> > diff --git a/populate_db.pl b/populate_db.pl
> > index 097bbda..7d07aa6 100755
> > --- a/populate_db.pl
> > +++ b/populate_db.pl
> > @@ -92,8 +92,8 @@ if (     $marcflavour ne 'MARC21'
> >  
> >  my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
> >  
> > -my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > systempreferences|);
> > -my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > borrowers|);
> > +my ( $prefs_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > systempreferences|) };
> > +my ( $patrons_count ) = eval { $dbh->selectrow_array(q|SELECT COUNT(*) FROM
> > borrowers|) };
> >  if ( $prefs_count or $patrons_count ) {
> >      die "Database is not empty!";
> >  }
> > 
> > You will need to apply this to koha-misc4dev first.
> 
> Can you clarify why we need to do this? Is koha-misc4dev currently broken?

Hum, good question. I did not spend much time on it, but we should not need it as we have the KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR flag set.
I will investigate a bit more and see if there is not something hidden behind it.

Created attachment 114136 [details] [review]
Bug 25026: Use HandleError to not raise or print error

DBIC has its own HandleError, and so it does not work to unset
RaiseError or PrintError if unsafe is not set (what we don't want here).

The idea of this patch is to overwrite the HandleError when we need it:
Either if KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR is set (misc4dev)
Or if the installer is triggered (we don't want to explode if the DB
does not exist).

There is an additional trick, when the installer is completed, we want
to restore the original behaviour, and so a disconnect is made.
Note that during the installer we want to display eventual SQL errors,
that's why we still display the errors.

The installer was broken!

This follow-up should do it.
We need to test `reset_all` and the install process. Also hit circ/pendingreserves.pl which should explode when the install is done (see the "trick" in the commit message)

Now the change in misc4dev is:
diff --git a/populate_db.pl b/populate_db.pl
index 097bbda..563cb28 100755
--- a/populate_db.pl
+++ b/populate_db.pl
@@ -92,6 +92,8 @@ if (     $marcflavour ne 'MARC21'
 
 my $dbh = C4::Context->dbh; # At the beginning to die if DB does not exist.
 
+my $HandleError = $dbh->{HandleError};
+$dbh->{HandleError} = sub { return 1 };
 my ( $prefs_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM systempreferences|);
 my ( $patrons_count ) = $dbh->selectrow_array(q|SELECT COUNT(*) FROM borrowers|);
 if ( $prefs_count or $patrons_count ) {


To hide the 2 errors that will be risen when the DB is empty.

I'm not sure about this change...

Why don't we just set RaiseError and PrintError in Database.pm and then deal with localizations in install.pl and misc4dev? 

For example:

"If you want to temporarily turn RaiseError off (inside a library function that is likely to fail, for example), the recommended way is like this:"
https://metacpan.org/pod/DBI#RaiseError

{
  local $h->{RaiseError};  # localize and turn off for this block
  ...
}

I'd prefer that ideally.

See the commit message, it does not work. It is what we are doing currently. Without unsafe, DBIC does not accept RaiseError turned off.

Have a look at DBIx::Class::Storage::DBI->_connect for more information.

(In reply to Jonathan Druart from comment #16)
> See the commit message, it does not work. It is what we are doing currently.
> Without unsafe, DBIC does not accept RaiseError turned off.
> 
> Have a look at DBIx::Class::Storage::DBI->_connect for more information.

I've read through the commit messages and DBIx::Class::Storage::DBI->_connect but I still don't see why it's necessary. 

It sounds like perhaps more so an issue with populate_db.pl than Koha...

The problem also exists for the install, when the tables don't exist yet.

I can try to explain again, it's as simple as: Without unsafe we cannot disable RaiseError (passing RaiseError => 0 to ->connect), because DBIC set its own HandleError (and will raise the error anyway, because it considers it's the right thing to do, unless you passed unsafe => 1).

The idea of this patch is the remove this HandleError and ignore the error in both case:
- KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR is set (to not have errors *displayed* on reset_all, ie. misc4dev/do_all_you_can_do.pl)
- we are hitting the installer for the first time

I have not retested, but I don't think misc4dev could deal with the replacement of the HandleError, $dbh is init at compilation and that's why we have KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR in the BEGIN block.
Even if we could, I don't think we should drop KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR's behaviour without deprecation notice.

Applied the patch, reset_all => multiple errors
Was another step needed?

root@kohadevbox:kohadevbox$ reset_all
This action will erase all your data. Are you sure? (y/N) y

Running [sudo koha-shell kohadev -p -c 'PERL5LIB=/kohadevbox/koha:/kohadevbox/qa-test-tools perl /kohadevbox/misc4dev/populate_db.pl -v --opac-base-url http://kohadev.myDNSname.org:8080 --intranet-base-url http://kohadev-intra.myDNSname.org:8081 --marcflavour MARC21']...
{UNKNOWN}: DBI Exception: DBD::mysql::db selectrow_array failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT COUNT(*) FROM systempreferences"]  at /usr/share/perl5/DBIx/Class/Schema.pm line 1118.
	DBIx::Class::Schema::throw_exception(Koha::Schema=HASH(0x556066af02d8), "DBI Exception: DBD::mysql::db selectrow_array failed: Table '"...) called at /usr/share/perl5/DBIx/Class/Storage.pm line 113
	DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::mysql=HASH(0x5560667c3cc8), "DBI Exception: DBD::mysql::db selectrow_array failed: Table '"...) called at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1501
	DBIx::Class::Storage::DBI::__ANON__("DBD::mysql::db selectrow_array failed: Table 'koha_kohadev.sy"..., DBI::db=HASH(0x556066f57c30), undef) called at /kohadevbox/misc4dev/populate_db.pl line 95

Restarting Apache httpd web server: apache2.
Restarting Koha ILS: koha-commonStopping Plack daemon for kohadev:.
Stopping Z39.50/SRU daemon for kohadev:.
Stopping Koha worker daemon for kohadev:.
Stopping Koha indexing daemon for kohadev:.
Starting Plack daemon for kohadev:.
Starting Z39.50/SRU daemon for kohadev:DBD::mysql::st execute failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='casauthentication'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='caslogout'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_kohadev.systempreferences' doesn't exist [for Statement "SELECT `me`.`variable`, `me`.`value`, `me`.`options`, `me`.`explanation`, `me`.`type` FROM `systempreferences` `me` WHERE ( `me`.`variable` = ? )" with ParamValues: 0='casserverurl'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::db selectall_arrayref failed: Table 'koha_kohadev.marc_subfield_structure' doesn't exist [for Statement "
        SELECT *
        FROM marc_subfield_structure
        WHERE frameworkcode = ?
        AND kohafield > ''
        ORDER BY frameworkcode,tagfield,tagsubfield
    "] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st execute failed: Table 'koha_kohadev.authorised_values' doesn't exist [for Statement "
        SELECT DISTINCT av.*
        FROM authorised_values av
     WHERE category = ? ORDER BY category, lib, lib_opac" with ParamValues: 0='Z3950_STATUS'] at /kohadevbox/koha/Koha/Database.pm line 124.
DBD::mysql::st fetchrow_hashref failed: fetch() without execute() [for Statement "
        SELECT DISTINCT av.*
        FROM authorised_values av
     WHERE category = ? ORDER BY category, lib, lib_opac" with ParamValues: 0='Z3950_STATUS'] at /kohadevbox/koha/Koha/Database.pm line 124.
.
Starting Koha worker daemon for kohadev:.
Starting Koha indexing daemon for kohadev:.

(In reply to Victor Grousset/tuxayo from comment #19)
> Applied the patch, reset_all => multiple errors
> Was another step needed?

Yes, see comment 14.

(In reply to Jonathan Druart from comment #20)
> (In reply to Victor Grousset/tuxayo from comment #19)
> > Applied the patch, reset_all => multiple errors
> > Was another step needed?
> 
> Yes, see comment 14.

Why don't we just use the first patch here and then update populate_db.pl to trap the fatal errors like in comment 7?

populate_db.pl could be updated before even testing this patch.

Am I missing something?

(In reply to David Cook from comment #21)
> (In reply to Jonathan Druart from comment #20)
> > (In reply to Victor Grousset/tuxayo from comment #19)
> > > Applied the patch, reset_all => multiple errors
> > > Was another step needed?
> > 
> > Yes, see comment 14.
> 
> Why don't we just use the first patch here and then update populate_db.pl to
> trap the fatal errors like in comment 7?

I am not sure I understand exactly what you mean. If you don't remove/disable the error handler when the dbh is created, you will have to eval all the queries executed before the installer will create the DB.

> populate_db.pl could be updated before even testing this patch.

Yes, maybe, but I wanted it to be approved before modifying it.

(In reply to Jonathan Druart from comment #22)
> 
> I am not sure I understand exactly what you mean. If you don't
> remove/disable the error handler when the dbh is created, you will have to
> eval all the queries executed before the installer will create the DB.
> 

Yeah, that's what I meant. I think that's a reasonable thing to do. 

> > populate_db.pl could be updated before even testing this patch.
> 
> Yes, maybe, but I wanted it to be approved before modifying it.

That's understandable too. It just makes it less pleasant to test here. Not the end of the world though.

(In reply to David Cook from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > 
> > I am not sure I understand exactly what you mean. If you don't
> > remove/disable the error handler when the dbh is created, you will have to
> > eval all the queries executed before the installer will create the DB.
> > 
> 
> Yeah, that's what I meant. I think that's a reasonable thing to do. 

I don't think so. It seems much more sensible to me to know it and do what's needed when the dbh is created.

It's also following the current behaviour.

Created attachment 116206 [details] [review]
Bug 25026: Turn on RaiseError

This is a follow-up for bug 22001.

The patch "Bug 22001: Set unsafe only if RaiseError is not set" proved that
unsafe => 1 and RaiseError => 0 was wrong.
We should move forward and turn RaiseError for the UI, and... see what's happening.
The change goes to the right direction, but we need to know how big is the task
to have a stable release with these parameters correctly set.

We will have a lot of fun once this patch is pushed! :)

Test plan:
0. Does not apply the patch
1. Add, somewhere in mainpage.pl
  C4::Acquisition::NewBasket();
2. restart_all
3. Hit the mainpage
=> Hum... nothing happens! However there is an error in the logs
 DBI Exception: DBD::mysql::db do failed: Column 'booksellerid' cannot be null
No basket has been created!
4. Apply the patch, restart_all
5. Hit the mainpage
=> Here we go, there was something wrong and the user is aware of that!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 116207 [details] [review]
Bug 25026: Use HandleError to not raise or print error

DBIC has its own HandleError, and so it does not work to unset
RaiseError or PrintError if unsafe is not set (what we don't want here).

The idea of this patch is to overwrite the HandleError when we need it:
Either if KOHA_DB_DO_NOT_RAISE_OR_PRINT_ERROR is set (misc4dev)
Or if the installer is triggered (we don't want to explode if the DB
does not exist).

There is an additional trick, when the installer is completed, we want
to restore the original behaviour, and so a disconnect is made.
Note that during the installer we want to display eventual SQL errors,
that's why we still display the errors.

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

This is a positive change to move us in the right direction for catching errors.

I'm sure there will be a little fallout, but I'm also confident fixes will come quickly.

As such.. I'm happy to PQA on this.. it works as I expect and is a positive move in the right direction.

Humm.. that second patch isn't working the way I expected actually.. the errors are causing script death in the installer

(In reply to Martin Renvoize from comment #28)
> Humm.. that second patch isn't working the way I expected actually.. the
> errors are causing script death in the installer

If it's about 0000-00-00 dates, then see bug 26997. We dependent on it.

(In reply to Jonathan Druart from comment #29)
> (In reply to Martin Renvoize from comment #28)
> > Humm.. that second patch isn't working the way I expected actually.. the
> > errors are causing script death in the installer
> 
> If it's about 0000-00-00 dates, then see bug 26997. We dependent on it.

* We can depend on it

Skipping QA as we must push this ASAP.

Pushed to master for 21.05.

Do you think this can be backported to 20.11.x ?

(In reply to Fridolin Somers from comment #33)
> Do you think this can be backported to 20.11.x ?

No, absolutely not! :)

(In reply to Jonathan Druart from comment #34)
> (In reply to Fridolin Somers from comment #33)
> > Do you think this can be backported to 20.11.x ?
> 
> No, absolutely not! :)

OK.
Enhancement not pushed to 20.11.x