Skip to content

Fixed directory deletion for SMB and FTP storages - #6062

Closed
PVince81 wants to merge 3 commits into
masterfrom
extstorage-deletedirs
Closed

Fixed directory deletion for SMB and FTP storages#6062
PVince81 wants to merge 3 commits into
masterfrom
extstorage-deletedirs

Conversation

@PVince81

Copy link
Copy Markdown
Contributor

Added deleteFileOrDir() operation

Some storages need to use different calls for deleting files or folders, usually unlink() and rmdir().

For this, added a new operation deleteFileOrDir() which default implementation will first detect whether the given argument is a file or a directory to decide which operation to use.

Some storage implementations don't need to distinguish them, in which case they reimplement deleteFileOrDir() to save the extra is_dir() detection.

Fixes #4532 (SMB dir deletion)
Fixes #5941 (FTP dir deletion)

This is based on @icewind1991's suggestion here: #4531 (comment)
I tried to not break the existing APIs.
Currently, only the ajax delete.php is using that new call.

If not acceptable, the alternative is to do the is_dir() detection in delete.php for now...

Please review @icewind1991 @karlitschek @DeepDiver1975 @schiesbn

Vincent Petry added 2 commits November 26, 2013 15:26
Some storages need to use different calls for deleting files or folders,
usually unlink() and rmdir().

For this, added a new operation deleteFileOrDir() which default
implementation will first detect whether the given argument is a file or
a directory to decide which operation to use.

Some storage implementations don't need to distinguish them, in which
case they reimplement deleteFileOrDir() to save the extra is_dir()
detection.

Fixes #4532 (SMB dir deletion)
Fixes #5941 (FTP dir deletion)
Some files appear with a "N" in the file listing from smbclient.
Fixed smb4php to also correctly parse those.
@PVince81

Copy link
Copy Markdown
Contributor Author

Note: I ran the unit tests for FTP, SFTP, SMB, WebDAV and Dropbox and it seemed to work fine.

@karlitschek

Copy link
Copy Markdown
Contributor

yes. 👍

@icewind1991

Copy link
Copy Markdown
Contributor

I would prefer to call the function delete instead of deleteFileOrDir, otherwise 👍

@PVince81

Copy link
Copy Markdown
Contributor Author

I decided against delete() because one might think it is a PHP function (there is even a docs page for it that refers to unlink). deleteFileOrDir() makes it clearer to distinguish from unlink and rmdir than delete.
i do agree that it looks a bit ugly 😉

@PVince81

Copy link
Copy Markdown
Contributor Author

Looks like I forgot some wrappers...

@PVince81

Copy link
Copy Markdown
Contributor Author

Fetch Jenkins, fetch!

@owncloud-bot please retest this

@PVince81

Copy link
Copy Markdown
Contributor Author

Failed to fetch again it seems 😦

@owncloud-bot retest please

@ghost

ghost commented Nov 28, 2013

Copy link
Copy Markdown

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2203/

@MorrisJobke

Copy link
Copy Markdown
Contributor

Is this ready to merge?

@PVince81

Copy link
Copy Markdown
Contributor Author

It should be.
I'm still a bit afraid that the addition to the interface break other things like third party apps that might implement their own storages (probably unlikely).
But since we're increasing the major version, that should be ok ?
What do you think @karlitschek ?

@PVince81

Copy link
Copy Markdown
Contributor Author

👎 had a chat with @schiesbn and we decided to use a less risky approach: put the code for "detect if dir, then use rmdir" inside the specific ext storage classes directly. This way we don't add new API calls and make this backportable to stable5.

@PVince81

Copy link
Copy Markdown
Contributor Author

Superseded by #6123

@PVince81 PVince81 closed this Nov 29, 2013
@MorrisJobke
MorrisJobke deleted the extstorage-deletedirs branch November 29, 2013 15:42
@PVince81

PVince81 commented Dec 2, 2013

Copy link
Copy Markdown
Contributor Author

Backported to stable5 as 3b18c1b

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FTP mount can't delete folders External API for sharing (upload and download)

4 participants