This queue is for tickets about the Class-Factory CPAN distribution.

Report information
The Basics
Id:
81711
Status:
open
Priority:
Low/Low
Queue:

People
Owner:
Nobody in particular
Requestors:
sabol [...] alderaan.gsfc.nasa.gov
Cc:
AdminCc:

BugTracker
Severity:
Wishlist
Broken in:
  • 0.01
  • 0.02
  • 0.03
  • 1.00
  • 1.02
  • 1.03
  • 1.04
  • 1.05
  • 1.06
Fixed in:
(no value)



Subject: Avoid eval "require $object_class"; use eval { require $object_class_file } instead
It would be nice if Class::Factory would avoid using eval "require $object_class"; to load modules. I'm sure I don't have to tell you that eval EXPR is much less efficient compared to eval BLOCK. It's especially good to avoid it in a mod_perl environment, I think, where the code might be executed many times. I recommend the following code instead: (my $object_class_file = $object_class) =~ s-::|'-/-g; $object_class_file .= ".pm" if ($object_class_file !~ /\.p[lmh]$/); eval { require $object_class_file }; See http://www.perlmonks.org/?node_id=634438 for related discussion. Tip of the hat to PerlMonk tye for posting code similar to the above in that thread. (It correctly deals with modules with names like Acme::Don't and D'oh.) The Class::Factory code indicates that the original author intended to support $object_class values that end in ".p[lmh]", i.e. file names, I presume. I'm not sure how useful that is in practice, but I've attempted to retain that support with the "if ($object_class_file !~ /\.p[lmh]$/)" conditional shown above. If you want to be conservative, you could use the "eval BLOCK" form only if ($object_class =~ /^\w+(?:::\w+)*$/) and use "eval EXPR" in all other cases. I suspect that would cover 98% of Class::Factory's usage. What do you think?
Subject: Re: [rt.cpan.org #81711] Avoid eval "require $object_class"; use eval { require $object_class_file } instead
Date: Wed, 5 Dec 2012 13:34:51 -0800
To: bug-Class-Factory@rt.cpan.org
From: Fred Moyer <fred@redhotpenguin.com>
Patches welcome :) On Tue, Dec 4, 2012 at 9:05 PM, sabol@alderaan.gsfc.nasa.gov via RT <bug-Class-Factory@rt.cpan.org> wrote:
Show quoted text
> Wed Dec 05 00:05:11 2012: Request 81711 was acted upon. > Transaction: Ticket created by sabol@alderaan.gsfc.nasa.gov > Queue: Class-Factory > Subject: Avoid eval "require $object_class"; use eval { require > $object_class_file } instead > Broken in: 0.01, 0.02, 0.03, 1.00, 1.02, 1.03, 1.04, 1.05, 1.06 > Severity: Wishlist > Owner: Nobody > Requestors: sabol@alderaan.gsfc.nasa.gov > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=81711 > > > > It would be nice if Class::Factory would avoid using > > eval "require $object_class"; > > to load modules. I'm sure I don't have to tell you that eval EXPR is > much less efficient compared to eval BLOCK. It's especially good to > avoid it in a mod_perl environment, I think, where the code might be > executed many times. > > I recommend the following code instead: > > (my $object_class_file = $object_class) =~ s-::|'-/-g; > $object_class_file .= ".pm" if ($object_class_file !~ /\.p[lmh]$/); > eval { require $object_class_file }; > > See http://www.perlmonks.org/?node_id=634438 for related discussion. Tip > of the hat to PerlMonk tye for posting code similar to the above in that > thread. (It correctly deals with modules with names like Acme::Don't and > D'oh.) > > The Class::Factory code indicates that the original author intended to > support $object_class values that end in ".p[lmh]", i.e. file names, I > presume. I'm not sure how useful that is in practice, but I've attempted > to retain that support with the "if ($object_class_file !~ /\.p[lmh]$/)" > conditional shown above. > > If you want to be conservative, you could use the "eval BLOCK" form only > if ($object_class =~ /^\w+(?:::\w+)*$/) and use "eval EXPR" in all other > cases. I suspect that would cover 98% of Class::Factory's usage. > > What do you think?
From: sabol@alderaan.gsfc.nasa.gov
On Wed Dec 05 16:35:02 2012, fred@redhotpenguin.com wrote:
Show quoted text
> Patches welcome :)
Attached. All tests pass on my Linux platform, but the test suite doesn't exactly cover all the possibilities. Regarding the following code that comes right before the eval: if ( $INC{ $object_class } ) { $item->factory_log( "Looks like class '$object_class' was already ", "included; no further work necessary" ); } else { ... Older versions of Class::Factory do not have this. I suspect it was added to avoid the speed penalty of "eval EXPR". With the code modified to use "eval BLOCK", I think it is unnecessary. Or do you feel the logging is useful? Also, Perl::Critic complains about a number of "return undef" statements in the Class::Factory code. I don't know how you feel about that, but they do seem ill-advised. Some indenting tabs sneaked into the code (lines 190-197, get_registered_class method). If you're a strict anti-tabs person, you might want to untabify those lines.
Subject: ClassFactory.patch
--- Factory.pm 2007-11-06 19:06:54.000000000 -0500 +++ ../../blib/lib/Class/Factory.pm 2012-12-05 19:13:52.371697000 -0500 @@ -4,7 +4,7 @@ use strict; -$Class::Factory::VERSION = '1.06'; +$Class::Factory::VERSION = '1.07'; my %CLASS_BY_FACTORY_AND_TYPE = (); my %FACTORY_INFO_BY_CLASS = (); @@ -81,11 +81,13 @@ "included; no further work necessary" ); } else { - eval "require $object_class"; + (my $object_class_file = $object_class) =~ s-::|\'-/-g; + $object_class_file .= '.pm' unless ($object_class_file =~ /\.p[hlm]$/); + eval { require $object_class_file }; if ( $@ ) { $item->factory_error( "Cannot add factory type '$object_type' to ", - "class '$class': factory class '$object_class' ", - "cannot be required: $@" ); + "class '$class': factory class ", + "'$object_class' cannot be required: $@" ); return undef; } }
Subject: Re: [rt.cpan.org #81711] Avoid eval "require $object_class"; use eval { require $object_class_file } instead
Date: Wed, 5 Dec 2012 22:00:01 -0800
To: bug-Class-Factory@rt.cpan.org
From: Fred Moyer <fred@redhotpenguin.com>
Great, I'll merge this patch and do a release. I don't have a preference on the additional items you mentioned. Feel free to make additional changes if you want. I can provide you with a CO-MAINT cpan bit if you feel so inclined, so you can do the releases. On Wed, Dec 5, 2012 at 4:36 PM, sabol@alderaan.gsfc.nasa.gov via RT <bug-Class-Factory@rt.cpan.org> wrote:
Show quoted text
> Queue: Class-Factory > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=81711 > > > On Wed Dec 05 16:35:02 2012, fred@redhotpenguin.com wrote:
>> Patches welcome :)
> > Attached. All tests pass on my Linux platform, but the test suite > doesn't exactly cover all the possibilities. > > Regarding the following code that comes right before the eval: > > if ( $INC{ $object_class } ) { > $item->factory_log( "Looks like class '$object_class' was already ", > "included; no further work necessary" ); > } > else { > ... > > Older versions of Class::Factory do not have this. I suspect it was > added to avoid the speed penalty of "eval EXPR". With the code modified > to use "eval BLOCK", I think it is unnecessary. Or do you feel the > logging is useful? > > Also, Perl::Critic complains about a number of "return undef" statements > in the Class::Factory code. I don't know how you feel about that, but > they do seem ill-advised. > > Some indenting tabs sneaked into the code (lines 190-197, > get_registered_class method). If you're a strict anti-tabs person, you > might want to untabify those lines.


This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

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