RT::Interface::Web _Overlay/_Vendor/_Local not being included

ivan · April 20, 2011, 5:44pm

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/

Ruslan_Zakirov1 · April 21, 2011, 2:18am

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@420.am 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.

ivan · April 26, 2011, 12:05am

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@420.am 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.

Jesse_Vincent · April 26, 2011, 3:04pm

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

ivan · April 27, 2011, 1:48am

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?

Give up on eliminating string eval? (the current patch, probably
isolated to a Web.pm-only version of _ImportOverlays)
Give up on the idea of changing into the caller’s current package?
(HTML::Mason::Commands).
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/

Tim_Cutts · April 27, 2011, 12:17pm

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:

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";

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.

Tim_Cutts · April 27, 2011, 12:51pm

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.

ivan · May 19, 2011, 12:39am

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?

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

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

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: The rt-devel Archives

Kevin_Falcone · May 19, 2011, 3:32pm

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

-kevin