Bug 15429 - sub _parseletter should not change referenced values
Summary: sub _parseletter should not change referenced values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Marc Véron
QA Contact: Testopia
URL:
Keywords:
: 15423 15433 (view as bug list)
Depends on: 6810
Blocks: 15466
  Show dependency treegraph
 
Reported: 2015-12-28 09:28 UTC by Marc Véron
Modified: 2019-06-27 09:24 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 15429 - sub _parseletter should not change referenced values (1.61 KB, patch)
2015-12-28 09:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15429 - sub _parseletter should not change referenced values (1.78 KB, patch)
2015-12-30 12:47 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15429 UT for _parseletter modifying its parameter (1.54 KB, patch)
2015-12-30 12:48 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 15429 - sub _parseletter should not change referenced values (1.85 KB, patch)
2015-12-30 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15429 UT for _parseletter modifying its parameter (1.62 KB, patch)
2015-12-30 13:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15429 Followup to avoid test failing (847 bytes, patch)
2015-12-31 18:38 UTC, Frédéric Demians
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-12-28 09:28:30 UTC
In C4/Letters.pm, sub _parseletter changes values that are passed by reference. E.g. patron's expiry date can change from iso format to syspref format,leading to strange behaviour in the calling routines (see Bug 15423).
Comment 1 Marc Véron 2015-12-28 09:33:50 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-12-28 09:39:46 UTC
To reproduce an issue with strange value changes (patron's expiry date) see Bug 15423.
Comment 3 Marc Véron 2015-12-28 13:31:54 UTC
*** Bug 15423 has been marked as a duplicate of this bug. ***
Comment 4 Marc Véron 2015-12-28 17:38:22 UTC
Switching to "Needs Signoff"

Test plan:
Reproduce without this patch:
- Try to repruduce issue in Bug 15423 (comment #2)
- make sure that patron with batch checkout permissionhas nothing checked out
- Go to batch checkout
- enter 2 valid barcodes
- Then enter a line with one space character
- Then enter one more empty line or a barcode
Example:
e.g.
1
2
 
3
('empty' line contains one space character)

Expected result:
Software error
The given date (13/05/2022 00:00) does not match the date format (iso) at /usr/share/kohaclone/Koha/DateUtils.pm line 153.

- If not yet done, apply patch 15432
- prove -v t/db_dependent/Letters.t
Expected Result: OK

Test with patch:
- apply patch
- try to reproduce software error above

Expected result: No software error, checkouts OK

- prove -v t/db_dependent/Letters.t
Expected result: OK


(Note: I did not manage to create a test to reproduce Bug 15423 in Letters.t)
Comment 5 Katrin Fischer 2015-12-30 08:18:15 UTC
*** Bug 15433 has been marked as a duplicate of this bug. ***
Comment 6 Marc Véron 2015-12-30 08:44:50 UTC
Setting to 'critical' because Bug 15433 (resolved by this patch) has status 'critical'
Comment 7 Frédéric Demians 2015-12-30 12:47:47 UTC Comment hidden (obsolete)
Comment 8 Frédéric Demians 2015-12-30 12:48:31 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-12-30 13:16:00 UTC
Created attachment 46076 [details] [review]
Bug 15429 - sub _parseletter should not change referenced values

In C4/Letters.pm, sub _parseletter changes values that are passed by
reference. E.g. patron's expiry date can change from iso format to
syspref format, leading to strange behaviour in the calling routines
(see Bug 15423).

This patch makes sub _parseletter work on a copy of the referenced values.

(Submitted to get feedback - is this the way to go?)

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
  Good solution to real time bomb.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2015-12-30 13:16:19 UTC
Created attachment 46077 [details] [review]
Bug 15429 UT for _parseletter modifying its parameter

Before/after applying the related patch, do:

   prove -v t/db_dependent/Letters.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2015-12-30 13:17:15 UTC
We should not do that, but it is definitely the safest and easiest way to go.
Comment 12 Jonathan Druart 2015-12-30 13:20:19 UTC
Seems to have been introduced by 6810.
Comment 13 Jonathan Druart 2015-12-30 13:25:24 UTC
(In reply to Jonathan Druart from comment #12)
> Seems to have been introduced by 6810.

waitingdate was already modified before bug 6810.
Comment 14 Kyle M Hall 2015-12-30 14:07:29 UTC
Pushed to master, thanks Marc!
Comment 15 Julian Maurice 2015-12-31 09:48:56 UTC
Patch pushed to 3.22.x, will be in 3.22.2
Comment 16 Frédéric Demians 2015-12-31 18:31:28 UTC
This patch has been pushed to 3.20.x, will be in 3.20.8.
Comment 17 Frédéric Demians 2015-12-31 18:38:36 UTC
Created attachment 46160 [details] [review]
Bug 15429 Followup to avoid test failing