chore(files_versions): Use new metadata API for versions by artonge · Pull Request #44175 · nextcloud/server

artonge · 2024-03-13T12:25:26Z

Use the new version metadata API for handling the label logic.

@emoral435 I also tweaked the API a bit, I left some comments to give some reasoning behind the changes.

apps/files_versions/lib/Sabre/Plugin.php

apps/files_versions/lib/Versions/LegacyVersionsBackend.php

artonge · 2024-03-13T12:30:21Z

apps/files_versions/lib/Sabre/VersionFile.php

+		if ($backend instanceof IMetadataVersionBackend) {
+			$backend->setMetadataValue($this->version, $key, $value);
 			return true;
+		} elseif ($key === 'label' && $backend instanceof INameableVersionBackend) {


Ensure compatibility with backends that are not migrated yet.

artonge · 2024-03-13T12:33:43Z

apps/files_versions/lib/Versions/IMetadataVersionBackend.php

-	 * @param string $key the key for the json value of the metadata column
-	 * @since 29.0.0
-	 */
-	public function getMetadataValue(Node $node, string $key): ?string;


The metadata can already be retrieved from the IMetadataVersion. So this method seem unnecessary. @emoral435 does it make sense to remove it? Can still be added later if needed, but I don't see a reason for it.

Agreed 👍

artonge · 2024-03-13T12:42:02Z

apps/files_versions/lib/Versions/Version.php

-		if ($this->backend instanceof IMetadataVersionBackend && $this->sourceFileInfo instanceof Node) {
-			return $this->backend->getMetadataValue($this->sourceFileInfo, "author");
-		}
-		return null;


The constructor now receives the metadata, so we can simplify the logic here.

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge · 2024-03-13T13:55:24Z

apps/files_versions/lib/Sabre/VersionFile.php

 	public function getMetadataValue(string $key): ?string {
 		if ($this->version instanceof IMetadataVersion) {
 			return $this->version->getMetadataValue($key);
+		} elseif ($key === 'label' && $this->version instanceof INameableVersion) {


Ensure compatibility with backends that are not migrated yet.

artonge · 2024-03-13T13:59:54Z

apps/files_versions/lib/Versions/IMetadataVersionBackend.php

-	 * @since 29.0.0
-	 */
-	public function getMetadataValue(Node $node, string $key): ?string;
+	public function setMetadataValue(Node $node, int $revision, string $key, string $value): void;


I added a $revision param to be able to target a specific version. @emoral435, ok with you?

I like this change! Ok with me.

apps/files_versions/lib/Sabre/VersionFile.php

-		if ($this->versionManager instanceof INameableVersionBackend) {
-			$this->versionManager->setVersionLabel($this->version, $label);
+		if ($backend instanceof IMetadataVersionBackend) {
+			$backend->setMetadataValue($this->version->getSourceFile(), $this->version->getRevisionId(), $key, $value);


apps/files_versions/lib/Sabre/VersionFile.php

-		if ($this->versionManager instanceof INameableVersionBackend) {
-			$this->versionManager->setVersionLabel($this->version, $label);
+		if ($backend instanceof IMetadataVersionBackend) {
+			$backend->setMetadataValue($this->version->getSourceFile(), $this->version->getRevisionId(), $key, $value);


apps/files_versions/lib/Sabre/VersionFile.php

+			$backend->setMetadataValue($this->version->getSourceFile(), $this->version->getRevisionId(), $key, $value);
+			return true;
+		} elseif ($key === 'label' && $backend instanceof INameableVersionBackend) {
+			$backend->setVersionLabel($this->version, $value);


apps/files_versions/lib/Sabre/VersionFile.php

 		if ($this->version instanceof IMetadataVersion) {
 			return $this->version->getMetadataValue($key);
+		} elseif ($key === 'label' && $this->version instanceof INameableVersion) {
+			return $this->version->getLabel();


Signed-off-by: Louis Chemineau <louis@chmn.me>

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

emoral435

I like the new changes. The capability to store whatever metadata we want and get whichever version's metadata seems really flexible :) I do not know if this PR is ready as I have not been marked down as a reviewer, though I approve the current changes. Ping me if you need me to approve!

emoral435 · 2024-03-13T14:41:03Z

apps/files_versions/lib/Versions/IMetadataVersionBackend.php

-	 * @since 29.0.0
-	 */
-	public function getMetadataValue(Node $node, string $key): ?string;
+	public function setMetadataValue(Node $node, int $revision, string $key, string $value): void;


I like this change! Ok with me.

emoral435 · 2024-03-13T14:52:26Z

apps/files_versions/lib/Versions/IMetadataVersionBackend.php

-	 * @param string $key the key for the json value of the metadata column
-	 * @since 29.0.0
-	 */
-	public function getMetadataValue(Node $node, string $key): ?string;


Agreed 👍

artonge · 2024-03-13T15:30:23Z

I do not know if this PR is ready as I have not been marked down as a reviewer

I was waiting for CI to be green. Now it's ok, and your approval would be appreciated. :)

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge self-assigned this Mar 13, 2024

github-advanced-security bot found potential problems Mar 13, 2024

View reviewed changes

apps/files_versions/lib/Sabre/Plugin.php Fixed Show fixed Hide fixed

apps/files_versions/lib/Versions/LegacyVersionsBackend.php Fixed Show fixed Hide fixed

artonge commented Mar 13, 2024

View reviewed changes

artonge requested review from emoral435 and removed request for emoral435 March 13, 2024 12:42

artonge added 2. developing Work in progress feature: versions technical debt php Pull requests that update Php code labels Mar 13, 2024

artonge added this to the Nextcloud 29 milestone Mar 13, 2024

artonge force-pushed the artonge/chore/use_new_version_metadata_API branch from d375386 to 28ee8af Compare March 13, 2024 12:44

artonge marked this pull request as ready for review March 13, 2024 12:44

artonge force-pushed the artonge/chore/use_new_version_metadata_API branch from 28ee8af to cf75f46 Compare March 13, 2024 12:49

artonge added a commit to nextcloud/groupfolders that referenced this pull request Mar 13, 2024

chore(files_versions): Use new metadata API for versions

fbebb4c

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge mentioned this pull request Mar 13, 2024

chore(files_versions): Use new metadata API for versions nextcloud/groupfolders#2857

Merged

artonge force-pushed the artonge/chore/use_new_version_metadata_API branch 2 times, most recently from 8aeb048 to 0fad147 Compare March 13, 2024 13:54

artonge commented Mar 13, 2024

View reviewed changes

github-advanced-security bot found potential problems Mar 13, 2024

View reviewed changes

chore(files_versions): Use new metadata API for versions

9361a30

Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge force-pushed the artonge/chore/use_new_version_metadata_API branch from 0fad147 to 9361a30 Compare March 13, 2024 14:27

artonge added a commit to nextcloud/groupfolders that referenced this pull request Mar 13, 2024

chore(files_versions): Use new metadata API for versions

2a8062a

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

emoral435 reviewed Mar 13, 2024

View reviewed changes

artonge requested review from come-nc and icewind1991 March 13, 2024 15:29

artonge added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 13, 2024

emoral435 approved these changes Mar 13, 2024

View reviewed changes

skjnldsv approved these changes Mar 14, 2024

View reviewed changes

skjnldsv merged commit 5764f03 into master Mar 14, 2024

skjnldsv deleted the artonge/chore/use_new_version_metadata_API branch March 14, 2024 07:23

Altahrim mentioned this pull request Mar 14, 2024

29.0.0 beta 3 #44186

Merged

artonge added a commit to nextcloud/groupfolders that referenced this pull request Mar 18, 2024

chore(files_versions): Use new metadata API for versions

cf5fb4b

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge added a commit to nextcloud/groupfolders that referenced this pull request Mar 18, 2024

chore(files_versions): Use new metadata API for versions

79ed5ad

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

artonge added a commit to nextcloud/groupfolders that referenced this pull request Mar 18, 2024

chore(files_versions): Use new metadata API for versions

bb59297

As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>

Uh oh!

Conversation

artonge commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artonge Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Check notice

Check notice

Check notice

emoral435 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artonge commented Mar 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

artonge commented Mar 13, 2024 •

edited

Loading

artonge Mar 13, 2024 •

edited

Loading