[trashbin] replace hook with storage wrapper #13377

schiessle · 2015-01-15T11:02:10Z

This is a minimal approach to replace the delete hook with a storage wrapper. This allows us to perform a move instead of a copy while moving files to the trash bin which will give us a huge performance improvement.

cc @DeepDiver1975 @icewind1991

DeepDiver1975 · 2015-01-15T11:19:21Z

@schiesbn

local unit test execution helps ;-)

00:02:13.738 PHP Fatal error:  Class 'OCA\Files_Trashbin\OCP\Util' not found in /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/apps/files_trashbin/lib/trashbin.php on line 927
00:02:13.738 PHP Stack trace:
00:02:13.738 PHP   1. {main}() /usr/local/bin/phpunit:0
00:02:13.738 PHP   2. PHPUnit_TextUI_Command::main() /usr/local/bin/phpunit:612
00:02:13.738 PHP   3. PHPUnit_TextUI_Command->run() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:138
00:02:13.738 PHP   4. PHPUnit_TextUI_Command->handleArguments() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:148
00:02:13.738 PHP   5. PHPUnit_TextUI_Command->handleBootstrap() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:651
00:02:13.738 PHP   6. PHPUnit_Util_Fileloader::checkAndLoad() phar:///usr/local/bin/phpunit/phpunit/TextUI/Command.php:817
00:02:13.738 PHP   7. PHPUnit_Util_Fileloader::load() phar:///usr/local/bin/phpunit/phpunit/Util/Fileloader.php:77
00:02:13.738 PHP   8. include_once() phar:///usr/local/bin/phpunit/phpunit/Util/Fileloader.php:93
00:02:13.738 PHP   9. OC_App::loadApps() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/tests/bootstrap.php:15
00:02:13.739 PHP  10. OC_App::loadApp() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/app.php:77
00:02:13.739 PHP  11. OC_App::requireAppFile() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/app.php:98
00:02:13.739 PHP  12. require_once() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/lib/private/app.php:117
00:02:13.739 PHP  13. OCA\Files_Trashbin\Trashbin::registerHooks() /var/jenkins/workspace/pull-request-analyser-ng-simple@5/label/SLAVE/apps/files_trashbin/appinfo/app.php:5
00:02:13.986 Build step 'Execute shell' marked build as failure

DeepDiver1975 · 2015-01-15T11:19:44Z

local unit test execution helps ;-)

and using a proper ide 🙊

DeepDiver1975 · 2015-01-15T11:21:31Z

apps/files_trashbin/lib/storage.php

+	private $mountPoint;
+	// remember already deleted files to avoid infinite loop if the trash bin
+	// move files across storages
+	private static $deletedFiles = array();


does this have to be static? I mean: will there ever be files being deleted in two different storage implementations?

MorrisJobke · 2015-01-15T12:14:10Z

@schiesbn Can I ask you to label your PRs too? something like "4 - Developing" for WIP PRs or "5 - To review" for ready to review PRs. Then stuff like "app:files_trashbin" to label the component. This helps a lot to master the huge amount of PRs/issues and are just 3 clicks away ;)

MorrisJobke · 2015-01-15T19:59:53Z

There were 4 errors:
00:38:10.273 
00:38:10.273 1) Test_Files_Sharing_Mount::testMoveGroupShare
00:38:10.275 Invalid argument supplied for foreach()
00:38:10.275 
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/cache/scanner.php:226
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/cache/scanner.php:248
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/cache/scanner.php:216
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:930
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/filesystem.php:782
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/trashbin.php:39
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/trashbin.php:145
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/storage.php:48
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:792
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:437
00:38:10.275 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/tests/sharedmount.php:45
00:38:10.276 
00:38:10.276 2) Test_Files_Sharing::testUnshareFromSelf
00:38:10.277 OCA\Files_Sharing\Exceptions\BrokenPath: Path does not start with /user/files
00:38:10.277 
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/lib/sharedmount.php:103
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/lib/sharedmount.php:121
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:499
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/trashbin.php:170
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/storage.php:48
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:792
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:437
00:38:10.277 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/tests/share.php:53
00:38:10.277 
00:38:10.277 3) Test_Files_Sharing::testShareAndUnshareFromSelf
00:38:10.278 OCA\Files_Sharing\Exceptions\BrokenPath: Path does not start with /user/files
00:38:10.278 
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/lib/sharedmount.php:103
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/lib/sharedmount.php:121
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:499
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/trashbin.php:170
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_trashbin/lib/storage.php:48
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:792
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/files/view.php:437
00:38:10.278 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/tests/share.php:53
00:38:10.278 
00:38:10.278 4) Test_Files_Sharing::testShareWithDifferentShareFolder
00:38:10.280 Exception: Sharing 5 failed, because this item is already shared with test-share-user2
00:38:10.280 
00:38:10.280 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/private/share/share.php:597
00:38:10.280 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/lib/public/share.php:236
00:38:10.280 /var/jenkins/workspace/pull-request-analyser-ng-simple@2/label/SLAVE/apps/files_sharing/tests/share.php:231

MorrisJobke · 2015-01-16T12:20:25Z

apps/files_trashbin/lib/trashbin.php

