Uh oh!
There was an error while loading. Please reload this page.
chore(files_versions): Use new metadata API for versions - #44175
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| if ($backend instanceof IMetadataVersionBackend) { | ||
| $backend->setMetadataValue($this->version, $key, $value); | ||
| return true; | ||
| } elseif ($key === 'label' && $backend instanceof INameableVersionBackend) { |
There was a problem hiding this comment.
Ensure compatibility with backends that are not migrated yet.
| * @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; |
There was a problem hiding this comment.
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.
| if ($this->backend instanceof IMetadataVersionBackend && $this->sourceFileInfo instanceof Node) { | ||
| return $this->backend->getMetadataValue($this->sourceFileInfo, "author"); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
The constructor now receives the metadata, so we can simplify the logic here.
d375386 to
28ee8afCompare28ee8af to
cf75f46CompareAs of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>
8aeb048 to
0fad147Compare| public function getMetadataValue(string $key): ?string { | ||
| if ($this->version instanceof IMetadataVersion) { | ||
| return $this->version->getMetadataValue($key); | ||
| } elseif ($key === 'label' && $this->version instanceof INameableVersion) { |
There was a problem hiding this comment.
Ensure compatibility with backends that are not migrated yet.
| * @since 29.0.0 | ||
| */ | ||
| public function getMetadataValue(Node $node, string $key): ?string; | ||
| public function setMetadataValue(Node $node, int $revision, string $key, string $value): void; |
There was a problem hiding this comment.
I added a $revision param to be able to target a specific version. @emoral435, ok with you?
There was a problem hiding this comment.
I like this change! Ok with me.
| 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); |
Check notice
Code scanning / Psalm
ArgumentTypeCoercion
| 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); |
Check notice
Code scanning / Psalm
PossiblyInvalidArgument
| $backend->setMetadataValue($this->version->getSourceFile(), $this->version->getRevisionId(), $key, $value); | ||
| return true; | ||
| } elseif ($key === 'label' && $backend instanceof INameableVersionBackend) { | ||
| $backend->setVersionLabel($this->version, $value); |
Check notice
Code scanning / Psalm
DeprecatedMethod
| if ($this->version instanceof IMetadataVersion) { | ||
| return $this->version->getMetadataValue($key); | ||
| } elseif ($key === 'label' && $this->version instanceof INameableVersion) { | ||
| return $this->version->getLabel(); |
Check notice
Code scanning / Psalm
DeprecatedMethod
Signed-off-by: Louis Chemineau <louis@chmn.me>
0fad147 to
9361a30CompareAs of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 Signed-off-by: Louis Chemineau <louis@chmn.me>
emoral435
left a comment
There was a problem hiding this comment.
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!
| * @since 29.0.0 | ||
| */ | ||
| public function getMetadataValue(Node $node, string $key): ?string; | ||
| public function setMetadataValue(Node $node, int $revision, string $key, string $value): void; |
There was a problem hiding this comment.
I like this change! Ok with me.
| * @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; |
artonge
commented
Mar 13, 2024
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>
As of: nextcloud/server#44049 Server equivalent: nextcloud/server#44175 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>
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.
As of: #44049