Bug 14588

Summary: Simplify merge_ajax.pl
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: CatalogingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: jonathan.druart, m.de.rooy, mtompset, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14589
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14588: Simplify merge_ajax.pl
Checked the code, looks good to me. I didn't notice any
Bug 14588: Simplify merge_ajax.pl
Bug 14588: Simplify merge_ajax.pl
Bug 14588: Simplify merge_ajax.pl
Bug 14588: Simplify merge_ajax.pl

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 Mark 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 Joonas Kylmälä 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 Mark 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 Mark 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!