Bug 16519 - Do not use global variables in [opac-]addbybiblionumbers.pl
Summary: Do not use global variables in [opac-]addbybiblionumbers.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 16517 16518
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-13 19:57 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

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


Attachments
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl (19.29 KB, patch)
2016-05-13 20:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl (19.29 KB, patch)
2016-05-25 16:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl (19.43 KB, patch)
2016-05-28 19:12 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl (19.50 KB, patch)
2016-09-18 20:04 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-05-13 19:57:38 UTC
To avoid bug like bug 16518 and to ease the readability/maintainability of these scripts, it would be great to see them rewritten not to use global variables.
Comment 1 Jonathan Druart 2016-05-13 20:02:23 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-05-25 14:08:42 UTC
Sorry, QA tools complain:
 FAIL	opac/opac-addbybiblionumber.pl
   FAIL	  valid
		Global symbol "$bib" requires explicit package name 
		opac/opac-addbybiblionumber.pl had compilation errors.
		Found = in conditional, should be == 
		Can't modify scalar in scalar assignment
Comment 3 Jonathan Druart 2016-05-25 16:10:55 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-05-25 16:11:48 UTC
Shame on me, sorry about that.
I have decided to rename the $bib var after my tests, but I have missed one!
Comment 5 Marc Véron 2016-05-28 19:12:09 UTC
Created attachment 51903 [details] [review]
Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl

To avoid bug like bug 16518 and to ease the readability/maintainability
of these scripts, this patch replaces the use of 'our' with 'my' to
avoid the use of global variables.

Basically the code has been moved from subroutines to the appropriate places.

Test plan:
At the intranet and OPAC sides
1/ Add items to a list
2/ Add items to a list using an existing name
3/ Add items to a list you don't have right on it (by modifying the
biblionumber in the url)
4/ At the OPAC, use the opac-addbybiblionumber.pl without being logged
in to add items to a list

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 6 Marcel de Rooy 2016-08-19 06:19:22 UTC
Hm Not sure if you are improving the code here. Wasn't it possible to move a lot of this code to some module shared by both scripts?
Comment 7 Jonathan Druart 2016-08-19 07:38:09 UTC
(In reply to Marcel de Rooy from comment #6)
> Hm Not sure if you are improving the code here. Wasn't it possible to move a
> lot of this code to some module shared by both scripts?

With this code this script will now follow the same pattern as other scripts.
So yes, I think it improves the code.
Everything is possible :) I did not want to add too much changes just for a var scope problem.
Comment 8 Katrin Fischer 2016-09-18 20:04:18 UTC
Created attachment 55662 [details] [review]
[PASSED QA] Bug 16519: Replace 'our' with 'my' in [opac-]addbybiblionumbers.pl

To avoid bug like bug 16518 and to ease the readability/maintainability
of these scripts, this patch replaces the use of 'our' with 'my' to
avoid the use of global variables.

Basically the code has been moved from subroutines to the appropriate places.

Test plan:
At the intranet and OPAC sides
1/ Add items to a list
2/ Add items to a list using an existing name
3/ Add items to a list you don't have right on it (by modifying the
biblionumber in the url)
4/ At the OPAC, use the opac-addbybiblionumber.pl without being logged
in to add items to a list

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

Signed-off-by: Katrin Fischer  <katrin.fischer@bsz-bw.de>
Comment 9 Kyle M Hall 2016-09-27 13:06:06 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 10 Frédéric Demians 2016-09-30 09:27:29 UTC
Pushed in 16.05. Will be in 16.05.05.