-
Notifications
You must be signed in to change notification settings - Fork 9
Make dependency to File Versions App optional #314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
R0Wi
commented
Jul 2, 2025
- Fixes Too few arguments to function OCA\WorkflowOcr\BackgroundJobs\ProcessFileJob #307
a66eecf
to
1330662
Compare
1330662
to
317fd24
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR makes the File Versions App dependency optional in the OCR workflow, allowing OCR processing to proceed even when versioning is disabled.
- Mark
IVersionManager
as a nullable dependency inOcrService
. - Early-return in
setFileVersionsLabel
when the version manager is not provided, with a debug log. - Add a unit test to confirm OCR processing succeeds without a version manager.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/psalm-baseline.xml | Updated baseline to accept ?IVersionManager as a valid type |
tests/Unit/Service/OcrServiceTest.php | Added test testRunOcrProcessWorksWithoutIVersionManager |
lib/Service/OcrService.php | Changed constructor and property to ?IVersionManager and added an early return in setFileVersionsLabel |
Comments suppressed due to low confidence (1)
lib/Service/OcrService.php:98
- Update the constructor's docblock annotations to indicate that $versionManager is now nullable, ensuring IDEs and static analysis reflect the
?IVersionManager
type.
?IVersionManager $versionManager,
@@ -289,6 +289,11 @@ private function createNewFileVersion(string $filePath, string $ocrContent, ?int | |||
* @param string $label The label to set | |||
*/ | |||
private function setFileVersionsLabel(File $file, string $uid, string $label): void { | |||
if ($this->versionManager === null) { | |||
$this->logger->debug('Skipping setting file version label because file version app is not active'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider extracting this log message into a class constant (e.g., SKIP_VERSION_LABEL_LOG
) to avoid duplication between code and tests and simplify future updates.
$this->logger->debug('Skipping setting file version label because file version app is not active'); | |
$this->logger->debug(self::SKIP_VERSION_LABEL_LOG); |
Copilot uses AI. Check for mistakes.
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-stable31 stable31
# Navigate to the new working tree
cd .worktrees/backport-stable31
# Create a new branch
git switch --create backport-314-to-stable31
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick ---mainline 1 ed8436543077c3c35d5b69b3740ca71ab3755dd0
# Push it to GitHub
git push --set-upstream origin backport-314-to-stable31
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-stable31 Then, create a pull request where the |