Bug 6877 - test for Libravatar::URL can cause scripts to abort
Summary: test for Libravatar::URL can cause scripts to abort
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Colin Campbell
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-17 11:43 UTC by Colin Campbell
Modified: 2013-12-05 19:59 UTC (History)
3 users (show)

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


Attachments
Proposed patch (1.62 KB, patch)
2011-09-17 12:15 UTC, Colin Campbell
Details | Diff | Splinter Review
Updated patch (2.07 KB, patch)
2011-09-23 16:05 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 6877 - use is executed and errors at compile time (2.08 KB, patch)
2011-10-28 20:11 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2011-09-17 11:43:10 UTC
Scripts attempt to do a runtime conditional import of the Libravatar::URL but uses 'use' rather than require/import so that this is done at compile time and the script will abort on compile and not process the code before the eval. 

Change the code to use require that is executed at runtime and an eval block rather than a string eval.

Personally using eval for this is a bit of a code smell. Checking the existence of the module on every run seems inefficient.
Comment 1 Colin Campbell 2011-09-17 12:15:15 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-09-19 09:35:10 UTC
Can you think of a nicer way to do it Colin? We could shift it inside the if, so its only checked if they have the syspref on?

If you don't mind I can change your patch to do that, as well as changing the use to a require
Comment 3 Colin Campbell 2011-09-23 16:05:32 UTC Comment hidden (obsolete)
Comment 4 Colin Campbell 2011-09-23 16:10:57 UTC
Updated patch to embed eval within check of syspreference. Makes for more readable code.
There is a CPAN module for conditional inclusion of modules but I've not actually used it. Having a wrapper for that functionality might be useful to make calls more consistent and keep calling code readable.
Comment 5 Chris Cormack 2011-10-28 20:11:27 UTC
Created attachment 6105 [details] [review]
Bug 6877 - use is executed and errors at compile time

Although use is being called in an eval it will still be executed
at compile time so that an error can cause the script to abort before
the eval is executed. The eval expression syntax is not checked
so eval block should be preferred.
Use require/import which execute at runtime which is the intention
here.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 6 Marcel de Rooy 2011-11-24 11:42:38 UTC
QA Comment: This patch already improves the current code. Ideas to do it even better (referring to above comments) are welcome of course.
Marking it as Passed QA.
Comment 7 Paul Poulain 2011-11-25 13:20:50 UTC
patch pushed, please test

QA comment: what can cause a problem ? I just see the case where the library is not installed & the feature disabled. Is there something else ?
Comment 8 Colin Campbell 2011-11-25 14:58:48 UTC
Re Paul's comment - the script aborts i.e. more than the tested feature gets disabled
Comment 9 Jared Camins-Esakov 2012-05-23 12:38:39 UTC
This was included in the 3.6 branch prior to 3.6.4.