Bug 16070 - Empty (undef) system preferences may cause some issues in combination with memcache
Summary: Empty (undef) system preferences may cause some issues in combination with me...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11998
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-14 17:32 UTC by Jacek Ablewicz
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16070: Default value for sysprefs should be an empty string (10.54 KB, patch)
2016-03-15 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16070: Default value for sysprefs should be an empty string (10.58 KB, patch)
2016-03-16 04:57 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 16070: Add atomic update file (691 bytes, patch)
2016-03-17 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16070: Default value for sysprefs should be an empty string (10.64 KB, patch)
2016-03-24 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16070: Add atomic update file (746 bytes, patch)
2016-03-24 19:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jacek Ablewicz 2016-03-14 17:32:05 UTC
See Bug 11998 comment #113
Comment 1 Jonathan Druart 2016-03-15 09:54:35 UTC
bug 11998 comment 113:
"""
For sysprefs with undef value in the database - while testing with memcache - there are 2 warnings generated:

Use of uninitialized value in subroutine entry at /home/koha/devkohaclone/Koha/Cache.pm line 287.
Use of uninitialized value in subroutine entry at /home/koha/devkohaclone/Koha/Cache.pm line 287.

This only happens when trying to fetch such syspref first time, from unpopulated cache. There are 9 such values in my test database:

   CoceHost
   CoceProviders
   DefaultLongOverdueChargeValue
   DefaultLongOverdueDays
   DefaultLongOverdueLostValue
   MembershipExpiryDaysNotice
   NovelistSelectPassword
   NovelistSelectProfile
   TagsExternalDictionary

Non-existing preference fetch generates the same warning (but again only once).

Note that subsequent C4::Context->preference() calls will return empty string for such preferences, instead of undef value - not sure if that may cause some problems or not, but it's a change from previous behaviour. Also it does not happen for Cache::Memory (= default caching system after this patch).
"""
Comment 2 Jonathan Druart 2016-03-15 10:00:44 UTC
In the case of sysprefs, it does not make sense to have them with an undefined value. They should be updated.
Comment 3 Jonathan Druart 2016-03-15 10:09:05 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2016-03-16 04:57:02 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-03-17 10:58:46 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-03-17 10:59:06 UTC
I forgot the atomic update file!
Comment 7 Tomás Cohen Arazi 2016-03-24 19:15:09 UTC
Created attachment 49550 [details] [review]
Bug 16070: Default value for sysprefs should be an empty string

The default value for sysprefs should not be NULL but an empty string.
When a pref is cleared, it's set to an empty string, so it does not make
sense to create it with an undefined value.

The main purpose of this patch is to remove the warning in logs when a
pref is accessed for the first time and the cache is not yet populated.
It also ensures that the behavior will be the same for the first access
and the others.

Test plan:
SELECT COUNT(*) FROM systempreferences WHERE value IS NULL;
Should not return any results after the update DB entry executed.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Tomás Cohen Arazi 2016-03-24 19:15:16 UTC
Created attachment 49551 [details] [review]
Bug 16070: Add atomic update file

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Brendan Gallagher 2016-03-31 20:00:58 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 10 Mason James 2016-08-28 01:03:18 UTC
Added to Koha 16.05.00 release