Skip Menu |
 

This queue is for tickets about the Log-Log4perl CPAN distribution.

Report information
The Basics
Id: 28987
Status: resolved
Priority: 0/
Queue: Log-Log4perl

People
Owner: Nobody in particular
Requestors: gberriz [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Bug in Log::Log4perl::Appender
Date: Sat, 25 Aug 2007 15:14:01 -0400
To: bug-Log-Log4perl [...] rt.cpan.org
From: "Gabriel Berriz" <gberriz [...] gmail.com>
Download (untitled) / with headers
text/plain 3.7k
Here's the patch: --- lib/Log/Log4perl/Appender.pm.dist 2006-08-09 00:54:54.000000000 -0400 +++ lib/Log/Log4perl/Appender.pm 2007-08-25 14:06:09.899333850 -0400 @@ -43,10 +43,9 @@ sub new { # anything in there that can't be class name. die "'$appenderclass' not a valid class name " if $appenderclass =~ /[^:\w]/; - # Check if the class/package is already in the namespace because + # Check if the class/package is already available because # something like Class::Prototyped injected it previously. - no strict 'refs'; - if(!scalar(keys %{"$appenderclass\::"})) { + if(!UNIVERSAL::can($appenderclass, 'new')) { # Not available yet, try to pull it in. # see 'perldoc -f require' for why two evals eval "require $appenderclass"; The original test in the if statement will fail spuriously if earlier some other part of the code had accessed a global variable in the appender class's package (e.g. with an expression such as "...if $My::Appender::Class::DEBUG;"). Note that Perl honors such expressions even if the corresponding package has not been loaded. Therefore, they should not be penalized. The proposed solution does not have this problem. It regards a package as missing only if its constructor is not available. After applying this patch all the distribution's tests succeed; at least all those that are run when installing under Linux. I have not tested the patch under any other OS, but I have little reason to expect that it would not work on any OS on which the original version works. BTW, there's a more fundamental bug here (which the patch above does not address): implicit in this code is the incorrect assumption that a class name corresponds to the name of the file, when in fact no such constraint exists: Perl has no problem if one chooses to define class Foo in a file called Bar.pm. The code above (with or without the patch) will fail in such situations. Since I don't see why this code should penalize what perl itself does not, IMHO, I think this code should treat all classes equally. It's less confusing this way. Therefore, I would get rid of the attempt to load missing classes altogether; that should be the calling code's responsibility. Just my 2¢. FWIW, I include the uuencoded version of the patch below. Best regards, Gabriel Berriz If the uuencoded version of the patch is needed, save everything between (but excluding) the ==BEGIN== and the ==END== lines to a file (say patch.uue) and restore with perl -ne 'print unpack("u*",$_)' patch.uue > patch == BEGIN == M+2TM(&QI8B],;V<O3&]G-'!E<FPO07!P96YD97(N<&TN9&ES=`DR,#`V+3`X =+3`Y(#`P.C4T.C4T+C`P,#`P,#`P,"`M,#0P,`H` M*RLK(&QI8B],;V<O3&]G-'!E<FPO07!P96YD97(N<&T),C`P-RTP."TR-2`Q 8-#HP-CHP.2XX.3DS,S,X-3`@+3`T,#`* =0$`@+30S+#$P("LT,RPY($!`('-U8B!N97<@>PH` M("`@("`@("`@("`@(R!A;GET:&EN9R!I;B!T:&5R92!T:&%T(&-A;B=T(&)E -(&-L87-S(&YA;64N"@`` M("`@("`@("`@9&EE("(G)&%P<&5N9&5R8VQA<W,G(&YO="!A('9A;&ED(&-L J87-S(&YA;64@(B!I9B`D87!P96YD97)C;&%S<R`]?B`O6UXZ7'==+SL* "(`H` M+2`@("`@("`@("`@(",@0VAE8VL@:68@=&AE(&-L87-S+W!A8VMA9V4@:7,@ A86QR96%D>2!I;B!T:&4@;F%M97-P86-E(&)E8V%U<V4* M*R`@("`@("`@("`@(",@0VAE8VL@:68@=&AE(&-L87-S+W!A8VMA9V4@:7,@ :86QR96%D>2!A=F%I;&%B;&4@8F5C875S90H` M("`@("`@("`@("`@(",@<V]M971H:6YG(&QI:V4@0VQA<W,Z.E!R;W1O='EP ;960@:6YJ96-T960@:70@<')E=FEO=7-L>2X* ;+2`@("`@("`@;F\@<W1R:6-T("=R969S)SL* M+2`@("`@("`@:68H(7-C86QA<BAK97ES("5[(B1A<'!E;F1E<F-L87-S7#HZ '(GTI*2!["@`` M*R`@("`@("`@:68H(55.259%4E-!3#HZ8V%N*"1A<'!E;F1E<F-L87-S+"`G );F5W)RDI('L* M("`@("`@("`@("`@(",@3F]T(&%V86EL86)L92!Y970L('1R>2!T;R!P=6QL ((&ET(&EN+@H` M("`@("`@("`@("`@(",@<V5E("=P97)L9&]C("UF(')E<75I<F4G(&9O<B!W -:'D@='=O(&5V86QS"@`` L("`@("`@("`@("`@(&5V86P@(G)E<75I<F4@)&%P<&5N9&5R8VQA<W,B.PH` ==END==
Download (untitled) / with headers
text/plain 954b
Sorry for the long wait, I've finally applied your patch with modifications: http://github.com/mschilli/log4perl/commit/220487622f9edf4124ed2d724cddbd35f3179ed1 Only modification was that I'm checking for UNIVERSAL and if it doesn't exist (possibly in ancient perl versions), the old, somewhat imperfect check is run instead. Regarding your comment on Log4perl requiring package and file names to be in sync, that's only the case if the class hasn't been loaded yet. If it has, then no attempt will be made to localize a file. If it hasn't, then loading is required at some point anyway and loading the .pm file is the only choice if the code hasn't loaded it beforehand. If you want to perform the loading yourself (and it doesn't matter which files the packages are located in as long as you load them somehow), you can do that before calling Log4perl's init() and Log4perl won't try to locate/load them again. Thanks for your contribution. --Mike


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.