Bugzilla – Attachment 187272 Details for
Bug 38489
EDI should be updated to use the new FTP/SFTP Servers management page
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38489: Standardize FTP transport API to match SFTP and Local classes
8ed390d.patch (text/plain), 6.60 KB, created by
Kyle M Hall (khall)
on 2025-10-02 13:35:36 UTC
(
hide
)
Description:
Bug 38489: Standardize FTP transport API to match SFTP and Local classes
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2025-10-02 13:35:36 UTC
Size:
6.60 KB
patch
obsolete
>From 8ed390dfe4ec0a8f3b31e64c9a26d1384b164f3b Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Tue, 23 Sep 2025 21:38:20 +0100 >Subject: [PATCH] Bug 38489: Standardize FTP transport API to match SFTP and > Local classes > >Fix several API inconsistencies in the FTP transport class: > >1. Message format: Use standardized `message => $operation` format > instead of hardcoded strings like "Upload succeeded" > >2. Operation tracking: Add $operation variable to all methods for > consistent error reporting and logging > >3. Error handling: Update _abort_operation() to accept operation > parameter and provide more detailed error payloads > >4. Success payloads: Replace empty `detail => ''` payloads with > meaningful operation-specific information (host, port, file paths, > directory info, file counts) > >All transport classes now have consistent APIs for: >- Standardized message formats >- Consistent operation tracking >- Uniform error handling >- Informative success payloads > >Sponsored-by: ByWater Solutions >Signed-off-by: Hannah Dunne-Howrie <hdunne-howrie@westminster.gov.uk> > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > Koha/File/Transport/FTP.pm | 67 ++++++++++++++++++++++++++------------ > 1 file changed, 46 insertions(+), 21 deletions(-) > >diff --git a/Koha/File/Transport/FTP.pm b/Koha/File/Transport/FTP.pm >index c6208d4cc24..f965ebd0ede 100644 >--- a/Koha/File/Transport/FTP.pm >+++ b/Koha/File/Transport/FTP.pm >@@ -37,22 +37,27 @@ Start the FTP transport connect, returns true on success or undefined on failure > > sub connect { > my ($self) = @_; >+ my $operation = "connection"; > > $self->{connection} = Net::FTP->new( > $self->host, > Port => $self->port, > Timeout => $self->DEFAULT_TIMEOUT, > Passive => $self->passive ? 1 : 0, >- ) or return $self->_abort_operation(); >+ ) or return $self->_abort_operation($operation); > > $self->{connection}->login( $self->user_name, $self->plain_text_password ) >- or return $self->_abort_operation(); >+ or return $self->_abort_operation($operation); > > $self->add_message( > { >- message => "Connect succeeded", >+ message => $operation, > type => 'success', >- payload => { detail => '' } >+ payload => { >+ status => 'connected', >+ host => $self->host, >+ port => $self->port >+ } > } > ); > >@@ -71,15 +76,19 @@ Returns true on success or undefined on failure. > > sub upload_file { > my ( $self, $local_file, $remote_file ) = @_; >+ my $operation = "upload"; > > $self->{connection}->put( $local_file, $remote_file ) >- or return $self->_abort_operation(); >+ or return $self->_abort_operation($operation); > > $self->add_message( > { >- message => "Upload succeeded", >+ message => $operation, > type => 'success', >- payload => { detail => '' } >+ payload => { >+ local_file => $local_file, >+ remote_file => $remote_file >+ } > } > ); > >@@ -98,15 +107,19 @@ Returns true on success or undefined on failure. > > sub download_file { > my ( $self, $remote_file, $local_file ) = @_; >+ my $operation = "download"; > > $self->{connection}->get( $remote_file, $local_file ) >- or return $self->_abort_operation(); >+ or return $self->_abort_operation($operation); > > $self->add_message( > { >- message => "Download succeeded", >+ message => $operation, > type => 'success', >- payload => { detail => '' } >+ payload => { >+ remote_file => $remote_file, >+ local_file => $local_file >+ } > } > ); > >@@ -125,14 +138,18 @@ Returns true on success or undefined on failure. > > sub change_directory { > my ( $self, $remote_directory ) = @_; >+ my $operation = "change_directory"; > >- $self->{connection}->cwd($remote_directory) or $self->_abort_operation(); >+ $self->{connection}->cwd($remote_directory) or return $self->_abort_operation($operation); > > $self->add_message( > { >- message => "Changed directory succeeded", >+ message => $operation, > type => 'success', >- payload => { detail => '' } >+ payload => { >+ directory => $remote_directory, >+ pwd => $self->{connection}->pwd >+ } > } > ); > >@@ -150,9 +167,10 @@ Each hashref contains: filename, longname, size, perms. > > sub list_files { > my ($self) = @_; >+ my $operation = "list"; > > # Get detailed listing using dir() for consistency with SFTP format >- my $detailed_list = $self->{connection}->dir or return $self->_abort_operation(); >+ my $detailed_list = $self->{connection}->dir or return $self->_abort_operation($operation); > > # Convert to hash format consistent with SFTP > my @file_list; >@@ -177,9 +195,12 @@ sub list_files { > > $self->add_message( > { >- message => "Listing files succeeded", >+ message => $operation, > type => 'success', >- payload => { detail => '' } >+ payload => { >+ count => scalar @file_list, >+ pwd => $self->{connection}->pwd >+ } > } > ); > >@@ -198,12 +219,13 @@ Returns true on success or undefined on failure. > > sub rename_file { > my ( $self, $old_name, $new_name ) = @_; >+ my $operation = "rename"; > >- $self->{connection}->rename( $old_name, $new_name ) or return $self->_abort_operation(); >+ $self->{connection}->rename( $old_name, $new_name ) or return $self->_abort_operation($operation); > > $self->add_message( > { >- message => "File rename succeeded", >+ message => $operation, > type => 'success', > payload => { detail => "$old_name -> $new_name" } > } >@@ -232,13 +254,16 @@ sub disconnect { > } > > sub _abort_operation { >- my ( $self, $message ) = @_; >+ my ( $self, $operation ) = @_; > > $self->add_message( > { >- message => $self->{connection} ? $self->{connection}->message : $@, >+ message => $operation || 'operation', > type => 'error', >- payload => { detail => $self->{connection} ? $self->{connection}->status : '' } >+ payload => { >+ detail => $self->{connection} ? $self->{connection}->status : '', >+ error => $self->{connection} ? $self->{connection}->message : $@ >+ } > } > ); > >-- >2.39.5 (Apple Git-154)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 38489
:
175204
|
175205
|
175206
|
175207
|
176118
|
176119
|
176161
|
185324
|
186819
|
186820
|
186845
|
186846
|
186847
|
186848
|
186849
|
186850
|
186851
|
186852
|
186854
|
187080
|
187081
|
187082
|
187083
|
187084
|
187085
|
187086
|
187087
|
187088
|
187089
|
187108
|
187147
|
187148
|
187149
|
187150
|
187151
|
187152
|
187153
|
187154
|
187155
|
187156
|
187157
|
187158
|
187159
|
187270
|
187271
|
187272
|
187273
|
187274
|
187275
|
187276
|
187277
|
187278
|
187279
|
187280
|
187281
|
187283
|
187284
|
187285
|
187286
|
187287
|
187288
|
187289
|
187290
|
187291
|
187292
|
187293
|
187294