Re: [cgiapp] Re: critical bug in 'import' of CGI::App 3.21 (test case available)

[Cees Hek <suppressed>]

For those that are interested, here is a test case that demonstrates the 
new problem I noticed late last night with the 'import' patch that 
appeared in CGI::Application 3.21.  Please note that this is a different 
problem then the one Mark found, and I think it is potentially more 
serious...

The code sample below will fail with the following error:

Undefined subroutine &CGI::Application::croak called at 
/usr/local/share/perl/5.8.3/CGI/Application.pm line 71.

This is because 'import' is never called because of the () in the use 
CGI::Application statement.  This in turn means that neither Carp or 
CGI::Carp are loaded and 'croak' is never imported into the namespace. 
So when CGI::App tries to croak it fails.

I think that perhaps it is best to just remove the need for CGI::Carp 
completely from CGI::Application and just use Carp instead.  Then a user 
that really wants to use CGI::Carp can load it themselves (CGI::Carp 
sets *CORE::GLOBAL::die = \&CGI::Carp::die, so CGI::Application will 
still use the CGI::Carp'ified die regardless of where CGI::Carp is 
loaded).  This isn't going to break any code, it will just mean the 
error messages are printed to the error log without timestamps.

Anyway, here is the sample test case that dies with 3.21.  Just place 
all of the code in a .pl file and execute it (no need to split it into a 
.pm file and a .pl file):

-----------------------------
#!/usr/bin/perl

use strict;
use warnings;

my $app = MyApp->new(
            PARAMS => [], # triggers a croak on line 71
                          # PARAMS should be a hashref not arrayref
);

$app->run;

package MyApp;

# The () means that import is never called
use CGI::Application 3.21 ();
use base 'CGI::Application';

# The rest of the code is not important,
# but left in for completeness
sub setup {
  my $self = shift;
  $self->start_mode('testmode');
  $self->run_modes(
    'testmode' => 'testmode',
  );
}

sub testmode {
  my $self = shift;

  return "testing runmode\n";
}

1;
--------------------------------------


Cees Hek wrote:
> Mark Stosberg wrote:
>
> > I understand what you are saying and agree that it would be best to fix
> > this in my code. However, look at like at it like this: I upgraded
> > CGI::App and many products quit working, all of which wouldn't be easy
> > to track down.  If this happened to me, this will happen to other
> > people.
> > The solution that makes sense to me is for CGI::App have an import
> > function that calls Exporter import to provide a 'standard' import
> > function, and then additionally adds the 'cgicarp' code to the mix. 
> > Does that seem reasonable or does it just sound like a hack?
>
>
> Again I'll reiterate that I don't use Exporter myself, so my comments 
> should be taken with that in mind.
>
> The following replacement import function fixes this particular problem, 
> but after writing it, I am not convinced that this is really what should 
> be done...
>
> sub import {
>        my $class = shift;
>
>        if ($class ne 'CGI::Application') {
>          warn "CGI::Application's import method was called for $class 
> indicating there may be an Exporter inheritance problem in $class";
>          # use Exporter's import which is probably what they want
>          require Exporter;
>          return Exporter::export_to_level($class, 1, @_);
>        }
>        my $cgicarp = 1;
>        foreach (@_) { $cgicarp = 0 if /^-nocgicarp$/io }
>        if ($cgicarp) {
>                require CGI::Carp;
>                CGI::Carp->import();
>        }
>        else {
>                require Carp;
>                Carp->import();
>        }
> }
>
> What this is really doing is potentially hiding a coding error!  I would 
> consider that a bad thing.
>
>
> Also, while I was playing with this problem I discovered another problem 
> with the new import method!
>
> use CGI::Application ();
> push @ISA, 'CGI::Application';
>
> That is a pretty standard way of inheriting from CGI::Application.  But 
> 'import' is never called because of the ().  What this means is that 
> neither Carp nor CGI::Carp are loaded.  The following doesn't work either:
>
> use base 'CGI::Application';
>
> since base.pm doesn't call import (it uses require to load the module if 
> it isn't already loaded).  Using base.pm does end up loading Carp 
> (base.pm uses vars.pm which uses warnings/register.pm which requires 
> warnings.pm which uses Carp.pm), but croak and carp will not be imported 
> into the CGI::Application namespace so it is still a problem.
>
> You have to do the following to get the import method to be executed 
> properly:
>
> use CGI::Application;
> push @ISA, 'CGI::Application';
>
> - or -
>
> use CGI::Application;
> use base 'CGI::Application';
>
>
> I can't see a simple solution for that problem.  Either Carp or 
> CGI::Carp must be use'd in the main scope since there is no guarantee 
> that import will be called.  And once CGI::Carp has been loaded, it will 
> be very messy to undo all the things it sets up...
>
> So that leaves using Carp by default, and using CGI::Carp if requested. 
>  Which happens to not be backwards compatible to older versions of 
> CGI::Application (it only affects error/warning messages, so I don't 
> think it is a big deal to change).
>
> Anyway, I think this will require some more discusion to come up with an 
> amicable solution.
>
> Cheers,
>
> Cees
>
> ---------------------------------------------------------------------
> Web Archive:  http://www.mail-archive.com/suppressed/
>              http://marc.theaimsgroup.com/?l=cgiapp&r=1&w=2
> To unsubscribe, e-mail: suppressed
> For additional commands, e-mail: suppressed


---------------------------------------------------------------------
Web Archive:  http://www.mail-archive.com/suppressed/
             http://marc.theaimsgroup.com/?l=cgiapp&r=1&w=2
To unsubscribe, e-mail: suppressed
For additional commands, e-mail: suppressed