[Bug 43088] New: Koha::File::Transport lacks a consistent current_directory() accessor across FTP/SFTP/Local
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Bug ID: 43088 Summary: Koha::File::Transport lacks a consistent current_directory() accessor across FTP/SFTP/Local Initiative type: --- Sponsorship --- status: Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@openfifth.co.uk QA Contact: testopia@bugs.koha-community.org Depends on: 43078 Target Milestone: --- Koha::File::Transport has no consistent, side-effect-free way to query a transport's current remote working directory. Bug 43078 unified list_files() across FTP/Local/SFTP, but left this gap: Net::SFTP::Foreign's cwd() is a pure read-only accessor, but Net::FTP's own cwd() is a mutator - called with no argument it defaults to "/" and issues a real CWD command, changing the connection's directory as a side effect and returning a boolean rather than a path. This surfaced in the koha-plugin-transport-browser plugin, which read $transport->{connection}->cwd directly to capture the initial working directory for its root/breadcrumb navigation. That happened to work for SFTP, but for FTP it silently reset the connection to the server's root directory and captured "1" (a boolean) as the "path", breaking directory navigation for FTP transports - clicking into a subdirectory just reloaded the same listing instead of navigating in. Depends on bug 43078. Test plan: 1. Apply the attached patch on top of bug 43078. 2. 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 3. koha-qa.pl -v 2 --more-tests reports [PASS] for all changed files with no critic/pod warnings. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43078 [Bug 43078] File transport SFTP backend returns inconsistent list() structure compared to FTP/Local backends -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |martin.renvoize@openfifth.c |ity.org |o.uk CC| |kyle@bywatersolutions.com, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #1 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202007 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202007&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #2 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- *** Bug 43087 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42656 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 [Bug 42656] File transport test responses can be unclear -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202007|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202017 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202017&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 202018 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202018&action=edit Bug 43088: Reuse _current_directory() instead of duplicating pwd() calls _change_directory(), _list_files() and _is_connected() each called $self->{connection}->pwd directly for their log payloads/connectivity check, duplicating exactly what _current_directory() now provides as a proper, documented internal accessor. Route them through it instead - no behaviour change, just removing the duplication now that the wrapper exists. Test plan: 1. prove -r t/db_dependent/Koha/File/ => All tests successful (32 tests), including the existing current_directory() and list_files()/change_directory() coverage. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202017|0 |1 is obsolete| | Attachment #202018|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 202118 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202118&action=edit 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 <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 202119 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202119&action=edit Bug 43088: Reuse _current_directory() instead of duplicating pwd() calls _change_directory(), _list_files() and _is_connected() each called $self->{connection}->pwd directly for their log payloads/connectivity check, duplicating exactly what _current_directory() now provides as a proper, documented internal accessor. Route them through it instead - no behaviour change, just removing the duplication now that the wrapper exists. Test plan: 1. prove -r t/db_dependent/Koha/File/ => All tests successful (32 tests), including the existing current_directory() and list_files()/change_directory() coverage. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #7 from David Nind <david@davidnind.com> --- Testing notes (using KTD): 1. Step 2 (for both patches): qa -v 2 --more-tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette@bywatersolutions.co | |m QA Contact|testopia@bugs.koha-communit |andrew@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202118|0 |1 is obsolete| | Attachment #202119|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #8 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 202831 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202831&action=edit 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 <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 202832 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202832&action=edit Bug 43088: Reuse _current_directory() instead of duplicating pwd() calls _change_directory(), _list_files() and _is_connected() each called $self->{connection}->pwd directly for their log payloads/connectivity check, duplicating exactly what _current_directory() now provides as a proper, documented internal accessor. Route them through it instead - no behaviour change, just removing the duplication now that the wrapper exists. Test plan: 1. prove -r t/db_dependent/Koha/File/ => All tests successful (32 tests), including the existing current_directory() and list_files()/change_directory() coverage. 2. koha-qa.pl -v 2 --more-tests => [PASS], no critic/pod warnings. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |43092 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43092 [Bug 43092] Koha::File::Transport::change_directory(undef) means three different things across FTP, SFTP and Local -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42656 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42656 [Bug 42656] File transport test responses can be unclear -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43088 --- Comment #10 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org