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

RecursiveDirectoryIterator does not work on NFS #8377

Merged
merged 5 commits into from
May 2, 2014
Merged

RecursiveDirectoryIterator does not work on NFS #8377

merged 5 commits into from
May 2, 2014

Conversation

pluijm
Copy link
Contributor

@pluijm pluijm commented Apr 28, 2014

Please see issue #8376

@ghost
Copy link

ghost commented Apr 28, 2014

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@pluijm
Copy link
Contributor Author

pluijm commented Apr 28, 2014

This contribution is MIT licensed

@DeepDiver1975
Copy link
Member

This contribution is MIT licensed

Thanks a lot!

@DeepDiver1975
Copy link
Member

@owncloud-bot this is ok to test

@ghost
Copy link

ghost commented Apr 28, 2014

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

@DeepDiver1975
Copy link
Member

@pluijm Can you please have a look at the failing unit tests? Something is not working properly - THX

@pluijm
Copy link
Contributor Author

pluijm commented Apr 28, 2014

At first I couldn't reproduce the error when I used a tmp directory in my home dir. Then I used /tmp and the error occured. Calling rewind() seems to fix the problem.

@ghost
Copy link

ghost commented Apr 28, 2014

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

@DeepDiver1975
Copy link
Member

@icewind1991 @bantu @karlitschek THX

@bantu
Copy link

bantu commented Apr 28, 2014

I'll take it.

@bantu bantu self-assigned this Apr 28, 2014
@@ -44,17 +44,21 @@ public function rmdir($path) {
new \RecursiveDirectoryIterator($this->datadir . $path),
\RecursiveIteratorIterator::CHILD_FIRST
);
foreach ($it as $file) {
$it->rewind();
Copy link

Choose a reason for hiding this comment

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

Please add a comment explaining why foreach (which would be prefered over while) is not used. Make sure to link to the bug ticket in question, i.e. #8376

@bantu
Copy link

bantu commented Apr 28, 2014

Patch might be incomplete. There is another match for RecursiveDirectoryIterator in

./apps/files_trashbin/lib/trashbin.php:     $iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($root), \RecursiveIteratorIterator::CHILD_FIRST);

and

./lib/private/files/storage/mappedlocal.php:                new \RecursiveDirectoryIterator($this->buildPath($path)),

although the last one probably does not have to be changed.

@icewind1991
Copy link
Contributor

Code looks good 👍 with some comments about the issue

@ghost
Copy link

ghost commented Apr 28, 2014

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

@ghost
Copy link

ghost commented Apr 28, 2014

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

@@ -883,11 +883,19 @@ private static function calculateSize($view) {
$iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($root), \RecursiveIteratorIterator::CHILD_FIRST);
$size = 0;

foreach ($iterator as $path) {
/**
Copy link

Choose a reason for hiding this comment

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

Remaining spaces here apparently.

@bantu
Copy link

bantu commented Apr 29, 2014

Looks good otherwise. 👍

@scrutinizer-notifier
Copy link

The inspection completed: 1 new issues

@ghost
Copy link

ghost commented Apr 29, 2014

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

@PVince81
Copy link
Contributor

PVince81 commented May 2, 2014

@pluijm has already fixed the requested issues and two thumbs up, tests passed.
Merging.

PVince81 pushed a commit that referenced this pull request May 2, 2014
RecursiveDirectoryIterator does not work on NFS
@PVince81 PVince81 merged commit 9e18be6 into owncloud:master May 2, 2014
@pluijm pluijm deleted the issue_8376 branch May 3, 2014 07:52
@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.

6 participants