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.
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.