@@ -192,6 +194,8 @@ public static function move2trash($file_path) {
 			if ($user !== $owner) {
 				self::copyFilesToOwner($file_path, $owner, $ownerPath, $timestamp);
 			}
+
+			$result = true;


From scrutinizer:

The assignment to $result is dead and can be removed.

scrutinizer-notifier · 2015-01-19T08:29:13Z

The inspection completed: 5 updated code elements

ghost · 2015-01-19T08:43:07Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/7234/
👍 Test PASSed. 👍

DeepDiver1975 · 2015-01-19T09:43:28Z

tested 👍

DeepDiver1975 · 2015-01-19T09:43:52Z

@icewind1991 @MorrisJobke @PVince81 can you please test this? THX

RobinMcCorkell · 2015-01-19T11:22:14Z

Tested with/without external storages, 👍

[trashbin] replace hook with storage wrapper

MorrisJobke · 2015-02-20T09:50:28Z

@schiesbn I got a question from @cdamken Is this the PR that fixes owncloud/client#2858 and is it reasonable/possible to backport this to stable7 ? cc @icewind1991

DeepDiver1975 · 2015-02-20T10:12:44Z

@schiesbn I got a question from @cdamken Is this the PR that fixes owncloud/client#2858 and is it reasonable/possible to backport this to stable7 ? cc @icewind1991

NO - additional fixes had to be applied on top - too risky - upgrade to 8 of disable trashbin

DeepDiver1975 added this to the 8.0-current milestone Jan 15, 2015

DeepDiver1975 reviewed Jan 15, 2015
View reviewed changes

MorrisJobke added 3 - To Review app:files_trashbin labels Jan 15, 2015

schiessle added 2 - Developing and removed 3 - To Review labels Jan 15, 2015

schiessle force-pushed the trashbin_storage_wrapper branch from 3f111cb to 5c7b6be Compare January 15, 2015 19:15

schiessle force-pushed the trashbin_storage_wrapper branch from 5c7b6be to f67942b Compare January 16, 2015 11:54

schiessle added 3 - To Review and removed 2 - Developing labels Jan 16, 2015

MorrisJobke reviewed Jan 16, 2015
View reviewed changes

schiessle force-pushed the trashbin_storage_wrapper branch 2 times, most recently from beae124 to 379dcd8 Compare January 16, 2015 13:31

replace hook with storage wrapper

15ae6b4

DeepDiver1975 force-pushed the trashbin_storage_wrapper branch from 379dcd8 to 15ae6b4 Compare January 19, 2015 08:16

MorrisJobke added a commit that referenced this pull request Jan 19, 2015

Merge pull request #13377 from owncloud/trashbin_storage_wrapper

cd4c7fd

[trashbin] replace hook with storage wrapper

MorrisJobke merged commit cd4c7fd into master Jan 19, 2015

MorrisJobke deleted the trashbin_storage_wrapper branch January 19, 2015 12:17

schiessle mentioned this pull request Jan 19, 2015

Server-to-server mount fails on master #13491

Closed

This was referenced Jan 23, 2015

Delete file on non-local storage does not properly delete file #13557

Closed

Versions lost when moving file to or out of external storage root #13634

Closed

PVince81 mentioned this pull request Feb 1, 2015

Deleting big files within webinterface takes long time #12866

Closed

MorrisJobke mentioned this pull request Feb 20, 2015

Deleting files and emptying trashbin not possible! owncloud/client#2858

Closed

This was referenced Mar 5, 2015

Deleting documents takes so time #9217

Closed

Trashbin auto-expire causes PHP timeout on delete #13974

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[trashbin] replace hook with storage wrapper #13377

[trashbin] replace hook with storage wrapper #13377

schiessle commented Jan 15, 2015

DeepDiver1975 commented Jan 15, 2015

DeepDiver1975 commented Jan 15, 2015

DeepDiver1975 Jan 15, 2015

MorrisJobke commented Jan 15, 2015

MorrisJobke commented Jan 15, 2015

MorrisJobke Jan 16, 2015

schiessle Jan 16, 2015

scrutinizer-notifier commented Jan 19, 2015

ghost commented Jan 19, 2015

DeepDiver1975 commented Jan 19, 2015

DeepDiver1975 commented Jan 19, 2015

RobinMcCorkell commented Jan 19, 2015

MorrisJobke commented Feb 20, 2015

DeepDiver1975 commented Feb 20, 2015

[trashbin] replace hook with storage wrapper #13377

[trashbin] replace hook with storage wrapper #13377

Conversation

schiessle commented Jan 15, 2015

DeepDiver1975 commented Jan 15, 2015

DeepDiver1975 commented Jan 15, 2015

DeepDiver1975 Jan 15, 2015

Choose a reason for hiding this comment

MorrisJobke commented Jan 15, 2015

MorrisJobke commented Jan 15, 2015

MorrisJobke Jan 16, 2015

Choose a reason for hiding this comment

schiessle Jan 16, 2015

Choose a reason for hiding this comment

scrutinizer-notifier commented Jan 19, 2015

ghost commented Jan 19, 2015

DeepDiver1975 commented Jan 19, 2015

DeepDiver1975 commented Jan 19, 2015

RobinMcCorkell commented Jan 19, 2015

MorrisJobke commented Feb 20, 2015

DeepDiver1975 commented Feb 20, 2015