Mailing List Archive: Request Tracker: Devel

[patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included

Index | Next | Previous | View Threaded

ivan-rt-devel at 420

Apr 20, 2011, 10:44 AM

Post #1 of 9 (1114 views)
Permalink

[patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included

RT::Interface::Web_Overlay, Web_Vendor and Web_Local are not being
called.

This is a regression from RT 3.8.9->3.8.10 and also affects 4.0.0rc8

RT::Interface::Web is in package HTML::Mason::Commands when it calls
RT::Base->_ImportOverlays, so it looks for the wrong files.

I figured reworking _ImportOverlays to use the calling filename instead
of calling package is not desirable (but I'd be happy to give that a
shot if you prefer), so this patch fixes things:

diff -u lib/RT/Interface/Web.pm.DIST lib/RT/Interface/Web.pm
--- lib/RT/Interface/Web.pm.DIST 2011-04-20 10:33:53.000000000 -0700
+++ lib/RT/Interface/Web.pm 2011-04-20 10:34:04.000000000 -0700
@@ -2756,6 +2756,7 @@
return $scrubber;
}

+package RT::Interface::Web;
RT::Base->_ImportOverlays();

1;

--
Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
Open-source billing, ticketing and provisioning - http://www.freeside.biz/
_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

ruz at bestpractical

Apr 20, 2011, 7:18 PM

Post #2 of 9 (1076 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

Hello,

This is still doesn't return old behaviour. Which was with side effects.

Old code was loading .../Web_Local.pm, but code was compiled in
HTML::Mason::Commands space unless explicit package is defined in
_Local.pm file.

On Wed, Apr 20, 2011 at 9:44 PM, Ivan Kohler <ivan-rt-devel [at] 420> wrote:
> RT::Interface::Web_Overlay, Web_Vendor and Web_Local are not being
> called.
>
> This is a regression from RT 3.8.9->3.8.10 and also affects 4.0.0rc8
>
> RT::Interface::Web is in package HTML::Mason::Commands when it calls
> RT::Base->_ImportOverlays, so it looks for the wrong files.
>
> I figured reworking _ImportOverlays to use the calling filename instead
> of calling package is not desirable (but I'd be happy to give that a
> shot if you prefer), so this patch fixes things:
>
>
> diff -u lib/RT/Interface/Web.pm.DIST lib/RT/Interface/Web.pm
> --- lib/RT/Interface/Web.pm.DIST Ā Ā Ā Ā 2011-04-20 10:33:53.000000000 -0700
> +++ lib/RT/Interface/Web.pm Ā Ā 2011-04-20 10:34:04.000000000 -0700
> @@ -2756,6 +2756,7 @@
> Ā Ā return $scrubber;
> Ā }
>
> +package RT::Interface::Web;
> Ā RT::Base->_ImportOverlays();
>
> Ā 1;

--
Best regards, Ruslan.
_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

ivan-rt-devel at 420

Apr 25, 2011, 5:05 PM

Post #3 of 9 (1040 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

On Thu, Apr 21, 2011 at 06:18:40AM +0400, Ruslan Zakirov wrote:
> Hello,
>
> This is still doesn't return old behaviour. Which was with side effects.
>
> Old code was loading .../Web_Local.pm, but code was compiled in
> HTML::Mason::Commands space unless explicit package is defined in
> _Local.pm file.

Okay, how about this instead of the previous patch:

--- lib/RT/Base.pm 18 Apr 2011 23:04:59 -0000 1.1.1.11
+++ lib/RT/Base.pm 25 Apr 2011 23:59:49 -0000
@@ -166,11 +166,11 @@

