Summary: | test for Libravatar::URL can cause scripts to abort | ||
---|---|---|---|
Product: | Koha | Reporter: | Colin Campbell <colin.campbell> |
Component: | OPAC | Assignee: | Colin Campbell <colin.campbell> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | chris, m.de.rooy, paul.poulain |
Version: | 3.6 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Proposed patch
Updated patch Bug 6877 - use is executed and errors at compile time |
Description
Colin Campbell
2011-09-17 11:43:10 UTC
Created attachment 5450 [details] [review] Proposed patch 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 Created attachment 5568 [details] [review] Updated patch 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. 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> 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. 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 ? Re Paul's comment - the script aborts i.e. more than the tested feature gets disabled This was included in the 3.6 branch prior to 3.6.4. |