[Dbix-class] Bug in DBIx::Class::ResultSource::remove_columns ?

[Peter Rabbitson]

Oleg Pronin wrote:
> Maybe i misunderstood something but i suppose there is a bug in
> remove_columns.
> 
>   DB<14> x
> $this->columns                                                                                                                                      
> 
> 0  'id'
> ...
> 16  'logo_small'
> 17  'logo_mid'
> 18  'logo_full'
> 19  'screenshots'
> 20  'data'
> 21  'logo'
> 
> $this->remove_column($c);     # $c = 'logo';
> 
>   DB<15> x
> $this->columns                                                                                                                                      
> 
> 0  'id'
> ...
> 16  'logo_mid'
> 17  'logo_full'
> 18  'screenshots'
> 19  'data'
> 
> It removed 'logo_small' as well.
> 
> In DBIx::Class::ResultSource:
> 
> sub remove_columns {
>   my ($self, @cols) = @_;
> 
> 
>   return unless $self->_ordered_columns;
> 
>   my $columns = $self->_columns;
>   my @remaining;
> 
>   foreach my $col (@{$self->_ordered_columns}) {
>     push @remaining, $col unless grep(/$col/, @cols);
> 
>   }
> 
>   foreach (@cols) {
>     delete $columns->{$_};
>   };
> 
>   $self->_ordered_columns(\@remaining);
> }
> 
> 
> What is this:  "grep(/$col/, @cols);" and what for?
> 
> Doc says
> "$table->remove_columns(qw/col1 col2 col3/);
> Removes columns from the result source."
> 
> Not "Removes all columns that looks like" :-)
> 
> Patch:
> 
> sub remove_columns {
>   my ($self, @to_remove) = @_;
> 
> 
>   return unless my @ordered = $self->_ordered_columns;
> 
>   my $columns = $self->_columns;
>   delete $columns->{$_} for @to_remove;
> 
>   my %to_remove = map {$_ => 1} @to_remove;
>   $self->_ordered_columns([grep {!$to_remove{$_}} @ordered]);
> 
> }
> 

Looks like a bug. Patch in the next email seems reasonable. However,
Oleg, you have been around long enough to know the drill - no patches
without tests.