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

Refactor recursiveDelete #121

Closed
wants to merge 5 commits into from
Closed

Refactor recursiveDelete #121

wants to merge 5 commits into from

Conversation

stjosh
Copy link
Contributor

@stjosh stjosh commented May 27, 2017

The recursiveDelete method causes problems in certain environments with the RecursiveDirectoryIterator not iterating through all elements of a tree when deleting nodes while iterating. See nextcloud/server#2737 for a more detailed discussion of the issue.

This is solved by first completing the iteration and only then deleting all elements (first the files, then the directories).

The recursiveDelete method causes problems in certain environments with the RecursiveDirectoryIterator not iterating through all elements of a tree when deleting nodes while iterating.
This is solved by first completing the iteration and only then deleting all elements.
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Could you apply the very same changes to the lib/Updater.php -

updater/lib/Updater.php

Lines 675 to 678 in 8b3d7d0

foreach ($iterator as $fileInfo) {
$action = $fileInfo->isDir() ? 'rmdir' : 'unlink';
$action($fileInfo->getRealPath());
}

This is also check in the integration tests and they will then succeed, if the changes are the very same.

Beside that the change looks good. 👍

Thanks

@stjosh
Copy link
Contributor Author

stjosh commented Jun 5, 2017

Changes also applied to lib/Updater.php. Hope that makes it work. :)

@caco3
Copy link

caco3 commented Aug 17, 2017

What is the reason this pull request got stuck?
The CI-Link https://drone.nextcloud.com/nextcloud/updater/103 only shows "page not found", then redirects to an almost empty overview page...

@stjosh
Copy link
Contributor Author

stjosh commented Aug 18, 2017

Ok, sending a dummy commit triggered the build pipeline again - the tests still failing (php55-cli, signed-off-check) look like stuff I can't do much about, right? @MorrisJobke: Another review and merging would be highly appreciated - updating from 12.0.1 to 12.0.2 was a pain again... :-/

@MorrisJobke
Copy link
Member

the tests still failing (php55-cli, signed-off-check)

A rebase helps here.

What is the reason this pull request got stuck?

I had I open but then forgot to answer here, that a rebase would help to get this in. Let me do this and open a separate PR for this and get it merged.

@MorrisJobke
Copy link
Member

I rebased this and squashed all commits into one. Let's see what CI says to it: #132

@stjosh
Copy link
Contributor Author

stjosh commented Aug 20, 2017

Ah I see - rebasing would have done the trick. Sorry for not noticing that. Anyway, #132 seems to make CI happy, so I'll close this one. Thanks for your help @MorrisJobke

@stjosh stjosh closed this Aug 20, 2017
@MorrisJobke
Copy link
Member

Thanks for your help @MorrisJobke

Thanks for your contribution 👍

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

Successfully merging this pull request may close these issues.

3 participants