Bug 14588 - Simplify merge_ajax.pl
Summary: Simplify merge_ajax.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-22 12:18 UTC by Marcel de Rooy
Modified: 2016-12-05 21:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14588: Simplify merge_ajax.pl (2.23 KB, patch)
2015-07-22 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Checked the code, looks good to me. I didn't notice any (2.32 KB, patch)
2015-08-12 13:28 UTC, j.kylmala
Details | Diff | Splinter Review
Bug 14588: Simplify merge_ajax.pl (2.28 KB, patch)
2015-08-19 14:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14588: Simplify merge_ajax.pl (2.20 KB, patch)
2015-08-19 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14588: Simplify merge_ajax.pl (2.21 KB, patch)
2015-08-19 18:09 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14588: Simplify merge_ajax.pl (2.28 KB, patch)
2015-08-20 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2015-07-22 12:18:53 UTC

    
Comment 1 Marcel de Rooy 2015-07-22 12:55:25 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2015-08-12 11:30:35 UTC
(In reply to Marcel de Rooy from comment #1)
> ... (Note that
> Class::->method is preferred above Class->method. See perlobj.)

http://perldoc.perl.org/perlobj.html#Invoking-Class-Methods
Interesting. I did not know this.
Comment 3 j.kylmala 2015-08-12 13:28:55 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2015-08-12 14:59:34 UTC
(In reply to Joonas Kylmälä from comment #3)
> Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>

Thx Joonas
Comment 5 Jonathan Druart 2015-08-18 15:23:47 UTC
(In reply to Marcel de Rooy from comment #1)
>(Note that Class::->method is preferred above Class->method. See perlobj.)

I did not find where it's written it's better.
I personally think you are adding a third way where we already have new CGI and CGI->new...
Comment 6 Marcel de Rooy 2015-08-19 12:08:23 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Marcel de Rooy from comment #1)
> >(Note that Class::->method is preferred above Class->method. See perlobj.)
> 
> I did not find where it's written it's better.
> I personally think you are adding a third way where we already have new CGI
> and CGI->new...

ThereIsMoreThanOneWayToDoIt
The reference is few comments up, but for the rest it is completely unimportant.
Comment 7 M. Tompsett 2015-08-19 13:25:38 UTC
(In reply to Jonathan Druart from comment #5)
> (In reply to Marcel de Rooy from comment #1)
> >(Note that Class::->method is preferred above Class->method. See perlobj.)
> 
> I did not find where it's written it's better.
> I personally think you are adding a third way where we already have new CGI
> and CGI->new...

I agree that "better" is not the word used. However, it is expressly explicit in how Perl should interpret the call. There is something to be said positively for explicitness.

"Because Perl allows you to use barewords for package names and subroutine names, it sometimes interprets a bareword's meaning incorrectly."
http://perldoc.perl.org/perlobj.html#Invoking-Class-Methods

Though, I do share Jonathan's view that CGI->new is sufficient. This explicit syntax is an example of confusing ugliness that I personally would prefer to avoid.
Comment 8 Marcel de Rooy 2015-08-19 13:29:53 UTC
If there is widespread concensus for removing this *ugly* construction, I will be happy to remove it quickly :)
Comment 9 Marcel de Rooy 2015-08-19 14:12:54 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2015-08-19 14:21:06 UTC Comment hidden (obsolete)
Comment 11 M. Tompsett 2015-08-19 18:09:29 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-08-20 10:21:28 UTC
Created attachment 41707 [details] [review]
Bug 14588: Simplify merge_ajax.pl

This script does not need a few modules:
    IO::File; CGI::Session; C4::UploadedFile.
Warnings can be enabled (with Modern::Perl).
Indirect object syntax replaced for CGI and CGI::Cookie.
Moved the $reply line upwards (not needed twice anymore).

Test plan:
[1] Log in as staff user. Run the URL cataloguing/merge_ajax.pl.
    You should see the JSON reponse in the browser.
[2] Logout. Run the URL again. Blank screen.
[3] Go to addbooks.pl (Cataloging Search). Search something, select two
    biblios and click Merge selected, etc.

Signed-off-by: Joonas Kylmala <j.kylmala@gmail.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Tomás Cohen Arazi 2015-08-21 13:40:30 UTC
Patch pushed to master.

Thanks Marcel!