Bug 14637 - Add patron category fails with MySQL 5.6.26
Summary: Add patron category fails with MySQL 5.6.26
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2015-08-03 11:46 UTC by Petter Goksøyr Åsen
Modified: 2017-05-15 10:01 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14637: Fix add patron category under MySQL 5.7 - tests (1.41 KB, patch)
2016-09-06 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14637: Fix add patron category under MySQL 5.7 (1.45 KB, patch)
2016-09-06 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14637: Fix add patron category under MySQL 5.7 - tests (1.27 KB, patch)
2016-10-19 20:16 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14637: Fix add patron category under MySQL 5.7 - tests (1.27 KB, patch)
2016-10-19 20:23 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14637: Fix add patron category under MySQL 5.7 (1.52 KB, patch)
2016-10-19 20:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14637: Followup - Fix number of tests due to rebase (693 bytes, patch)
2016-10-19 20:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14637: Fix add patron category under MySQL 5.7 - tests (1.33 KB, patch)
2016-12-23 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14637: Fix add patron category under MySQL 5.7 (1.71 KB, patch)
2016-12-23 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14637: Followup - Fix number of tests due to rebase (749 bytes, patch)
2016-12-23 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Petter Goksøyr Åsen 2015-08-03 11:46:10 UTC
Cannot add patron category in web interface (/cgi-bin/koha/admin/categorie.pl?op=add_form). It fails with "An error occurred when inserting this patron category. The patron category might already exist." 

The logs wil show:
categorie.pl: DBD::mysql::st execute failed: Incorrect date value: '' for column 'enrolmentperioddate' at row 1 at /usr/share/koha/intranet/cgi-bin/admin/categorie.pl line 231., referer: http://192.168.50.12:8081/cgi-bin/koha/admin/categorie.pl?op=add_form

Disabling STRICT_TRANS_TABLES in MySQL will fix the problem, but as the setting is default now, we should have a permanent fix.

This is a similar problem as bug #13523.
Comment 1 Eric Bégin 2016-01-24 18:13:58 UTC
I encounter this problem for a client that moved his Koha database on MySQL 5.6.

A temporary fix I did was to force the sql_mode to NO_ENGINE_SUBSTITUTION (removing the STRICT_TRANS_TABLES) on the connection time.

I added the following line in Context.pm

$dbo->do("set SESSION sql_mode='NO_ENGINE_SUBSTITUTION'");
Comment 2 Jonathan Druart 2016-01-25 11:46:28 UTC
You can do the same without modifying the source code, just comment the sql_mode=STRICT_TRANS_TABLES line in your MySQL/MariaDB config file.
Comment 3 Jonathan Druart 2016-09-06 09:47:07 UTC
Created attachment 55232 [details] [review]
Bug 14637: Fix add patron category under MySQL 5.7 - tests
Comment 4 Jonathan Druart 2016-09-06 09:47:10 UTC
Created attachment 55233 [details] [review]
Bug 14637: Fix add patron category under MySQL 5.7

If no dateofbirthrequired or upperagelimit is set on the interface, the
->store method will receive an empty string defined for these values.
For INT field, we must explicitely set these empty value to undef
instead to avoid MySQL 5.7 (and strict mode) to raise:
  DBD::mysql::st execute failed: Incorrect integer value: ' for column
'dateofbirthrequired''

Test plan:
Using MySQL 5.7 (and/or sql_mode=STRICT_TRANS_TABLES)
Create a patron category without specifying upperagelimit or
dateofbirthrequired
Comment 5 Jonathan Druart 2016-09-06 12:44:03 UTC
I am still not sure if this is the way to go.
Actually update_or_insert should set the default value defined at the DB level if the field is not passed to the constructor.
Comment 6 Josef Moravec 2016-10-19 20:16:48 UTC
Created attachment 56674 [details] [review]
[SIGNED-OFF] Bug 14637: Fix add patron category under MySQL 5.7 - tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2016-10-19 20:23:26 UTC
Created attachment 56675 [details] [review]
[SIGNED-OFF] Bug 14637: Fix add patron category under MySQL 5.7 - tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Josef Moravec 2016-10-19 20:23:38 UTC
Created attachment 56676 [details] [review]
[SIGNED-OFF] Bug 14637: Fix add patron category under MySQL 5.7

If no dateofbirthrequired or upperagelimit is set on the interface, the
->store method will receive an empty string defined for these values.
For INT field, we must explicitely set these empty value to undef
instead to avoid MySQL 5.7 (and strict mode) to raise:
  DBD::mysql::st execute failed: Incorrect integer value: ' for column
