Re: DBI: Eliminate inheritance from DynaLoader and Exporter

[CC'd to [email protected] for discussion]

On Tue, Sep 25, 2012 at 09:55:17PM +0200, Max Maischein wrote:
> Hello Tim,
> 
> I was looking at DynaLoader and AutoLoader, and found that some
> old(er) versions of DynaLoader used AutoLoader, like the one
> distributed with 5.10. Having an AUTOLOAD subroutine strikes me as
> an ugly idea, and the easiest fix that works backwards for DBI
> seemed to me to eliminate the inheritance from DynaLoader. That way,
> DynaLoader can still inherit from AutoLoader (in old versions), but
> the inheritance stops there.

> I've attached two patches that eliminate the inheritance from
> DynaLoader and from Exporter. I hope eliminating the inheritance
> from DynaLoader finds your approval. Eliminating the inheritance
> from Exporter is somewhat more controversial as I use a feature of
> Exporter that came into being only with Exporter 5.57, which is core
> since 5.8.3. This would bump the minimum "pristine core" requirement
> of DBI from 5.8.1 to 5.8.3, which you may or may not like. I think
> eliminating unneccessary inheritance is a nice goal, but not at the
> price of bumping the minimum Perl version. If you have other reasons
> to raise the minimum Exporter version or don't see that as an
> impediment. Maybe most people use CPAN to install DBI anyway and
> thus don't care if a higher version of Exporter is a prerequisite.
> 
> I've made my changes against DBI 1.622, because for some reason the
> SVN checkout did fail cloning several times and then didn't pass
> tests at all. On the upside, I built in a directory with whitespace
> in the path and the build still worked. All tests still pass on my
> machine with the changes.
> 
> Anyway, I hope my changes to how DBI uses DynaLoader are in the
> spirit of you like DynaLoader to be used.
> 
> Thanks for writing and maintaining DBI,
> -max

Thanks Max.

For the Exporter change, I don't have a strong opinion on bumping to
minimum requirement to 5.8.3, but others might.

For the DynaLoader change, I suspect it would break "static linking"
(perl Makefile.PL LINKTYPE=static) which some people/environments may
rely on.

Tim.

> >From 359091f215fcbd235a871898e42a23b9e438a4a4 Mon Sep 17 00:00:00 2001
> From: Max Maischein <[email protected]>
> Date: Tue, 25 Sep 2012 21:33:23 +0200
> Subject: [PATCH 2/2] Eliminate inheritance from Exporter.
> 
> This requires Exporter 5.57, which came into core with Perl 5.8.3.
> Currently, DBI only requires 5.8.1, so this may be undesireable.
> ---
>  DBI.pm |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/DBI.pm b/DBI.pm
> index badb6fd..acc6dda 100644
> --- a/DBI.pm
> +++ b/DBI.pm
> @@ -156,10 +156,9 @@ L<http://search.cpan.org/search?query=DBI&mode=all>.
>  
>  use Carp();
>  use DynaLoader ();
> -use Exporter ();
> +use Exporter 'import';
>  
>  BEGIN {
> -@ISA = qw(Exporter);
>          *dl_load_flags = \&DynaLoader::dl_load_flags;
>  
>  # Make some utility functions available if asked for
> -- 
> 1.6.5.1.1367.gcd48
> 

> >From 5cb368816753c526895a68aa411711e33675ac3a Mon Sep 17 00:00:00 2001
> From: Max Maischein <[email protected]>
> Date: Tue, 25 Sep 2012 21:32:19 +0200
> Subject: [PATCH 1/2] Eliminate inheritance from DynaLoader
> 
> ---
>  DBI.pm |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/DBI.pm b/DBI.pm
> index 9b39b14..badb6fd 100644
> --- a/DBI.pm
> +++ b/DBI.pm
> @@ -159,7 +159,8 @@ use DynaLoader ();
>  use Exporter ();
>  
>  BEGIN {
> -@ISA = qw(Exporter DynaLoader);
> +@ISA = qw(Exporter);
> +        *dl_load_flags = \&DynaLoader::dl_load_flags;
>  
>  # Make some utility functions available if asked for
>  @EXPORT    = ();		    # we export nothing by default
> @@ -251,12 +252,12 @@ $DBI::stderr = 2_000_000_000; # a very round number below 2**31
>  # then you haven't installed the DBI correctly. Read the README
>  # then install it again.
>  if ( $ENV{DBI_PUREPERL} ) {
> -    eval { bootstrap DBI } if       $ENV{DBI_PUREPERL} == 1;
> +    eval { __PACKAGE__->DynaLoader::bootstrap( $VERSION ) } if       $ENV{DBI_PUREPERL} == 1;
>      require DBI::PurePerl  if $@ or $ENV{DBI_PUREPERL} >= 2;
>      $DBI::PurePerl ||= 0; # just to silence "only used once" warnings
>  }
>  else {
> -    bootstrap DBI;
> +    __PACKAGE__->DynaLoader::bootstrap( $VERSION );
>  }
>  
>  $EXPORT_TAGS{preparse_flags} = [ grep { /^DBIpp_\w\w_/ } keys %{__PACKAGE__."::"} ];
> -- 
> 1.6.5.1.1367.gcd48
>