Merge pull request #37744 from nextcloud/backport/36690/stable25

[stable25] fix: Make sure that rollback hook is triggered on all version backends
nextcloud · Jul 10, 2023 · a4b7bb7 · a4b7bb7
2 parents 2ec0a9f + 5121cca
commit a4b7bb7
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 10 deletions.
diff --git a/apps/files_versions/lib/Storage.php b/apps/files_versions/lib/Storage.php
@@ -404,12 +404,6 @@ public static function rollback(string $file, int $revision, IUser $user) {
 
 			$node = $userFolder->get($file);
 
-			// TODO: move away from those legacy hooks!
-			\OC_Hook::emit('\OCP\Versions', 'rollback', [
-				'path' => $filename,
-				'revision' => $revision,
-				'node' => $node,
-			]);
 			return true;
 		} elseif ($versionCreated) {
 			self::deleteVersion($users_view, $version);

diff --git a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php
@@ -58,6 +58,14 @@ public function getVersionsForFile(IUser $user, FileInfo $file): array {
 		if ($storage->instanceOfStorage(SharedStorage::class)) {
 			$owner = $storage->getOwner('');
 			$user = $this->userManager->get($owner);
+
+			$userFolder = $this->rootFolder->getUserFolder($user->getUID());
+			$nodes = $userFolder->getById($file->getId());
+			$file = array_pop($nodes);
+
+			if (!$file) {
+				throw new NotFoundException("version file not found for share owner");
+			}
 		}
 
 		$userFolder = $this->rootFolder->getUserFolder($user->getUID());

diff --git a/apps/files_versions/lib/Versions/VersionManager.php b/apps/files_versions/lib/Versions/VersionManager.php
@@ -99,7 +99,16 @@ public function createVersion(IUser $user, FileInfo $file) {
 
 	public function rollback(IVersion $version) {
 		$backend = $version->getBackend();
-		return self::handleAppLocks(fn(): ?bool => $backend->rollback($version));
+		$result = self::handleAppLocks(fn(): ?bool => $backend->rollback($version));
+		// rollback doesn't have a return type yet and some implementations don't return anything
+		if ($result === null || $result === true) {
+			\OC_Hook::emit('\OCP\Versions', 'rollback', [
+				'path' => $version->getVersionPath(),
+				'revision' => $version->getRevisionId(),
+				'node' => $version->getSourceFile(),
+			]);
+		}
+		return $result;
 	}
 
 	public function read(IVersion $version) {

diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php
@@ -38,6 +38,8 @@
 use OCP\IConfig;
 use OCP\IUser;
 use OCP\Share\IShare;
+use OCA\Files_Versions\Versions\IVersionManager;
+use Test\Traits\MountProviderTrait;
 
 /**
  * Class Test_Files_versions
@@ -49,6 +51,7 @@ class VersioningTest extends \Test\TestCase {
 	public const TEST_VERSIONS_USER = 'test-versions-user';
 	public const TEST_VERSIONS_USER2 = 'test-versions-user2';
 	public const USERS_VERSIONS_ROOT = '/test-versions-user/files_versions';
+	use MountProviderTrait;
 
 	/**
 	 * @var \OC\Files\View
@@ -646,7 +649,8 @@ public function testRestoreSameStorage() {
 
 	public function testRestoreCrossStorage() {
 		$storage2 = new Temporary([]);
-		\OC\Files\Filesystem::mount($storage2, [], self::TEST_VERSIONS_USER . '/files/sub');
+		$this->registerMount(self::TEST_VERSIONS_USER, $storage2, self::TEST_VERSIONS_USER . '/files/sub');
+		$this->loginAsUser(self::TEST_VERSIONS_USER);
 
 		$this->doTestRestore();
 	}
@@ -789,7 +793,12 @@ private function doTestRestore() {
 		$params = [];
 		$this->connectMockHooks('rollback', $params);
 
-		$this->assertTrue(\OCA\Files_Versions\Storage::rollback('sub/test.txt', $t2, $this->user1));
+		$versionManager = \OCP\Server::get(IVersionManager::class);
+		$versions = $versionManager->getVersionsForFile($this->user1, $info1);
+		$version = array_filter($versions, function ($version) use ($t2) {
+			return $version->getRevisionId() === $t2;
+		});
+		$this->assertTrue($versionManager->rollback(current($version)));
 		$expectedParams = [
 			'path' => '/sub/test.txt',
 		];

diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php
@@ -326,7 +326,7 @@ protected function getAppDataDirectoryName(): string {
 	 * @param int $id
 	 * @return array
 	 */
-	protected function 	getByIdInRootMount(int $id): array {
+	protected function getByIdInRootMount(int $id): array {
 		if (!method_exists($this->root, 'createNode')) {
 			// Always expected to be false. Being a method of Folder, this is
 			// always implemented. For it is an internal method and should not