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
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
X-RT-Encrypt: 0
X-RT-Sign: 0
Content-Length: 1302
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 fred@redhotpenguin.com Wed Dec 5 16: 35:01 2012
MIME-Version: 1.0
X-Spam-Status: No, score=-6.983 tagged_above=-99.9 required=10 tests=[AWL=-0.083, BAYES_00=-1.9, RCVD_IN_DNSWL_HI=-5] autolearn=ham
In-Reply-To: <rt-3.8.HEAD-25469-1354683911-344.81711-4-0@rt.cpan.org>
X-Spam-Flag: NO
References: <RT-Ticket-81711@rt.cpan.org> <rt-3.8.HEAD-25469-1354683911-344.81711-4-0@rt.cpan.org>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
Message-ID: <CAHrynWD9pFtNgm7zf53kKT706cZP7vdNp315cYYzRZY8phbysA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
X-RT-Original-Encoding: utf-8
X-Spam-Score: -6.983
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 19008240A6C for <cpan-bug+Class-Factory@hipster.bestpractical.com>; Wed, 5 Dec 2012 16:35:01 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GMPGVfdftJzb for <cpan-bug+Class-Factory@hipster.bestpractical.com>; Wed, 5 Dec 2012 16:34:59 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id 2AB7D2409AD for <bug-Class-Factory@rt.cpan.org>; Wed, 5 Dec 2012 16:34:58 -0500 (EST)
Received: (qmail 20213 invoked by uid 103); 5 Dec 2012 21:34:58 -0000
Received: from x16.dev (10.0.100.26) by x1.dev with QMQP; 5 Dec 2012 21:34:58 -0000
Received: from mail-vb0-f41.google.com (HELO mail-vb0-f41.google.com) (209.85.212.41) by 16.mx.develooper.com (qpsmtpd/0.84/v0.84-167-g4ed6cab) with ESMTP; Wed, 05 Dec 2012 13:34:55 -0800
Received: by mail-vb0-f41.google.com with SMTP id l22so5805332vbn.0 for <bug-Class-Factory@rt.cpan.org>; Wed, 05 Dec 2012 13:34:52 -0800 (PST)
Received: by 10.58.172.103 with SMTP id bb7mr16861396vec.41.1354743291906; Wed, 05 Dec 2012 13:34:51 -0800 (PST)
Received: by 10.58.207.9 with HTTP; Wed, 5 Dec 2012 13:34:51 -0800 (PST)
Delivered-To: cpan-bug+Class-Factory@hipster.bestpractical.com
Subject: Re: [rt.cpan.org #81711] Avoid eval "require $object_class"; use eval { require $object_class_file } instead
Return-Path: <fred@redhotpenguin.com>
X-Spam-Check-BY: 16.mx.develooper.com
X-Original-To: cpan-bug+Class-Factory@hipster.bestpractical.com
X-RT-Mail-Extension: class-factory
X-Google-Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:x-gm-message-state; bh=mydes+0GOvhwHhLPW7RRRCUzqAqGUewp4583vN9B4B8=; b=iLiq2EuIFXiIVjniO8UgWGM56omZb8q0pUCQIqudfLRVhvoyKsC2N8iXSyDGcoHWK6 RoRkF52+OllY+VKOiRw2ksPO0T20Z3D8YZhu7Av8b5AsaBwmgeMQJc+ytWS1qYuWQzPw fWphFCRGK4ViNa4/yzdIsjm+HxEaeZQw1LZju988vsuBWSy2FxvDGNVZT3W80WgzWD6b QU2cVrFvB7IWGbH6ujz5C1epAAq5v2FKjK8iWfdyGenPn2ANENTpWLSTsJNHmIvlTENf iJrOxFdy+42ZTADRkn4bZQsGy/ZS2kwtjGlMh2VwvixgByYwE+1o0izmXmh7RPctCL2d BiwQ==
Date: Wed, 5 Dec 2012 13:34:51 -0800
X-Spam-Level:
To: bug-Class-Factory@rt.cpan.org
From: Fred Moyer <fred@redhotpenguin.com>
X-GM-Message-State: ALoCoQnOcWXhOdD4xiOYB87gAwIm2OlX0d7r7WE6TXm3suihadpXklr297r5qLI7q3vGfE1vIRFG
RT-Message-ID: <rt-3.8.HEAD-14815-1354743302-1066.81711-0-0@rt.cpan.org>
Content-Length: 1990
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?
MIME-Version: 1.0
In-Reply-To: <rt-3.8.HEAD-14815-1354743302-1066.81711-0-0@rt.cpan.org>
X-Mailer: MIME-tools 5.427 (Entity 5.427)
References: <RT-Ticket-81711@rt.cpan.org> <rt-3.8.HEAD-25469-1354683911-344.81711-4-0@rt.cpan.org> <CAHrynWD9pFtNgm7zf53kKT706cZP7vdNp315cYYzRZY8phbysA@mail.gmail.com> <rt-3.8.HEAD-14815-1354743302-1066.81711-0-0@rt.cpan.org>
Content-Type: multipart/mixed; boundary="----------=_1354754183-17003-3"
Message-ID: <rt-3.8.HEAD-17003-1354754183-1894.81711-0-0@rt.cpan.org>
From: sabol@alderaan.gsfc.nasa.gov
X-RT-Original-Encoding: utf-8
Content-Length: 0
Content-Disposition: inline
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 1029
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.
MIME-Version: 1.0
Subject: ClassFactory.patch
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Type: text/x-patch; name="ClassFactory.patch"
Content-Disposition: inline; filename="ClassFactory.patch"
Content-Transfer-Encoding: binary
Content-Length: 1089
--- 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; } }
From fred@redhotpenguin.com Thu Dec 6 01: 00:12 2012
MIME-Version: 1.0
X-Spam-Status: No, score=-6.978 tagged_above=-99.9 required=10 tests=[AWL=-0.078, BAYES_00=-1.9, RCVD_IN_DNSWL_HI=-5] autolearn=ham
In-Reply-To: <rt-3.8.HEAD-17003-1354754183-1040.81711-5-0@rt.cpan.org>
X-Spam-Flag: NO
References: <RT-Ticket-81711@rt.cpan.org> <rt-3.8.HEAD-25469-1354683911-344.81711-4-0@rt.cpan.org> <CAHrynWD9pFtNgm7zf53kKT706cZP7vdNp315cYYzRZY8phbysA@mail.gmail.com> <rt-3.8.HEAD-14815-1354743302-1066.81711-5-0@rt.cpan.org> <rt-3.8.HEAD-17003-1354754183-1040.81711-5-0@rt.cpan.org>
X-Virus-Scanned: Debian amavisd-new at bestpractical.com
Message-ID: <CAHrynWBO+49KG9GYm+igsJ2TGOO2FzmhN1F0SWW3jTycrrBKUg@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
X-RT-Original-Encoding: utf-8
X-Spam-Score: -6.978
Received: from localhost (localhost [127.0.0.1]) by hipster.bestpractical.com (Postfix) with ESMTP id 2C5CD24083E for <cpan-bug+Class-Factory@hipster.bestpractical.com>; Thu, 6 Dec 2012 01:00:12 -0500 (EST)
Received: from hipster.bestpractical.com ([127.0.0.1]) by localhost (hipster.bestpractical.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xeTj6-lZfAbh for <cpan-bug+Class-Factory@hipster.bestpractical.com>; Thu, 6 Dec 2012 01:00:10 -0500 (EST)
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by hipster.bestpractical.com (Postfix) with SMTP id 206C2240338 for <bug-Class-Factory@rt.cpan.org>; Thu, 6 Dec 2012 01:00:09 -0500 (EST)
Received: (qmail 25724 invoked by uid 103); 6 Dec 2012 06:00:08 -0000
Received: from x16.dev (10.0.100.26) by x1.dev with QMQP; 6 Dec 2012 06:00:08 -0000
Received: from mail-vb0-f41.google.com (HELO mail-vb0-f41.google.com) (209.85.212.41) by 16.mx.develooper.com (qpsmtpd/0.84/v0.84-167-g4ed6cab) with ESMTP; Wed, 05 Dec 2012 22:00:04 -0800
Received: by mail-vb0-f41.google.com with SMTP id l22so6387699vbn.0 for <bug-Class-Factory@rt.cpan.org>; Wed, 05 Dec 2012 22:00:01 -0800 (PST)
Received: by 10.58.0.83 with SMTP id 19mr224262vec.53.1354773601740; Wed, 05 Dec 2012 22:00:01 -0800 (PST)
Received: by 10.58.207.9 with HTTP; Wed, 5 Dec 2012 22:00:01 -0800 (PST)
Delivered-To: cpan-bug+Class-Factory@hipster.bestpractical.com
Subject: Re: [rt.cpan.org #81711] Avoid eval "require $object_class"; use eval { require $object_class_file } instead
Return-Path: <fred@redhotpenguin.com>
X-Spam-Check-BY: 16.mx.develooper.com
X-Original-To: cpan-bug+Class-Factory@hipster.bestpractical.com
X-RT-Mail-Extension: class-factory
X-Google-Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type:x-gm-message-state; bh=nB8/uF1bIe9EPtXRWDjCC48Q1kepz4aJ68O/cXVLlTo=; b=cfObCZjqIYScPQD427966d9dGQg9Aq9hrT2X3c/yI9OUkoXHbpRUmnwjKTDYrNO5ri iW/7eQMEmvw7Tqdo+eOHVOHJGnPNc3Ab/kRyQJJyuRJfCerBIhuZLEaNVBcfub3bxjVs i93S7b9maUMhcd2LX9oYRu3ZlcNSLGiQPEN3pJqhdsd3fRNs5VZYq7gaMdHi3X5N4c+Q 9sy393Eb35qRdV9cFtGdqron6p3de+xWlumHnWVVFfKJx91UOhX3MfR0V8XMhxLPT4/I zdGrGpUWH4Af1p8xclUfCCN/bMHIRpNxGP3cajYoP5HqBVYhKNj28ilYKooh+nXUx4Iy JNtg==
Date: Wed, 5 Dec 2012 22:00:01 -0800
X-Spam-Level:
To: bug-Class-Factory@rt.cpan.org
From: Fred Moyer <fred@redhotpenguin.com>
X-GM-Message-State: ALoCoQnjEAvjU9Of1mISUzmWHQXparjve4K5ilsSGd72gcirdpntuOiLLOQrJPowG3fbg73vF2XE
RT-Message-ID: <rt-3.8.HEAD-20066-1354773613-27.81711-0-0@rt.cpan.org>
Content-Length: 1543
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.