Bug 9312 - strict perl for picture-upload.pl
Summary: strict perl for picture-upload.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-21 13:28 UTC by Fridolin Somers
Modified: 2015-06-04 23:33 UTC (History)
10 users (show)

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


Attachments
Proposed patch (8.16 KB, patch)
2012-12-21 13:56 UTC, Fridolin Somers
Details | Diff | Splinter Review
Proposed patch (follow-up 1) (23.58 KB, patch)
2012-12-21 14:03 UTC, Fridolin Somers
Details | Diff | Splinter Review
Proposed patch (rebased master) (8.16 KB, patch)
2012-12-24 08:20 UTC, Fridolin Somers
Details | Diff | Splinter Review
Proposed patch (follow-up 1) (rebased master) (23.58 KB, patch)
2012-12-24 08:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: strict perl for picture-upload.pl (8.89 KB, patch)
2013-01-16 16:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9312: strict perl for picture-upload.pl (8.89 KB, patch)
2013-01-16 16:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9312: whitespace cleanup for picture-upload.pl (23.62 KB, patch)
2013-01-16 16:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9312: fix a couple more uninitialized variable warnings (1.46 KB, patch)
2013-01-16 16:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9312: strict perl for picture-upload.pl (8.74 KB, patch)
2013-06-12 13:34 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: whitespace cleanup for picture-upload.pl (23.77 KB, patch)
2013-06-12 13:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: fix a couple more uninitialized variable warnings (1.41 KB, patch)
2013-06-12 13:37 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: strict perl for picture-upload.pl (8.73 KB, patch)
2013-06-13 13:16 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: whitespace cleanup for picture-upload.pl (23.77 KB, patch)
2013-06-13 13:16 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: fix a couple more uninitialized variable warnings (1.41 KB, patch)
2013-06-13 13:16 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 9312: Followup for one tab character (959 bytes, patch)
2013-08-22 12:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9312: strict perl for picture-upload.pl (8.72 KB, patch)
2014-10-26 04:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9312: whitespace cleanup for picture-upload.pl (23.22 KB, patch)
2014-10-26 04:29 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9312: fix a couple more uninitialized variable warnings (1.41 KB, patch)
2014-10-26 04:29 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9312: Followup for one tab character (916 bytes, patch)
2014-10-26 04:29 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 9312: strict perl for picture-upload.pl (8.78 KB, patch)
2014-10-26 05:37 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9312: Perltidying picture-upload.pl (23.23 KB, patch)
2014-10-26 05:37 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9312: fix a couple more uninitialized variable warnings (1.46 KB, patch)
2014-10-26 05:38 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9312: Followup for one tab character (1.10 KB, patch)
2014-10-26 05:40 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9312: Resolve error triggered by mutli-type return value. (2.04 KB, patch)
2014-10-27 02:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9312: Resolve error triggered by mutli-type return value. (2.16 KB, patch)
2014-11-07 01:11 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 9312: strict perl for picture-upload.pl (8.84 KB, patch)
2014-11-07 07:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9312: Perltidying picture-upload.pl (23.30 KB, patch)
2014-11-07 07:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9312: fix a couple more uninitialized variable warnings (1.53 KB, patch)
2014-11-07 07:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9312: Followup for one tab character (1.17 KB, patch)
2014-11-07 07:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 9312: Resolve error triggered by mutli-type return value. (2.21 KB, patch)
2014-11-07 07:16 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 Fridolin Somers 2012-12-21 13:28:55 UTC
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.
Comment 1 Fridolin Somers 2012-12-21 13:56:39 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2012-12-21 14:03:25 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-12-21 16:04:07 UTC
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)
Comment 4 Fridolin Somers 2012-12-24 08:20:40 UTC Comment hidden (obsolete)
Comment 5 Fridolin Somers 2012-12-24 08:21:13 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-01-04 13:34:12 UTC
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
Comment 7 Jonathan Druart 2013-01-04 14:58:20 UTC
"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)
Comment 8 Fridolin Somers 2013-01-04 16:34:04 UTC
(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' ?
Comment 9 Galen Charlton 2013-01-16 16:18:03 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2013-01-16 16:18:43 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-01-16 16:18:48 UTC Comment hidden (obsolete)
Comment 12 Galen Charlton 2013-01-16 16:18:51 UTC Comment hidden (obsolete)
Comment 13 Galen Charlton 2013-01-16 16:19:40 UTC
Note that in the course of signing off I added a test plan, reworded a commit message, and added a followup patch.
Comment 14 Jonathan Druart 2013-03-14 13:53:38 UTC
Patches don't apply (conflicts with bug 9421).
Comment 15 Fridolin Somers 2013-06-12 13:34:53 UTC Comment hidden (obsolete)
Comment 16 Fridolin Somers 2013-06-12 13:36:16 UTC Comment hidden (obsolete)
Comment 17 Fridolin Somers 2013-06-12 13:37:06 UTC Comment hidden (obsolete)
Comment 18 Fridolin Somers 2013-06-12 13:41:34 UTC
All patches rebased master.
There are a lot of changes so I set to needs signoff.
Comment 19 Galen Charlton 2013-06-12 15:26:55 UTC
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.
Comment 20 Fridolin Somers 2013-06-13 13:16:02 UTC Comment hidden (obsolete)
Comment 21 Fridolin Somers 2013-06-13 13:16:29 UTC Comment hidden (obsolete)
Comment 22 Fridolin Somers 2013-06-13 13:16:48 UTC Comment hidden (obsolete)
Comment 23 Srdjan Jankovic 2013-07-19 07:52:20 UTC
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.
Comment 24 Fridolin Somers 2013-07-19 11:46:00 UTC
(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.
Comment 25 Marcel de Rooy 2013-08-22 12:48:22 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2013-08-22 12:50:10 UTC
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 :)
Comment 27 Srdjan Jankovic 2013-08-22 22:44:37 UTC
Newer versions of Perl shouldn't mind that. Older versions required, and up until 5.12 I believe preferred keys(%whatever).
Comment 28 Fridolin Somers 2013-10-14 15:36:25 UTC
What is missing to pass QA ?
Comment 29 Marcel de Rooy 2013-10-16 11:08:59 UTC
(In reply to Fridolyn SOMERS from comment #28)
> What is missing to pass QA ?
Please see comment 26
Comment 30 Colin Campbell 2013-10-16 11:43:20 UTC
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
Comment 31 Srdjan Jankovic 2013-10-16 19:48:10 UTC
Yes, sorry I misread the comment, I thought it was about if(%hash) vs if( keys %hash)
Comment 32 Mark Tompsett 2014-04-04 17:50:18 UTC
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.
Comment 33 Jonathan Druart 2014-04-07 07:29:16 UTC
(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.
Comment 34 Mark Tompsett 2014-10-26 04:28:57 UTC Comment hidden (obsolete)
Comment 35 Mark Tompsett 2014-10-26 04:29:06 UTC Comment hidden (obsolete)
Comment 36 Mark Tompsett 2014-10-26 04:29:16 UTC Comment hidden (obsolete)
Comment 37 Mark Tompsett 2014-10-26 04:29:27 UTC Comment hidden (obsolete)
Comment 38 Mark Tompsett 2014-10-26 04:31:00 UTC
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.
Comment 39 Chris Cormack 2014-10-26 05:37:20 UTC Comment hidden (obsolete)
Comment 40 Chris Cormack 2014-10-26 05:37:55 UTC Comment hidden (obsolete)
Comment 41 Chris Cormack 2014-10-26 05:38:25 UTC Comment hidden (obsolete)
Comment 42 Chris Cormack 2014-10-26 05:40:23 UTC Comment hidden (obsolete)
Comment 43 Katrin Fischer 2014-10-26 09:55:42 UTC
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.
Comment 44 Mark Tompsett 2014-10-27 02:53:57 UTC Comment hidden (obsolete)
Comment 45 Bernardo Gonzalez Kriegel 2014-11-07 01:11:17 UTC Comment hidden (obsolete)
Comment 46 Bernardo Gonzalez Kriegel 2014-11-07 01:12:48 UTC
First 4 patches were signed, I tested all and signed the last one
Comment 47 Katrin Fischer 2014-11-07 07:15:52 UTC
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>
Comment 48 Katrin Fischer 2014-11-07 07:15:56 UTC
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>
Comment 49 Katrin Fischer 2014-11-07 07:16:00 UTC
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>
Comment 50 Katrin Fischer 2014-11-07 07:16:03 UTC
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>
Comment 51 Katrin Fischer 2014-11-07 07:16:08 UTC
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>
Comment 52 Tomás Cohen Arazi 2014-11-07 14:46:01 UTC
Patches pushed to master.

Thanks!