sub _ImportOverlays {
my $class = shift;
- my ($package,undef,undef) = caller();
- $package =~ s|::|/|g;
+ my ($package, $package_filename, undef) = caller();
+ $package_filename =~ s/\.pm$//;
for (qw(Overlay Vendor Local)) {
- my $filename = $package."_".$_.".pm";
- eval { require $filename };
+ my $filename = $package_filename."_".$_.".pm";
+ eval " package $package; require '$filename'; ";
die $@ if ($@ && $@ !~ qr{^Can't locate $filename});
}
}

This uses the calling filename to determine the overlay names, then
compiles the result in the caller's current package.

Let me know if this works for you.

--
Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
Open-source billing, ticketing and provisioning - http://www.freeside.biz/

> On Wed, Apr 20, 2011 at 9:44 PM, Ivan Kohler <ivan-rt-devel [at] 420> wrote:
> > RT::Interface::Web_Overlay, Web_Vendor and Web_Local are not being
> > called.
> >
> > This is a regression from RT 3.8.9->3.8.10 and also affects 4.0.0rc8
> >
> > RT::Interface::Web is in package HTML::Mason::Commands when it calls
> > RT::Base->_ImportOverlays, so it looks for the wrong files.
> >
> > I figured reworking _ImportOverlays to use the calling filename instead
> > of calling package is not desirable (but I'd be happy to give that a
> > shot if you prefer), so this patch fixes things:
> >
> >
> > diff -u lib/RT/Interface/Web.pm.DIST lib/RT/Interface/Web.pm
> > --- lib/RT/Interface/Web.pm.DIST 2011-04-20 10:33:53.000000000 -0700
> > +++ lib/RT/Interface/Web.pm 2011-04-20 10:34:04.000000000 -0700
> > @@ -2756,6 +2756,7 @@
> > return $scrubber;
> > }
> >
> > +package RT::Interface::Web;
> > RT::Base->_ImportOverlays();
> >
> > 1;
>
>
>
> --
> Best regards, Ruslan.

_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

jesse at bestpractical

Apr 26, 2011, 8:04 AM

Post #4 of 9 (1038 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

On Mon 25.Apr'11 at 17:05:22 -0700, Ivan Kohler wrote:
> On Thu, Apr 21, 2011 at 06:18:40AM +0400, Ruslan Zakirov wrote:
> > Hello,
> >
> > This is still doesn't return old behaviour. Which was with side effects.
> >
> > Old code was loading .../Web_Local.pm, but code was compiled in
> > HTML::Mason::Commands space unless explicit package is defined in
> > _Local.pm file.
>
>
> Okay, how about this instead of the previous patch:
>

I believe killing string eval was an explicit goal of the rewrite.

-j

> --- lib/RT/Base.pm 18 Apr 2011 23:04:59 -0000 1.1.1.11
> +++ lib/RT/Base.pm 25 Apr 2011 23:59:49 -0000
> @@ -166,11 +166,11 @@
>
> sub _ImportOverlays {
> my $class = shift;
> - my ($package,undef,undef) = caller();
> - $package =~ s|::|/|g;
> + my ($package, $package_filename, undef) = caller();
> + $package_filename =~ s/\.pm$//;
> for (qw(Overlay Vendor Local)) {
> - my $filename = $package."_".$_.".pm";
> - eval { require $filename };
> + my $filename = $package_filename."_".$_.".pm";
> + eval " package $package; require '$filename'; ";
> die $@ if ($@ && $@ !~ qr{^Can't locate $filename});
> }
> }
>
>
> This uses the calling filename to determine the overlay names, then
> compiles the result in the caller's current package.
>
> Let me know if this works for you.
>
> --
> Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
> Open-source billing, ticketing and provisioning - http://www.freeside.biz/
>
>
> > On Wed, Apr 20, 2011 at 9:44 PM, Ivan Kohler <ivan-rt-devel [at] 420> wrote:
> > > RT::Interface::Web_Overlay, Web_Vendor and Web_Local are not being
> > > called.
> > >
> > > This is a regression from RT 3.8.9->3.8.10 and also affects 4.0.0rc8
> > >
> > > RT::Interface::Web is in package HTML::Mason::Commands when it calls
> > > RT::Base->_ImportOverlays, so it looks for the wrong files.
> > >
> > > I figured reworking _ImportOverlays to use the calling filename instead
> > > of calling package is not desirable (but I'd be happy to give that a
> > > shot if you prefer), so this patch fixes things:
> > >
> > >
> > > diff -u lib/RT/Interface/Web.pm.DIST lib/RT/Interface/Web.pm
> > > --- lib/RT/Interface/Web.pm.DIST 2011-04-20 10:33:53.000000000 -0700
> > > +++ lib/RT/Interface/Web.pm 2011-04-20 10:34:04.000000000 -0700
> > > @@ -2756,6 +2756,7 @@
> > > return $scrubber;
> > > }
> > >
> > > +package RT::Interface::Web;
> > > RT::Base->_ImportOverlays();
> > >
> > > 1;
> >
> >
> >
> > --
> > Best regards, Ruslan.
>
> _______________________________________________
> List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel
_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

ivan-rt-devel at 420

Apr 26, 2011, 6:48 PM

Post #5 of 9 (1028 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

On Tue, Apr 26, 2011 at 11:04:52PM +0800, Jesse Vincent wrote:
>
> On Mon 25.Apr'11 at 17:05:22 -0700, Ivan Kohler wrote:
> > On Thu, Apr 21, 2011 at 06:18:40AM +0400, Ruslan Zakirov wrote:
> > > Hello,
> > >
> > > This is still doesn't return old behaviour. Which was with side effects.
> > >
> > > Old code was loading .../Web_Local.pm, but code was compiled in
> > > HTML::Mason::Commands space unless explicit package is defined in
> > > _Local.pm file.
> >
> >
> > Okay, how about this instead of the previous patch:
>
> I believe killing string eval was an explicit goal of the rewrite.

So far, I haven't thought of a way to change the current namespace
without string eval. "package" doesn't take a scalar. Any suggestions?
Perhaps it can't be done?

Assuming that's the case, how would you like to reconcile the goal of
killing string eval with Ruslan's request to evaluate the files in the
HTML::Mason::Commands namespace?

1. Give up on eliminating string eval? (the current patch, probably
isolated to a Web.pm-only version of _ImportOverlays)

2. Give up on the idea of changing into the caller's current package?
(HTML::Mason::Commands).

3. Some sort of namespace chicanery to evaluate everything in a
temporary package and then alias it into the desired package? (without
string eval)

#2 is my preference - I don't think preserving the "Web_*.pm overlays
are in the HTML::Mason::Commands namespace" behavior is particularly
important.

I'd be happy to contribute a patch implementing #1 or #2.

#3 may be more than I can bite off Real Soon, but I guess I could give
it a try if that's the only way you'll even consider fixing the original
bug that the files aren't pulled in at all... :)

--
Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
Open-source billing, ticketing and provisioning - http://www.freeside.biz/

_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

tjrc at sanger

Apr 27, 2011, 5:17 AM

Post #6 of 9 (1026 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

On 26 Apr 2011, at 01:05, Ivan Kohler wrote:

> Okay, how about this instead of the previous patch:
>
> --- lib/RT/Base.pm 18 Apr 2011 23:04:59 -0000 1.1.1.11
> +++ lib/RT/Base.pm 25 Apr 2011 23:59:49 -0000
> @@ -166,11 +166,11 @@
>
> sub _ImportOverlays {
> my $class = shift;
> - my ($package,undef,undef) = caller();
> - $package =~ s|::|/|g;
> + my ($package, $package_filename, undef) = caller();
> + $package_filename =~ s/\.pm$//;
> for (qw(Overlay Vendor Local)) {
> - my $filename = $package."_".$_.".pm";
> - eval { require $filename };
> + my $filename = $package_filename."_".$_.".pm";
> + eval " package $package; require '$filename'; ";
> die $@ if ($@ && $@ !~ qr{^Can't locate $filename});
> }
> }

I think this is close, but I don't think you need the string eval. I've hit this same issue breaking RT::Extension::FastGroupRights, and the two changes I'm using are:

1) Allow an override of the filename to be used:

--- RT/Base.pm.orig 2011-04-27 12:48:05.352246706 +0100
+++ RT/Base.pm 2011-04-27 13:14:42.214209587 +0100
@@ -166,7 +166,7 @@

sub _ImportOverlays {
my $class = shift;
- my ($package,undef,undef) = caller();
+ my $package = shift || (caller())[0];
$package =~ s|::|/|g;
for (qw(Overlay Vendor Local)) {
my $filename = $package."_".$_.".pm";

2) Make sure RT::Interface::Web overlays are loaded using that overridden filename:

--- RT/Interface/Web.pm.orig 2011-04-27 12:49:09.149185332 +0100
+++ RT/Interface/Web.pm 2011-04-27 12:52:52.954206821 +0100
@@ -2299,6 +2299,6 @@
return ( _load_container_object( $obj_type, $obj_id ), $search_id );
}

-RT::Base->_ImportOverlays();
+RT::Base->_ImportOverlays('RT::Interface::Web');

1;

No string evals required, and everything seems to be loaded into the right namespace, from the right files. At least, RT::Extension::FastGroupRights works for me now...

Tim

--
The Wellcome Trust Sanger Institute is operated by Genome Research
Limited, a charity registered in England with number 1021457 and a
company registered in England with number 2742969, whose registered
office is 215 Euston Road, London, NW1 2BE.
_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

tjrc at sanger

Apr 27, 2011, 5:51 AM

Post #7 of 9 (1021 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

On 27 Apr 2011, at 13:17, Tim Cutts wrote:

> No string evals required, and everything seems to be loaded into the right namespace, from the right files. At least, RT::Extension::FastGroupRights works for me now...

Ah, bother, it does still require a small change to the extension, of course, as Ruslan said, so that it defines its routines in HTML::Mason::Commands. This does raise the spectre of namespace conflicts, I suppose.

Tim

--
The Wellcome Trust Sanger Institute is operated by Genome Research
Limited, a charity registered in England with number 1021457 and a
company registered in England with number 2742969, whose registered
office is 215 Euston Road, London, NW1 2BE.
_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

ivan-rt-devel at 420

May 18, 2011, 5:39 PM

Post #8 of 9 (867 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

Hi,

Sorry to be a pain. Any idea here now that the dust has settled from
4.0.0? I'd really like to see this fixed and Web_Vendor/_Local files
working again.

I would be happy to contribute a patch if someone from the RT
team could let me know your preference wrt the options for fixing this I
outlined below.

--
Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
Open-source billing, ticketing and provisioning - http://www.freeside.biz/

On Tue, Apr 26, 2011 at 06:48:23PM -0700, Ivan Kohler wrote:
> On Tue, Apr 26, 2011 at 11:04:52PM +0800, Jesse Vincent wrote:
> >
> > On Mon 25.Apr'11 at 17:05:22 -0700, Ivan Kohler wrote:
> > > On Thu, Apr 21, 2011 at 06:18:40AM +0400, Ruslan Zakirov wrote:
> > > > Hello,
> > > >
> > > > This is still doesn't return old behaviour. Which was with side effects.
> > > >
> > > > Old code was loading .../Web_Local.pm, but code was compiled in
> > > > HTML::Mason::Commands space unless explicit package is defined in
> > > > _Local.pm file.
> > >
> > >
> > > Okay, how about this instead of the previous patch:
> >
> > I believe killing string eval was an explicit goal of the rewrite.
>
>
> So far, I haven't thought of a way to change the current namespace
> without string eval. "package" doesn't take a scalar. Any suggestions?
> Perhaps it can't be done?
>
> Assuming that's the case, how would you like to reconcile the goal of
> killing string eval with Ruslan's request to evaluate the files in the
> HTML::Mason::Commands namespace?
>
>
> 1. Give up on eliminating string eval? (the current patch, probably
> isolated to a Web.pm-only version of _ImportOverlays)
>
> 2. Give up on the idea of changing into the caller's current package?
> (HTML::Mason::Commands).
>
> 3. Some sort of namespace chicanery to evaluate everything in a
> temporary package and then alias it into the desired package? (without
> string eval)
>
>
> #2 is my preference - I don't think preserving the "Web_*.pm overlays
> are in the HTML::Mason::Commands namespace" behavior is particularly
> important.
>
> I'd be happy to contribute a patch implementing #1 or #2.
>
> #3 may be more than I can bite off Real Soon, but I guess I could give
> it a try if that's the only way you'll even consider fixing the original
> bug that the files aren't pulled in at all... :)
>
>
> --
> Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
> Open-source billing, ticketing and provisioning - http://www.freeside.biz/
>
> _______________________________________________
> List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

_______________________________________________
List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

falcone at bestpractical

May 19, 2011, 8:32 AM

Post #9 of 9 (865 views)
Permalink

Re: [patch] RT::Interface::Web _Overlay/_Vendor/_Local not being included [In reply to]

On Wed, May 18, 2011 at 05:39:19PM -0700, Ivan Kohler wrote:
> Sorry to be a pain. Any idea here now that the dust has settled from
> 4.0.0? I'd really like to see this fixed and Web_Vendor/_Local files
> working again.

Fixes have been on trunk since yesterday and in branches for a while.
You can follow rt on github here
https://github.com/bestpractical/rt/

-kevin

> I would be happy to contribute a patch if someone from the RT
> team could let me know your preference wrt the options for fixing this I
> outlined below.
>
> --
> Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
> Open-source billing, ticketing and provisioning - http://www.freeside.biz/
>
>
> On Tue, Apr 26, 2011 at 06:48:23PM -0700, Ivan Kohler wrote:
> > On Tue, Apr 26, 2011 at 11:04:52PM +0800, Jesse Vincent wrote:
> > >
> > > On Mon 25.Apr'11 at 17:05:22 -0700, Ivan Kohler wrote:
> > > > On Thu, Apr 21, 2011 at 06:18:40AM +0400, Ruslan Zakirov wrote:
> > > > > Hello,
> > > > >
> > > > > This is still doesn't return old behaviour. Which was with side effects.
> > > > >
> > > > > Old code was loading .../Web_Local.pm, but code was compiled in
> > > > > HTML::Mason::Commands space unless explicit package is defined in
> > > > > _Local.pm file.
> > > >
> > > >
> > > > Okay, how about this instead of the previous patch:
> > >
> > > I believe killing string eval was an explicit goal of the rewrite.
> >
> >
> > So far, I haven't thought of a way to change the current namespace
> > without string eval. "package" doesn't take a scalar. Any suggestions?
> > Perhaps it can't be done?
> >
> > Assuming that's the case, how would you like to reconcile the goal of
> > killing string eval with Ruslan's request to evaluate the files in the
> > HTML::Mason::Commands namespace?
> >
> >
> > 1. Give up on eliminating string eval? (the current patch, probably
> > isolated to a Web.pm-only version of _ImportOverlays)
> >
> > 2. Give up on the idea of changing into the caller's current package?
> > (HTML::Mason::Commands).
> >
> > 3. Some sort of namespace chicanery to evaluate everything in a
> > temporary package and then alias it into the desired package? (without
> > string eval)
> >
> >
> > #2 is my preference - I don't think preserving the "Web_*.pm overlays
> > are in the HTML::Mason::Commands namespace" behavior is particularly
> > important.
> >
> > I'd be happy to contribute a patch implementing #1 or #2.
> >
> > #3 may be more than I can bite off Real Soon, but I guess I could give
> > it a try if that's the only way you'll even consider fixing the original
> > bug that the files aren't pulled in at all... :)
> >
> >
> > --
> > Ivan Kohler, President and Head Geek, Freeside Internet Services, Inc.
> > Open-source billing, ticketing and provisioning - http://www.freeside.biz/
> >
> > _______________________________________________
> > List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel
>
> _______________________________________________
> List info: http://lists.bestpractical.com/cgi-bin/mailman/listinfo/rt-devel

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads