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.
Created attachment 51484 [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
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
Created attachment 51796 [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
Shame on me, sorry about that. I have decided to rename the $bib var after my tests, but I have missed one!
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>
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?
(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.
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>
Pushed to master for 16.11, thanks Jonathan!
Pushed in 16.05. Will be in 16.05.05.