Bug 15446 - Koha::Object[s]->type should be renamed to _type to avoid conflict with column name
Summary: Koha::Object[s]->type should be renamed to _type to avoid conflict with colum...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: Oldversions
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 13019 13967
Blocks: 15451 15632
  Show dependency treegraph
 
Reported: 2015-12-30 17:05 UTC by Jonathan Druart
Modified: 2018-12-16 19:43 UTC (History)
3 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:
Version(s) released in:


Attachments
Bug 15446: Add tests (907 bytes, patch)
2015-12-30 17:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15446: Rename Koha::Object[s]->type with _type (13.25 KB, patch)
2015-12-30 17:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15446: Update systempreferences rows where type=YesNo and value='' (742 bytes, patch)
2015-12-30 17:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15446: Add tests (1.02 KB, patch)
2015-12-31 10:27 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15446: Rename Koha::Object[s]->type with _type (13.50 KB, patch)
2015-12-31 10:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15446: Update systempreferences rows where type=YesNo and value='' (882 bytes, patch)
2015-12-31 10:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15446: Update systempreferences rows where type=YesNo and value='' (921 bytes, patch)
2015-12-31 10:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type (4.76 KB, patch)
2016-01-05 15:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15446: Add tests (1.07 KB, patch)
2016-01-08 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: Rename Koha::Object[s]->type with _type (13.44 KB, patch)
2016-01-08 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: Update systempreferences rows where type=YesNo and value='' (971 bytes, patch)
2016-01-08 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type (4.82 KB, patch)
2016-01-08 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: Add tests (1.07 KB, patch)
2016-02-12 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: Rename Koha::Object[s]->type with _type (13.43 KB, patch)
2016-02-12 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: Update systempreferences rows where type=YesNo and value='' (971 bytes, patch)
2016-02-12 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type (4.27 KB, patch)
2016-02-12 18:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15446 [QA Followup] - Update new classes (1.99 KB, patch)
2016-02-12 19:10 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 Jonathan Druart 2015-12-30 17:05:43 UTC
In a few case (at least systempreferences and export_format (csv profiles), the type method of Koha::Object and Koha::Objects can be in conflict with the column names.
Indeed systempreferences.type exists and so the method will return 'Systempreference' (the name of the module) instead of the value of the row in DB.

I have found at least 1 place where it can cause issue:
In C4::Context->set_preference:
 601     my $syspref = Koha::Config::SysPrefs->find( $var );
 602     my $type = $syspref ? $syspref->type() : undef;
 603 
 604     $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' );

type will always be 'Systempreference' and the YesNo pref will be set to an empty string '' instead of 0.
Comment 1 Jonathan Druart 2015-12-30 17:22:54 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-12-30 17:22:57 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-12-30 17:23:00 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2015-12-31 10:27:41 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-12-31 10:30:30 UTC Comment hidden (obsolete)
Comment 6 Marc Véron 2015-12-31 10:33:46 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2015-12-31 10:50:19 UTC
Oh, I was to fast with the 3rd patch, the update statement has a typo:

UPDATE systempreferences SET value="0" where type="YesNo" and value=";"
Comment 8 Marc Véron 2015-12-31 10:53:37 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-12-31 10:54:35 UTC
...and tested, update works fine now.
Comment 10 Marc Véron 2015-12-31 11:27:51 UTC
Oh, I get a software error with checkong out. Not sure if it is related to this bug, but setting back to NSO.

Will go to a previous VM to test again.
Comment 11 Jonathan Druart 2015-12-31 11:46:59 UTC
(In reply to Marc Véron from comment #10)
> Oh, I get a software error with checkong out. Not sure if it is related to
> this bug, but setting back to NSO.
> 
> Will go to a previous VM to test again.

See third patch on bug 15344 for a fix.
Comment 12 Marc Véron 2015-12-31 14:04:38 UTC
Switching back to signed off again, the issue from comment #10 was not related to this bug.
Comment 13 Jonathan Druart 2016-01-05 15:26:45 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2016-01-08 12:51:44 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2016-01-08 12:51:50 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2016-01-08 12:51:53 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2016-01-08 12:51:55 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-01-12 16:59:16 UTC
I have added a new rule to the qa tools:

commit 75bf0eead3fab5b1918f4d2f2fc6c1bc66f2dc76
    type subroutine should be _type   
    See bug 15446
Comment 19 Kyle M Hall 2016-02-12 18:59:19 UTC
Created attachment 48005 [details] [review]
Bug 15446: Add tests

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2016-02-12 18:59:40 UTC
Created attachment 48006 [details] [review]
Bug 15446: Rename Koha::Object[s]->type with _type

In a few case (at least systempreferences and export_format (csv profiles),
the type method of Koha::Object and Koha::Objects can be in conflict with the
column names.
Indeed systempreferences.type exists and so the method will return
'Systempreference' (the name of the module) instead of the value of the row in
DB.

I have found at least 1 place where it can cause issue:
In C4::Context->set_preference:
 601     my $syspref = Koha::Config::SysPrefs->find( $var );
 602     my $type = $syspref ? $syspref->type() : undef;
 603
 604     $value = 0 if ( $type && $type eq 'YesNo' && $value eq '' );

type will always be 'Systempreference' and the YesNo pref will be set to an
empty string '' instead of 0.

I am not sure about the consequences of this, but it is preferable to
fix it ASAP.

To reproduce:
0/ Do not apply this patch
1/ Edit a YesNo prefs, AutoEmailOpacUser for instance
2/ Set it to "Don't sent"
3/ Check the value in DB, it should be set to an empty string, instead
of 0
4/ Apply this patch and try again. Now the value should be 0

Followed test plan, value is now 0 as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2016-02-12 18:59:46 UTC
Created attachment 48007 [details] [review]
Bug 15446: Update systempreferences rows where type=YesNo and value=''

Signed-off-by: Marc Véron <veron@veron.ch>

Amended to fix typo in sql statement.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2016-02-12 18:59:52 UTC
Created attachment 48008 [details] [review]
Bug 15446: (follow-up) Rename Koha::Object[s]->type with _type

Some occurrences have been pushed in the meantime.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2016-02-12 19:10:34 UTC
Created attachment 48019 [details] [review]
Bug 15446 [QA Followup] - Update new classes
Comment 24 Jesse Weaver 2016-02-12 23:48:34 UTC
Pushed to master, thanks for your work.
Comment 25 Julian Maurice 2016-03-14 10:21:32 UTC
Patches pushed to 3.22.x, will be in 3.22.5

Note: 4th and 5th patches are not needed in 3.22.x, so they are not pushed