You've done a lot of interesting stuff with Catalyst, and I'm not trying to insult your work. As I said, the issues with the code are not bad. They won't stop me from using Catalyst. They will stop me from recommending the source code to newbies as an example to be copied though, mostly because of how it strays from established perl practices.
The use of import() I'm referring to is in Catalyst.pm. The documentation says that some options can be passed when calling use Catalyst to load plugins and activate debugging. It doesn't say that if you don't call import the whole thing will blow up. I discovered this when I wanted to turn off debugging and changed the line in my auto-generated controller module from use Catalyst qw/-Debug/; to use Catalyst qw//;. Not importing anything is a common memory optimization technique under mod_perl.
I was curious about why skipping the import kills everything, so I looked at the code, and found this:
# Prepare inheritance
unless ( $caller->isa($self) ) {
no strict 'refs';
push @{"$caller\::ISA"}, $self;
}
Perl has well-established ways to set up inheritance. Messing with the caller's @ISA from your module's import() method is not a clean way to do it, and the documentation doesn't mention it being done at all. It does the same for the dispatcher class.
When I see no strict 'refs', it's a signal that something bad is about to happen. The uses here seem unncessary. There are other ways to keep track of a debug setting than adding a method to someone else's symbol table.
As long as we're talking about this code, I would also suggest breaking up this long method into smaller ones, and avoiding the use of UNIVERSAL::require to put a method into everyone else's namespace, but those are very minor things.
The other thing I referred to is not Catalyst::Base. I didn't look at that code. I was talking about Class::DBI::FromForm. It's arguably not directly a part of Catalyst, but it is a key component of Catalyst::Model::CDBI::CRUD and was written by sri, so the distinction is kind of academic.
While looking at how that module works, I found this code:
sub _run_create {
my ( $me, $class, $results ) = @_;
my $them = bless {}, $class;
my $cols = {};
foreach my $col ( $them->columns('All') ) {
$cols->{$col} = $results->valid($col);
}
return $class->create($cols);
}
This code blesses a hashref into the class of your Class::DBI module and calls a method on it. It assumes that any Class::DBI subclass will function happily as a hash with no data, and respond correctly to a columns() method call. It's a total violation of encapsulation and will break if Class::DBI changes its internals significantly in the future.
I'm not trying to make a big case out of these things. I don't think they are serious enough problems to merit a bug report, or even a complaint on the mailing list. They are certainly no worse than what I've seen in many other CPAN modules, and better than most. I do think they make the source a questionable example for young coders in training though. I hope you can see why and won't take it too personally. |