'dateofbirthrequired''

Test plan:
Using MySQL 5.7 (and/or sql_mode=STRICT_TRANS_TABLES)
Create a patron category without specifying upperagelimit or
dateofbirthrequired

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Josef Moravec 2016-10-19 20:23:45 UTC
Created attachment 56677 [details] [review]
Bug 14637: Followup - Fix number of tests due to rebase
Comment 10 Josef Moravec 2016-10-19 20:24:13 UTC
So it looks like this kind of issue could be in more places, is it true? 

I agree with you, that it would be better to fix it generally, but I have no idea, how to do it... 

But if we'd go with this solution, I am adding sign-off as it works as described.
Comment 11 Jonathan Druart 2016-10-20 06:52:43 UTC
(In reply to Josef Moravec from comment #10)
> So it looks like this kind of issue could be in more places, is it true? 
> 
> I agree with you, that it would be better to fix it generally, but I have no
> idea, how to do it... 
> 
> But if we'd go with this solution, I am adding sign-off as it works as
> described.

Could you prove the tests from this bug without the changes to Koha::Patron::Category->store and apply bug 17080 and see if they pass with MySQL5.7?
Comment 12 Josef Moravec 2016-10-20 08:48:46 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Josef Moravec from comment #10)
> > So it looks like this kind of issue could be in more places, is it true? 
> > 
> > I agree with you, that it would be better to fix it generally, but I have no
> > idea, how to do it... 
> > 
> > But if we'd go with this solution, I am adding sign-off as it works as
> > described.
> 
> Could you prove the tests from this bug without the changes to
> Koha::Patron::Category->store and apply bug 17080 and see if they pass with
> MySQL5.7?

I tried, but unfortunately, the test failed with this error message:

t/db_dependent/Koha/Patron/Categories.t .. 1/8 DBD::mysql::st execute failed: Incorrect integer value: '' for column 'dateofbirthrequired' at row 1 [for Statement "INSERT INTO `categories` ( `category_type`, `categorycode`, `dateofbirthrequired`, `description`, `upperagelimit`) VALUES ( ?, ?, ?, ?, ? )" with ParamValues: 0='A', 1='mycatcodeW', 2='', 3='mycatdescW', 4=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect integer value: '' for column 'dateofbirthrequired' at row 1 at /home/vagrant/kohaclone/Koha/Object.pm line 119
# Looks like your test exited with 255 just after 8.
t/db_dependent/Koha/Patron/Categories.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
All 8 subtests passed 

Test Summary Report
-------------------
t/db_dependent/Koha/Patron/Categories.t (Wstat: 65280 Tests: 8 Failed: 0)
  Non-zero exit status: 255
Files=1, Tests=8,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.51 cusr  0.27 csys =  0.79 CPU)
Result: FAIL


Maybe I unterstand it wrong but 17080 handles NOT NULL columns and upperagelimit and dateofbirthrequired are nullable...
Comment 13 Jonathan Druart 2016-10-20 09:05:35 UTC
(In reply to Josef Moravec from comment #12)
> Maybe I unterstand it wrong but 17080 handles NOT NULL columns and
> upperagelimit and dateofbirthrequired are nullable...

Yes sorry, not enough tea this morning.
This patch makes sense but we could reuse the same logic for date and int fields (if equal '', then set to undef).
Comment 14 Kyle M Hall 2016-12-23 13:36:27 UTC
Created attachment 58416 [details] [review]
Bug 14637: Fix add patron category under MySQL 5.7 - tests

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2016-12-23 13:36:34 UTC
Created attachment 58417 [details] [review]
Bug 14637: Fix add patron category under MySQL 5.7

If no dateofbirthrequired or upperagelimit is set on the interface, the
->store method will receive an empty string defined for these values.
For INT field, we must explicitely set these empty value to undef
instead to avoid MySQL 5.7 (and strict mode) to raise:
  DBD::mysql::st execute failed: Incorrect integer value: ' for column
'dateofbirthrequired''

Test plan:
Using MySQL 5.7 (and/or sql_mode=STRICT_TRANS_TABLES)
Create a patron category without specifying upperagelimit or
dateofbirthrequired

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2016-12-23 13:36:37 UTC
Created attachment 58418 [details] [review]
Bug 14637: Followup - Fix number of tests due to rebase

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2016-12-28 13:31:48 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 18 Katrin Fischer 2017-01-03 23:36:52 UTC
These patches have been pushed to 16.11.x, will be in 16.11.02
.
Comment 19 Mason James 2017-01-27 02:36:44 UTC
Pushed to 16.05.x, for 16.05.08 release