Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SftpAdapter not overwriting on move #1682

Merged
merged 4 commits into from
Aug 11, 2024

Conversation

kynx
Copy link

@kynx kynx commented Jul 14, 2023

Currently the SftpAdapter doesn't overwrite existing files when moving, unlike other adapters. This PR adds that ability.

@kynx
Copy link
Author

kynx commented Jul 14, 2023

Moving and copying are both deterministic operations. This means they will always overwrite the target location, and parent directories are always created (if and when needed).

(from https://flysystem.thephpleague.com/docs/usage/filesystem-api/ )

Judging by the failing tests this is based on some alternative facts 😉 But fixing the other adapters should probably be done in separate PRs.

@kynx
Copy link
Author

kynx commented Jul 17, 2023

OK, I've moved the moving_a_file_and_overwriting() and moving_a_directory_and_overwriting() tests to SftpAdapterTest so that the other adapter tests no longer fail and this will stand a chance of getting merged. Once the other adapters are fixed these tests should be added back to FilesystemAdapterTestCase

@kynx kynx changed the title Sftp move and overwrite Fix SftpAdapter not overwriting on move Jul 18, 2023
@kynx
Copy link
Author

kynx commented Aug 15, 2023

@frankdejonge Apologies, I totally missed that this had conflicts. Those are resolved now.

kynx pushed a commit to kynx/flysystem-sftp-v3 that referenced this pull request Aug 30, 2023
# Conflicts:
#	src/PhpseclibV3/SftpAdapterTest.php
@SamMousa
Copy link

SamMousa commented Aug 9, 2024

@kynx I'm gonna try and get posix_rename support in PHPSecLib.

phpseclib/phpseclib#2024

Just an FYI. If that works out we should support it in this adapter as well (and fallback to the manual solution if not supported).

Edit:
I've also implemented a generic solution for all adapters here: collecthor/flysystem-adapters#7
Of course this is suboptimal but it provides a path forward at least.

@kynx
Copy link
Author

kynx commented Aug 9, 2024

@SamMousa great idea with the posix_rename stuff! I've always felt a little nervous around the phpseclib code 😄

The generic adapter solution is also a worthwhile compromise - I should have thought of something like that.

TBH I'd completely forgotten this PR, but just checked and oops - it's been in production on one project for over a year! Got to find some tool to send reminders when something lurks in composer's' repositories too long 😉

@frankdejonge frankdejonge merged commit c6cc4fe into thephpleague:3.x Aug 11, 2024
6 checks passed
@SamMousa
Copy link

@kynx posix_rename has been merged: phpseclib/phpseclib#2029

Are you up for adding support to the sftp adapter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants