Bug 40405 - systempreferences.value cannot be set to NULL
Summary: systempreferences.value cannot be set to NULL
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 40444
  Show dependency treegraph
 
Reported: 2025-07-16 09:46 UTC by Jonathan Druart
Modified: 2025-07-23 22:42 UTC (History)
1 user (show)

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


Attachments
Bug 40405: Set systempreferences.value to NOT NULL (14.87 KB, patch)
2025-07-16 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40405: Set decreaseLoanHighHolds\* to 0 (1.71 KB, patch)
2025-07-16 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40405: [DO NOT PUSH] Show the problem with C4::Context->set_preference (1.39 KB, patch)
2025-07-16 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40405: And ILLOpacUnauthenticatedRequest (1.58 KB, patch)
2025-07-17 14:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40405: Set systempreferences.value to NOT NULL (14.93 KB, patch)
2025-07-23 09:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40405: Set decreaseLoanHighHolds\* to 0 (1.77 KB, patch)
2025-07-23 09:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40405: [DO NOT PUSH] Show the problem with C4::Context->set_preference (1.46 KB, patch)
2025-07-23 09:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40405: And ILLOpacUnauthenticatedRequest (1.64 KB, patch)
2025-07-23 09:44 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-07-16 09:46:37 UTC
Some sysprefs have NULL as their default value, and systempreferences.value allows NULL values.

However C4::Context->set_preference does not unset a syspref if "undef" is passed.

Additionally some of the sysprefs that have NULL as default will generate warnings in the logs (and test suite output).

opac/opac-suggestions.pl:        for my $f ( split( /\s*\,\s*/, C4::Context->preference("OPACSuggestionUnwantedFields") ) ) {



Here is the current list:
+-------------------------------+-------+
| variable                      | value |
+-------------------------------+-------+
| decreaseLoanHighHoldsDuration | NULL  |
| decreaseLoanHighHoldsValue    | NULL  |
| ILLHiddenRequestStatuses      | NULL  |
| ILLOpacbackends               | NULL  |
| ILLSendStaffNotices           | NULL  |
| ListOwnerDesignated           | NULL  |
| MarcFieldDocURL               | NULL  |
| MaxFine                       | NULL  |
| OPACSuggestionUnwantedFields  | NULL  |
| ReportsExportLimit            | NULL  |
| SerialsSearchResultsLimit     | NULL  |
+-------------------------------+-------+
Comment 1 Jonathan Druart 2025-07-16 10:01:33 UTC
Created attachment 184129 [details] [review]
Bug 40405: Set systempreferences.value to NOT NULL

Some sysprefs have NULL as their default value, and systempreferences.value allows NULL values.

However C4::Context->set_preference does not unset a syspref if "undef" is passed.

Additionally some of the sysprefs that have NULL as default will generate warnings in the logs (and test suite output).

opac/opac-suggestions.pl:        for my $f ( split( /\s*\,\s*/, C4::Context->preference("OPACSuggestionUnwantedFields") ) ) {

Here is the current list:
+-------------------------------+-------+
| variable                      | value |
+-------------------------------+-------+
| decreaseLoanHighHoldsDuration | NULL  |
| decreaseLoanHighHoldsValue    | NULL  |
| ILLHiddenRequestStatuses      | NULL  |
| ILLOpacbackends               | NULL  |
| ILLSendStaffNotices           | NULL  |
| ListOwnerDesignated           | NULL  |
| MarcFieldDocURL               | NULL  |
| MaxFine                       | NULL  |
| OPACSuggestionUnwantedFields  | NULL  |
| ReportsExportLimit            | NULL  |
| SerialsSearchResultsLimit     | NULL  |
+-------------------------------+-------+

Test plan:
Code review required here, please confirm the above and that the
different occurrences do not use "undef" as a value.

Note that some tests now do not longer generate warnings
(eg. t/db_dependent/selenium/opac_ill_requests.t)
Comment 2 Jonathan Druart 2025-07-16 10:01:35 UTC
Created attachment 184130 [details] [review]
Bug 40405: Set decreaseLoanHighHolds\* to 0

When reviewing the different sysprefs that had "NULL" as default, I
only found those 2 occurrences that could be problematic. I think we
should default to 0 here.
Comment 3 Jonathan Druart 2025-07-16 10:03:19 UTC
Created attachment 184131 [details] [review]
Bug 40405: [DO NOT PUSH] Show the problem with C4::Context->set_preference

Apply this patch before the others and notice the failure.
Comment 4 Jonathan Druart 2025-07-17 14:27:09 UTC
Created attachment 184205 [details] [review]
Bug 40405: And ILLOpacUnauthenticatedRequest

No idea why it was not NULL in DB when I did my first SELECT
Comment 5 Martin Renvoize (ashimema) 2025-07-23 09:44:02 UTC
Created attachment 184535 [details] [review]
Bug 40405: Set systempreferences.value to NOT NULL

Some sysprefs have NULL as their default value, and systempreferences.value allows NULL values.

However C4::Context->set_preference does not unset a syspref if "undef" is passed.

Additionally some of the sysprefs that have NULL as default will generate warnings in the logs (and test suite output).

opac/opac-suggestions.pl:        for my $f ( split( /\s*\,\s*/, C4::Context->preference("OPACSuggestionUnwantedFields") ) ) {

Here is the current list:
+-------------------------------+-------+
| variable                      | value |
+-------------------------------+-------+
| decreaseLoanHighHoldsDuration | NULL  |
| decreaseLoanHighHoldsValue    | NULL  |
| ILLHiddenRequestStatuses      | NULL  |
| ILLOpacbackends               | NULL  |
| ILLSendStaffNotices           | NULL  |
| ListOwnerDesignated           | NULL  |
| MarcFieldDocURL               | NULL  |
| MaxFine                       | NULL  |
| OPACSuggestionUnwantedFields  | NULL  |
| ReportsExportLimit            | NULL  |
| SerialsSearchResultsLimit     | NULL  |
+-------------------------------+-------+

Test plan:
Code review required here, please confirm the above and that the
different occurrences do not use "undef" as a value.

Note that some tests now do not longer generate warnings
(eg. t/db_dependent/selenium/opac_ill_requests.t)

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 6 Martin Renvoize (ashimema) 2025-07-23 09:44:05 UTC
Created attachment 184536 [details] [review]
Bug 40405: Set decreaseLoanHighHolds\* to 0

When reviewing the different sysprefs that had "NULL" as default, I
only found those 2 occurrences that could be problematic. I think we
should default to 0 here.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 7 Martin Renvoize (ashimema) 2025-07-23 09:44:07 UTC
Created attachment 184537 [details] [review]
Bug 40405: [DO NOT PUSH] Show the problem with C4::Context->set_preference

Apply this patch before the others and notice the failure.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 8 Martin Renvoize (ashimema) 2025-07-23 09:44:09 UTC
Created attachment 184538 [details] [review]
Bug 40405: And ILLOpacUnauthenticatedRequest

No idea why it was not NULL in DB when I did my first SELECT

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 9 Martin Renvoize (ashimema) 2025-07-23 09:45:05 UTC
I've reviewed all the cases and agree with Jonathan here.
Comment 10 Lucas Gass (lukeg) 2025-07-23 22:42:44 UTC
Nice work everyone!

Pushed to main for 25.11