strict directive is commented for tools/picture-upload.pl. This is because many vars are missing explicit package name. I propose to correct only this bug, a complete work may be done like said in FIXME in head of file.
Created attachment 14226 [details] [review] Proposed patch
Created attachment 14227 [details] [review] Proposed patch (follow-up 1) Perltidy on picture-upload.pl. (very prettier)
git am -3 /tmp/Proposed-patch-follow-up-1-ZTkJrF.patch Applying: Bug 9312: strict perl for picture-upload.pl (followup 1) fatal: sha1 information is lacking or useless (tools/picture-upload.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 9312: strict perl for picture-upload.pl (followup 1)
Created attachment 14255 [details] [review] Proposed patch (rebased master) Patch rebased master
Created attachment 14256 [details] [review] Proposed patch (follow-up 1) (rebased master) Patch rebased master
I'm not sure what's going on, but I still cannot apply the second patch. I still get the same 'sha1 information is lacking' error. (In reply to comment #5) > Created attachment 14256 [details] [review] > Proposed patch (follow-up 1) (rebased master) > > Patch rebased master
"it works for me" % git reset --hard origin/master HEAD is now at 588a55a Merge branch 'bug_8984' into 3.12-master % git bz apply 9312 Bug 9312 - strict perl for picture-upload.pl Proposed patch (rebased master) Apply? [yn] y Applying: Bug 9312: strict perl for picture-upload.pl Proposed patch (follow-up 1) (rebased master) Apply? [yn] y Applying: Bug 9312: strict perl for picture-upload.pl (followup 1)
(In reply to comment #6) > I'm not sure what's going on, but I still cannot apply the second patch. I > still get the same 'sha1 information is lacking' error. Can you test with 'git apply' ?
Created attachment 14623 [details] [review] Bug 9312: strict perl for picture-upload.pl Testing - this is a code-cleanup patch, so the purpose of testing is to ensure that patron image upload functionality still works. To test: - Turn on the patronimages system preference - Go to Tools | Upload patron images and import a suitable image for a test patron. - Bring up the test patron and verify that the image is attached. - From the patron details page, upload a replacement image. - Create a patron image ZIP file containing at least two images ( per the documentation of the patron image feature) and load it via the patron image import tool. - Verfiy that the test patrons now have images. - Verify that tools/picture-upload.pl didn't report any errors in the Apache error log. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 14624 [details] [review] Bug 9312: strict perl for picture-upload.pl Testing - this is a code-cleanup patch, so the purpose of testing is to ensure that patron image upload functionality still works. To test: - Turn on the patronimages system preference - Go to Tools | Upload patron images and import a suitable image for a test patron. - Bring up the test patron and verify that the image is attached. - From the patron details page, upload a replacement image. - Create a patron image ZIP file containing at least two images ( per the documentation of the patron image feature) and load it via the patron image import tool. - Verfiy that the test patrons now have images. - Verify that tools/picture-upload.pl didn't report any errors in the Apache error log. Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 14625 [details] [review] Bug 9312: whitespace cleanup for picture-upload.pl Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Created attachment 14626 [details] [review] Bug 9312: fix a couple more uninitialized variable warnings Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Note that in the course of signing off I added a test plan, reworded a commit message, and added a followup patch.
Patches don't apply (conflicts with bug 9421).
Created attachment 18895 [details] [review] Bug 9312: strict perl for picture-upload.pl Patch rebased master
Created attachment 18896 [details] [review] Bug 9312: whitespace cleanup for picture-upload.pl Patch rewritten. No need to rebase, it is a code formatting using perltidy.
Created attachment 18897 [details] [review] Bug 9312: fix a couple more uninitialized variable warnings Patch rebased master
All patches rebased master. There are a lot of changes so I set to needs signoff.
Comment on attachment 18895 [details] [review] Bug 9312: strict perl for picture-upload.pl Review of attachment 18895 [details] [review]: ----------------------------------------------------------------- ::: tools/picture-upload.pl @@ +19,5 @@ > # > # > > +use strict; > +use warnings; 'use Modern::Perl' is preferred for this nowadays.
Created attachment 18936 [details] [review] Bug 9312: strict perl for picture-upload.pl use of Modern::Perl
Created attachment 18937 [details] [review] Bug 9312: whitespace cleanup for picture-upload.pl
Created attachment 18938 [details] [review] Bug 9312: fix a couple more uninitialized variable warnings
opendir RECDIR, $recursive_dir is much better written as opendir my $recdir, $recursive_dir That way no close is needed, it closes itself when goes out of scope (globs are passe :) If you choose to stick with globs, I'll sign it off as is.
(In reply to Srdjan Jankovic from comment #23) > opendir RECDIR, $recursive_dir > > is much better written as > > opendir my $recdir, $recursive_dir > Ok, I thought is was not possible. Ok to use vars with those names.
Created attachment 20548 [details] [review] Bug 9312: Followup for one tab character To satisfy qa tools, removing one tab somewhere.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I got this error on uploading one image file: Can't use string ("1") as a HASH ref while "strict refs" in use at /usr/share/koha/testclone/tools/picture-upload.pl line 138 It seems that $results can be just 1. Line 138 refers to it as a hash: if ( %$results || %errors ) { And perl does not like that :)
Newer versions of Perl shouldn't mind that. Older versions required, and up until 5.12 I believe preferred keys(%whatever).
What is missing to pass QA ?
(In reply to Fridolyn SOMERS from comment #28) > What is missing to pass QA ? Please see comment 26
Comment 27 is misleading you cant use a string as a hash_ref in any release of perl. A variable called $results suggests theres a bit of confusion here. Meaningless variable names should be avoided, and we should be more explicit as to what we are testing. Another reason for failing QA is that a lexical dirhandle is replaced by the global DIR. That's surely a backward move
Yes, sorry I misread the comment, I thought it was about if(%hash) vs if( keys %hash)
And now patch doesn't apply. Side comment: I personally don't like splitting the if condition on an assignment to the next line, because one may accidentally think you forgot a semi-colon.
(In reply to M. Tompsett from comment #32) > And now patch doesn't apply. Fridolin, can you have a look please? fatal: sha1 information is lacking or useless (tools/picture-upload.pl). > Side comment: I personally don't like splitting the if condition on an > assignment to the next line, because one may accidentally think you forgot a > semi-colon. The file has been formatted using perltidy.
Created attachment 32753 [details] [review] Bug 9312: strict perl for picture-upload.pl Testing - this is a code-cleanup patch, so the purpose of testing is to ensure that patron image upload functionality still works. To test: - Turn on the patronimages system preference - Go to Tools | Upload patron images and import a suitable image for a test patron. - Bring up the test patron and verify that the image is attached. - From the patron details page, upload a replacement image. - Create a patron image ZIP file containing at least two images ( per the documentation of the patron image feature) and load it via the patron image import tool. - Verfiy that the test patrons now have images. - Verify that tools/picture-upload.pl didn't report any errors in the Apache error log.
Created attachment 32754 [details] [review] Bug 9312: whitespace cleanup for picture-upload.pl Formatting code using perltidy
Created attachment 32755 [details] [review] Bug 9312: fix a couple more uninitialized variable warnings
Created attachment 32756 [details] [review] Bug 9312: Followup for one tab character To satisfy qa tools, removing one tab somewhere.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
It had conflicts, but no sha-1 errors this time. And the conflicts were obviously caused by perltidy'ing, so they were easy enough to fix up. I've attached the rebased versions.
Created attachment 32759 [details] [review] Bug 9312: strict perl for picture-upload.pl Testing - this is a code-cleanup patch, so the purpose of testing is to ensure that patron image upload functionality still works. To test: - Turn on the patronimages system preference - Go to Tools | Upload patron images and import a suitable image for a test patron. - Bring up the test patron and verify that the image is attached. - From the patron details page, upload a replacement image. - Create a patron image ZIP file containing at least two images ( per the documentation of the patron image feature) and load it via the patron image import tool. - Verfiy that the test patrons now have images. - Verify that tools/picture-upload.pl didn't report any errors in the Apache error log. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 32760 [details] [review] Bug 9312: Perltidying picture-upload.pl Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 32761 [details] [review] Bug 9312: fix a couple more uninitialized variable warnings Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 32762 [details] [review] Bug 9312: Followup for one tab character To satisfy qa tools, removing one tab somewhere.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Sometimes our QA tools are pretty annoying, in cases like this, when its a tab in a comment, .. I don't think we really need to hold up a patch set for it
Uploding a single image from the tools area is broken with the patches: Can't use string ("1") as a HASH ref while "strict refs" in use at /home/katrin/kohaclone/tools/picture-upload.pl line 136.
Created attachment 32781 [details] [review] Bug 9312: Resolve error triggered by mutli-type return value. $results is 1 upon success or a hash on failure. Rather than check %$results || %error, I changed it to $results!=1 || %error. Strangely, this works without messy warnings. I believe this resolves the problem Katrin mentions in comment #43. TEST PLAN --------- 0) Make sure the system preference patronimages is set to Allow. 1) Apply all the patches 2) Login to staff client 3) Tools -> Upload patron images (it's in the bottom left) 4) Choose an image file, browse for a photo, enter a patron #. 5) Click Upload 6) Click the card number link -- the uploaded photo should be visible on the left side of the screen. 7) Run koha qa test tools.
Created attachment 33355 [details] [review] [SIGNED-OFF] Bug 9312: Resolve error triggered by mutli-type return value. $results is 1 upon success or a hash on failure. Rather than check %$results || %error, I changed it to $results!=1 || %error. Strangely, this works without messy warnings. I believe this resolves the problem Katrin mentions in comment #43. TEST PLAN --------- 0) Make sure the system preference patronimages is set to Allow. 1) Apply all the patches 2) Login to staff client 3) Tools -> Upload patron images (it's in the bottom left) 4) Choose an image file, browse for a photo, enter a patron #. 5) Click Upload 6) Click the card number link -- the uploaded photo should be visible on the left side of the screen. 7) Run koha qa test tools. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Image uploaded without problems No koha-qa errors
First 4 patches were signed, I tested all and signed the last one
Created attachment 33362 [details] [review] [PASSED QA] Bug 9312: strict perl for picture-upload.pl Testing - this is a code-cleanup patch, so the purpose of testing is to ensure that patron image upload functionality still works. To test: - Turn on the patronimages system preference - Go to Tools | Upload patron images and import a suitable image for a test patron. - Bring up the test patron and verify that the image is attached. - From the patron details page, upload a replacement image. - Create a patron image ZIP file containing at least two images ( per the documentation of the patron image feature) and load it via the patron image import tool. - Verfiy that the test patrons now have images. - Verify that tools/picture-upload.pl didn't report any errors in the Apache error log. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33363 [details] [review] [PASSED QA] Bug 9312: Perltidying picture-upload.pl Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33364 [details] [review] [PASSED QA] Bug 9312: fix a couple more uninitialized variable warnings Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33365 [details] [review] [PASSED QA] Bug 9312: Followup for one tab character To satisfy qa tools, removing one tab somewhere.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Sometimes our QA tools are pretty annoying, in cases like this, when its a tab in a comment, .. I don't think we really need to hold up a patch set for it Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 33366 [details] [review] [PASSED QA] Bug 9312: Resolve error triggered by mutli-type return value. $results is 1 upon success or a hash on failure. Rather than check %$results || %error, I changed it to $results!=1 || %error. Strangely, this works without messy warnings. I believe this resolves the problem Katrin mentions in comment #43. TEST PLAN --------- 0) Make sure the system preference patronimages is set to Allow. 1) Apply all the patches 2) Login to staff client 3) Tools -> Upload patron images (it's in the bottom left) 4) Choose an image file, browse for a photo, enter a patron #. 5) Click Upload 6) Click the card number link -- the uploaded photo should be visible on the left side of the screen. 7) Run koha qa test tools. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Image uploaded without problems No koha-qa errors Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patches pushed to master. Thanks!