From 1b0c5e57e5e4042ebb4f0e5956330a908645b61a Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 22 Nov 2013 18:18:08 +0100 Subject: [PATCH 1/3] Fixed SMB rename function to overwrite target file When uploading files through WebDAV, a part file is created and a rename operation is performed with the expectation that the part file overwrites an existing file, if any. This fix makes the SMB external storage delete the target file before renaming, as smbclient doesn't support overwrite on move/rename. Fixes #5348 --- apps/files_external/3rdparty/smb4php/smb.php | 16 +++++++++++++++- apps/files_external/lib/streamwrapper.php | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/apps/files_external/3rdparty/smb4php/smb.php b/apps/files_external/3rdparty/smb4php/smb.php index 9650f80904..87638271f0 100644 --- a/apps/files_external/3rdparty/smb4php/smb.php +++ b/apps/files_external/3rdparty/smb4php/smb.php @@ -302,6 +302,7 @@ class smb { } function rename ($url_from, $url_to) { + $replace = false; list ($from, $to) = array (smb::parse_url($url_from), smb::parse_url($url_to)); if ($from['host'] <> $to['host'] || $from['share'] <> $to['share'] || @@ -314,7 +315,20 @@ class smb { trigger_error('rename(): error in URL', E_USER_ERROR); } smb::clearstatcache ($url_from); - $result = smb::execute ('rename "'.$from['path'].'" "'.$to['path'].'"', $to); + $cmd = ''; + // check if target file exists + if (smb::url_stat($url_to)) { + // delete target file first + $cmd = 'del "' . $to['path'] . '"; '; + $replace = true; + } + $cmd .= 'rename "' . $from['path'] . '" "' . $to['path'] . '"'; + $result = smb::execute($cmd, $to); + if ($replace) { + // clear again, else the cache will return the info + // from the old file + smb::clearstatcache ($url_to); + } return $result !== false; } diff --git a/apps/files_external/lib/streamwrapper.php b/apps/files_external/lib/streamwrapper.php index 23c5f91a2f..4f3dc889b0 100644 --- a/apps/files_external/lib/streamwrapper.php +++ b/apps/files_external/lib/streamwrapper.php @@ -38,7 +38,7 @@ abstract class StreamWrapper extends Common { } public function filetype($path) { - return filetype($this->constructUrl($path)); + return @filetype($this->constructUrl($path)); } public function file_exists($path) { From af7118aa5db19165bcded3c860b3163488d7b6f6 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 25 Nov 2013 18:52:14 +0100 Subject: [PATCH 2/3] Added unit test for "overwrite file on rename/move" Also fixed "rename" unit test that was ready the result out of the wrong file. --- tests/lib/files/storage/storage.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/lib/files/storage/storage.php b/tests/lib/files/storage/storage.php index 6c433e9547..be534635ec 100644 --- a/tests/lib/files/storage/storage.php +++ b/tests/lib/files/storage/storage.php @@ -139,6 +139,12 @@ abstract class Storage extends \PHPUnit_Framework_TestCase { $this->instance->rename('/source.txt', '/target2.txt'); $this->assertTrue($this->instance->file_exists('/target2.txt')); $this->assertFalse($this->instance->file_exists('/source.txt')); + $this->assertEquals(file_get_contents($textFile), $this->instance->file_get_contents('/target2.txt')); + + // move to overwrite + $this->instance->rename('/target2.txt', '/target.txt'); + $this->assertTrue($this->instance->file_exists('/target.txt')); + $this->assertFalse($this->instance->file_exists('/target2.txt')); $this->assertEquals(file_get_contents($textFile), $this->instance->file_get_contents('/target.txt')); } From c3e34676ba0a5467cadc1fd931ed659262705388 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 25 Nov 2013 18:54:58 +0100 Subject: [PATCH 3/3] Improved unit test for "overwrite on move" Now using a different content to make sure the file was overwritten. --- tests/lib/files/storage/storage.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/lib/files/storage/storage.php b/tests/lib/files/storage/storage.php index be534635ec..c627395945 100644 --- a/tests/lib/files/storage/storage.php +++ b/tests/lib/files/storage/storage.php @@ -142,10 +142,12 @@ abstract class Storage extends \PHPUnit_Framework_TestCase { $this->assertEquals(file_get_contents($textFile), $this->instance->file_get_contents('/target2.txt')); // move to overwrite - $this->instance->rename('/target2.txt', '/target.txt'); - $this->assertTrue($this->instance->file_exists('/target.txt')); + $testContents = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; + $this->instance->file_put_contents('/target3.txt', $testContents); + $this->instance->rename('/target2.txt', '/target3.txt'); + $this->assertTrue($this->instance->file_exists('/target3.txt')); $this->assertFalse($this->instance->file_exists('/target2.txt')); - $this->assertEquals(file_get_contents($textFile), $this->instance->file_get_contents('/target.txt')); + $this->assertEquals(file_get_contents($textFile), $this->instance->file_get_contents('/target3.txt')); } public function testLocal() {