Bug 20886 - Koha::Object::TO_JSON indiscriminately casting to integer
Summary: Koha::Object::TO_JSON indiscriminately casting to integer
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 20797
  Show dependency treegraph
 
Reported: 2018-06-06 10:15 UTC by Andrew Isherwood
Modified: 2022-11-11 10:33 UTC (History)
6 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 20886: Do not cast undef to 0 (TO_JSON) (2.25 KB, patch)
2018-06-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20886: (sign-off) Koha::Object::TO_JSON indiscriminately casting to integer (2.33 KB, patch)
2018-06-07 09:50 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 20886: Do not cast undef to 0 (TO_JSON) (2.40 KB, patch)
2018-06-07 13:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20886: Do not cast undef to 0 (TO_JSON) (2.58 KB, patch)
2018-06-07 14:16 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 Andrew Isherwood 2018-06-06 10:15:00 UTC
As noted in bug 20797 there appears to be a bug in Koha::Object::TO_JSON. Specifically:

elsif ( _numeric_column_type( $columns_info->{$col}->{data_type} ) ) {

    # TODO: Remove once the solution for
    # https://rt.cpan.org/Ticket/Display.html?id=119904
    # is ported to whatever distro we support by that time
    $unblessed->{$col} += 0;
}

Here we are explicitly casting to an integer to workaround an upstream bug. This works fine unless the data in question comes from a numeric type column but is NULL, in which case it is cast to 0.

I suspect the desired behaviour here is for it to remain unaltered unless the value is also numeric.
Comment 1 Andrew Isherwood 2018-06-06 10:16:07 UTC
CC'ing Tomas in at JD's suggestion
Comment 2 Tomás Cohen Arazi 2018-06-06 12:31:54 UTC
(In reply to Andrew Isherwood from comment #1)
> CC'ing Tomas in at JD's suggestion

You are right. Good catch!
Comment 3 Jonathan Druart 2018-06-06 14:13:31 UTC
Created attachment 75857 [details] [review]
Bug 20886: Do not cast undef to 0 (TO_JSON)

We should not cast undefined values to 0.
Comment 4 Jonathan Druart 2018-06-06 14:16:00 UTC
Any side-effects to expect with this change?
Comment 5 Andrew Isherwood 2018-06-07 09:50:11 UTC
Created attachment 75869 [details] [review]
Bug 20886: (sign-off) Koha::Object::TO_JSON indiscriminately casting to integer

Works as advertised for me.
Comment 6 Marcel de Rooy 2018-06-07 13:28:00 UTC
Created attachment 75875 [details] [review]
Bug 20886: Do not cast undef to 0 (TO_JSON)

We should not cast undefined values to 0.

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Jonathan Druart 2018-06-07 14:06:17 UTC
We need Tomas's stamp on this one
Comment 8 Tomás Cohen Arazi 2018-06-07 14:16:30 UTC
Created attachment 75883 [details] [review]
Bug 20886: Do not cast undef to 0 (TO_JSON)

We should not cast undefined values to 0.

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This was a bug I introduced. I think the fix is right, and if something
gets broken for this, it means it was making the wrong assumptions on
the data.
Comment 9 Nick Clemens 2018-06-08 13:54:08 UTC
Pushed to master for 18.11, awesome work all!
Comment 10 Martin Renvoize 2018-06-08 14:39:56 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01
Comment 11 Fridolin Somers 2018-06-14 13:31:37 UTC Comment hidden (obsolete)
Comment 12 Fridolin Somers 2018-06-14 13:32:31 UTC
Is this also for 17.11.x ?
It looks similar to Bug 18330