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

Allow FTP deleteDir function to be recursive on multiple levels #306

Closed
wants to merge 1 commit into from
Closed

Allow FTP deleteDir function to be recursive on multiple levels #306

wants to merge 1 commit into from

Conversation

kristianedlund
Copy link

Hi

Currently the implementation of deleteDir assumes that the ftp_rawlist returns with an array with all files at the end. However, that seems not to be the case for all FTP servers. I have tested both on a synology FTP and pureftp server.

I have changed the function to be recursive instead of relaying on the server to return the array correctly.

Also the ftp_rmdir throws an exception when the file it tries to delete does not exist. I have changed the code to return false instead, to be aligned with the other specification.

PS: I am sorry if I mess something up, this is my very first pull request.

@kristianedlund
Copy link
Author

Rebased the PR to fit with the interface changes in 0.6

@frankdejonge
Copy link
Member

Hi @kristianedlund, could this be moved to the Synology adapter? Seems more appropriate.

@kristianedlund
Copy link
Author

@FrenkyNet I realise that my description is wrong, but the solution is correct.

The problem was that it can't handle nested directories, and that goes for "normal" FTP servers as well. So I think it would be good to include it in the general FTP. Since right now it can only handle one level nesting.

@kristianedlund
Copy link
Author

Just updated the pull request to be compliant with the lastest master.

@GrahamCampbell
Copy link
Member

Could you rebase without the commits please? Also, squashing to one commit would be a good idea.

@kristianedlund
Copy link
Author

@GrahamCampbell Sorry about that. I have very limited experience with Github. I think it is rebased and squashed it into one commit.... I think.

@@ -28,6 +28,11 @@ function ftp_rmdir($connection, $dirname)
return false;
}

if (strpos($dirname, 'rmdir.dontexist') !== false) {
throw new \ErrorException("ftp_rmdir(): Can't remove directory: No such file or directory");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No extra line here please.

@kristianedlund
Copy link
Author

@GrahamCampbell fixed the two issues.

@kristianedlund
Copy link
Author

Updated the PR to be mergable with the current master

@kristianedlund kristianedlund changed the title Updates to the FTP deleteDir function Allow FTP deleteDir function to be recursive on multiple levels Feb 5, 2015
return ftp_rmdir($connection, $dirname);
try {
return ftp_rmdir($connection, $dirname);
} catch (\Exception $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't happen, the original implementation doesn't throw an exception.

@kristianedlund
Copy link
Author

I am pretty sure I managed to get it to throw an exception at some point, which was why I added the try catch. But I can't recreate that situation. So it is removed now.

@frankdejonge
Copy link
Member

This was fixed in a different PR.

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.

None yet

3 participants