From 7a750c7e8350c80ede7823a9e9aba20b6a84cc10 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 16 Jul 2026 11:13:05 +0100 Subject: [PATCH] Bug 43088: Add a consistent current_directory() accessor to Koha::File::Transport FTP, Local and SFTP list_files() were unified to return the same flat shape, but consumers still had no portable way to ask a transport for its current remote directory - the underlying libraries' own methods of that name don't agree on what "cwd" means. Net::SFTP::Foreign's cwd() is a pure read-only accessor, but Net::FTP's cwd() is a mutator: called with no argument it defaults to "/", issues a real CWD command, and returns a boolean rather than a path. The koha-plugin-transport-browser plugin discovered this the hard way: it read $transport->{connection}->cwd directly to capture the initial working directory, which happened to work for SFTP but silently reset the FTP connection to root and captured "1" (a boolean) as the "path" on every request, breaking directory navigation for FTP transports. Add a public current_directory() method to the Koha::File::Transport base class, following the same connect-on-demand pattern as list_files()/upload_file()/etc, with a _current_directory() hook each subclass implements using its own real read-only accessor: pwd() for FTP, cwd() for SFTP, and the tracked {current_directory} (falling back to download_directory) for Local. Consumers should use this instead of reaching into transport internals. Test plan: 1. prove t/db_dependent/Koha/File/Transport.t t/db_dependent/Koha/File/Transport/FTP.t t/db_dependent/Koha/File/Transport/Local.t t/db_dependent/Koha/File/Transport/SFTP.t => All tests successful, including the new current_directory() tests subtest in each transport test file, which mock the underlying libraries (Net::FTP / Net::SFTP::Foreign) to assert each backend reads its own real read-only accessor. 2. koha-qa.pl -v 2 --more-tests => [PASS] for all four changed modules, no critic/pod warnings. Signed-off-by: David Nind Signed-off-by: Andrew Fuerste Henry Signed-off-by: Pedro Amorim --- Koha/File/Transport.pm | 39 +++++++++++++++++++++ Koha/File/Transport/FTP.pm | 20 +++++++++++ Koha/File/Transport/Local.pm | 16 +++++++++ Koha/File/Transport/SFTP.pm | 18 ++++++++++ t/db_dependent/Koha/File/Transport/FTP.t | 33 +++++++++++++++++- t/db_dependent/Koha/File/Transport/Local.t | 40 +++++++++++++++++++++- t/db_dependent/Koha/File/Transport/SFTP.t | 33 +++++++++++++++++- 7 files changed, 196 insertions(+), 3 deletions(-) diff --git a/Koha/File/Transport.pm b/Koha/File/Transport.pm index 30375cb24f..8146aa5f41 100644 --- a/Koha/File/Transport.pm +++ b/Koha/File/Transport.pm @@ -244,6 +244,45 @@ sub _is_connected { die "Subclass must implement _is_connected"; } +=head3 current_directory + + my $path = $transport->current_directory(); + +Returns the transport's current remote working directory as a plain string, +without side effects. Automatically establishes a connection if needed. + +B This is a read-only query. It must never navigate the connection - +unlike some underlying libraries' own C/C methods, which differ +in meaning between backends (see subclass implementations). + +B The current directory path on success, undef on failure. Check +object_messages() for details. + +=cut + +sub current_directory { + my ($self) = @_; + + return unless $self->_ensure_connected(); + + return $self->_current_directory(); +} + +=head3 _current_directory + + my $path = $transport->_current_directory(); + +Internal method that returns the protocol-specific current working +directory. Must be implemented by subclasses. Called by current_directory() +after connection verification. + +=cut + +sub _current_directory { + my ($self) = @_; + die "Subclass must implement _current_directory"; +} + =head3 _auto_change_directory $transport->_auto_change_directory($dir_type, $custom_path); diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm index e6ad0c49d3..11ab02f539 100644 --- a/Koha/File/Transport/FTP.pm +++ b/Koha/File/Transport/FTP.pm @@ -123,6 +123,26 @@ sub _download_file { return 1; } +=head3 _current_directory + + my $path = $self->_current_directory; + +Internal method that returns the FTP connection's current working directory. + +Net::FTP's own C method is a mutator: called with no argument it +defaults to "/" and issues a real C command, changing the connection's +directory as a side effect and returning a boolean rather than a path. The +read-only equivalent is C, which sends PWD and parses the path out of +the response without navigating anywhere. + +=cut + +sub _current_directory { + my ($self) = @_; + + return $self->{connection}->pwd; +} + =head3 _change_directory my $success = $server->_change_directory($directory); diff --git a/Koha/File/Transport/Local.pm b/Koha/File/Transport/Local.pm index e27908b4dc..c2076ec1cd 100644 --- a/Koha/File/Transport/Local.pm +++ b/Koha/File/Transport/Local.pm @@ -254,6 +254,22 @@ sub _change_directory { return 1; } +=head3 _current_directory + + my $path = $self->_current_directory; + +Internal method that returns the local transport's current working +directory: the directory most recently set via change_directory(), falling +back to the configured download_directory, then to '.'. + +=cut + +sub _current_directory { + my ($self) = @_; + + return $self->{current_directory} || $self->download_directory || '.'; +} + =head3 _list_files Internal method that performs the local file system file listing operation. diff --git a/Koha/File/Transport/SFTP.pm b/Koha/File/Transport/SFTP.pm index 31d7f3ab45..6d7ef73b10 100644 --- a/Koha/File/Transport/SFTP.pm +++ b/Koha/File/Transport/SFTP.pm @@ -138,6 +138,24 @@ sub _download_file { return 1; } +=head3 _current_directory + + my $path = $self->_current_directory; + +Internal method that returns the SFTP connection's current working directory. + +Net::SFTP::Foreign's C is a pure read-only accessor - it reports the +current remote directory without navigating - unlike FTP's C (see +Koha::File::Transport::FTP::_current_directory). + +=cut + +sub _current_directory { + my ($self) = @_; + + return $self->{connection}->cwd; +} + =head3 _change_directory my $success = $server->_change_directory($directory); diff --git a/t/db_dependent/Koha/File/Transport/FTP.t b/t/db_dependent/Koha/File/Transport/FTP.t index b1bdc19b91..29ff8f57ea 100755 --- a/t/db_dependent/Koha/File/Transport/FTP.t +++ b/t/db_dependent/Koha/File/Transport/FTP.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 8; +use Test::More tests => 9; use Test::Exception; use Test::NoWarnings; use Test::Warn; @@ -135,6 +135,37 @@ subtest 'change_directory() tests' => sub { can_ok( $transport, 'change_directory' ); }; +subtest 'current_directory() tests' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'ftp', password => 'testpass' } + } + ); + + can_ok( $transport, 'current_directory' ); + + # Net::FTP's own cwd() is a mutator (no-arg means "change to /" and + # returns a boolean) - current_directory() must read pwd(), the real + # read-only accessor, never cwd(). + my $mock_ftp = Test::MockModule->new('Net::FTP'); + $mock_ftp->mock( 'pwd', sub { return '/incoming/'; } ); + $mock_ftp->mock( 'cwd', sub { die 'cwd() must not be called by current_directory()'; } ); + + $transport->{connection} = bless {}, 'Net::FTP'; + $transport->{_user_set_directory} = 1; + + is( $transport->current_directory, '/incoming/', 'current_directory() reads pwd(), not cwd()' ); + + $transport->{connection} = undef; + + $schema->storage->txn_rollback; +}; + subtest 'list_files() MLSD tests' => sub { plan tests => 16; diff --git a/t/db_dependent/Koha/File/Transport/Local.t b/t/db_dependent/Koha/File/Transport/Local.t index 2653e6dc80..1b6691cc17 100755 --- a/t/db_dependent/Koha/File/Transport/Local.t +++ b/t/db_dependent/Koha/File/Transport/Local.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 2; +use Test::More tests => 3; use Test::NoWarnings; use File::Temp qw( tempdir ); use File::Spec; @@ -79,4 +79,42 @@ subtest 'list_files() tests' => sub { $schema->storage->txn_rollback; }; +subtest 'current_directory() tests' => sub { + plan tests => 3; + + $schema->storage->txn_begin; + + my $tempdir = tempdir( CLEANUP => 1 ); + my $download_dir = File::Spec->catdir( $tempdir, 'download' ); + mkdir $download_dir or die "Cannot create download_dir: $!"; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { + transport => 'local', + download_directory => $download_dir, + } + } + ); + + can_ok( $transport, 'current_directory' ); + + is( + $transport->current_directory, $download_dir, + 'current_directory() falls back to the configured download_directory before any change_directory() call' + ); + + my $subdir_path = File::Spec->catdir( $download_dir, 'incoming' ); + mkdir $subdir_path or die "Cannot create subdir: $!"; + + $transport->change_directory($subdir_path); + is( + $transport->current_directory, $subdir_path, + "current_directory() reflects the most recent change_directory() call" + ); + + $schema->storage->txn_rollback; +}; + 1; diff --git a/t/db_dependent/Koha/File/Transport/SFTP.t b/t/db_dependent/Koha/File/Transport/SFTP.t index 3fdb781a35..cac5f9d64b 100755 --- a/t/db_dependent/Koha/File/Transport/SFTP.t +++ b/t/db_dependent/Koha/File/Transport/SFTP.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 10; use Test::Exception; use Test::NoWarnings; use Test::Warn; @@ -249,6 +249,37 @@ subtest 'change_directory() tests' => sub { can_ok( $transport, 'change_directory' ); }; +subtest 'current_directory() tests' => sub { + plan tests => 2; + + $schema->storage->txn_begin; + + my $transport = $builder->build_object( + { + class => 'Koha::File::Transports', + value => { transport => 'sftp', password => 'testpass' } + } + ); + + can_ok( $transport, 'current_directory' ); + + # Net::SFTP::Foreign's cwd() is a pure read-only accessor - unlike FTP's + # cwd(), it never navigates - so current_directory() can read it directly. + my $undef; + my $mock_sftp = Test::MockModule->new('Net::SFTP::Foreign'); + $mock_sftp->mock( 'error', sub { return $undef; } ); + $mock_sftp->mock( 'cwd', sub { return '/home/vendor'; } ); + + $transport->{connection} = bless {}, 'Net::SFTP::Foreign'; + $transport->{_user_set_directory} = 1; + + is( $transport->current_directory, '/home/vendor', "current_directory() reads the SFTP connection's cwd()" ); + + $transport->{connection} = undef; + + $schema->storage->txn_rollback; +}; + subtest 'list_files() tests' => sub { plan tests => 12;