Skip to content

fix(deprecation): stop using OC_App and non-public AppManager methods - #960

Merged
oleksandr-nc merged 1 commit into
mainfrom
fix/deprecated-oc-app-usage
Aug 3, 2026
Merged

fix(deprecation): stop using OC_App and non-public AppManager methods#960
oleksandr-nc merged 1 commit into
mainfrom
fix/deprecated-oc-app-usage

Conversation

@oleksandr-nc

Copy link
Copy Markdown
Contributor

OC_App is scheduled for removal, so force() now uses the already-injected IAppManager::cleanAppId().

Worth noting the two are not equivalent: OC_App::cleanAppId() strips a denylist of characters, while IAppManager::cleanAppId() applies an allowlist (lowercase alphanumeric plus _ and -). Core does the same thing in the appstore ApiController::enableApp(). I ran the real ExApp and app ids through both and none of them change.

Removing the OC_App usage also dropped its psalm baseline entry, which turned out to be masking a second issue in the same method: overwriteNextcloudRequirement() is not part of IAppManager at all, it only exists on the concrete OC\App\AppManager. There is no public API for it (enableApp($appId, forceEnable: true) is public and does call it, but it also enables the app and calls getAppPath() first, which throws for ExApps since they have no app directory on disk). All that method does is append the id to the app_install_overwrite system value, which this controller already reads back through IConfig a few hundred lines up, so the write now goes through IConfig as well and both sides are symmetric. No private API left in this path.

It would still be nicer if core exposed overwriteNextcloudRequirement() on IAppManager. Happy to open a server issue for that.

force() had no test coverage, so I added some.

Fixes #906

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f4e4794-86b4-41b6-820d-8dd5dbeed322

📥 Commits

Reviewing files that changed from the base of the PR and between 183d25c and 135d560.

📒 Files selected for processing (3)
  • lib/Controller/ExAppsPageController.php
  • tests/php/Controller/ExAppsPageControllerTest.php
  • tests/psalm-baseline.xml
💤 Files with no reviewable changes (1)
  • tests/psalm-baseline.xml
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/Controller/ExAppsPageController.php

📝 Walkthrough

Walkthrough

Updated force() to clean app IDs through IAppManager and manage app_install_overwrite directly. The method resets invalid non-array values, appends missing IDs, and avoids duplicate writes. Tests now cover these cases and use a fixture-level IAppManager mock.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes replacing deprecated OC_App usage and non-public AppManager methods.
Description check ✅ Passed The description accurately explains the deprecated API replacement, configuration update, Psalm change, and added tests.
Linked Issues check ✅ Passed The PR satisfies issue #906 by replacing OC_App::cleanAppId() with the injected IAppManager method.
Out of Scope Changes check ✅ Passed The implementation, tests, and Psalm baseline update directly support the linked issue and stated PR objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 94d37e51-062d-402b-93f1-aae597bdcb2a

📥 Commits

Reviewing files that changed from the base of the PR and between aa7c470 and 0e6e2c7.

📒 Files selected for processing (3)
  • lib/Controller/ExAppsPageController.php
  • tests/php/Controller/ExAppsPageControllerTest.php
  • tests/psalm-baseline.xml
💤 Files with no reviewable changes (1)
  • tests/psalm-baseline.xml

Comment thread tests/php/Controller/ExAppsPageControllerTest.php Outdated
@oleksandr-nc
oleksandr-nc force-pushed the fix/deprecated-oc-app-usage branch from 0e6e2c7 to 183d25c Compare August 3, 2026 11:09
Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc
oleksandr-nc force-pushed the fix/deprecated-oc-app-usage branch from 183d25c to 135d560 Compare August 3, 2026 12:14

@marcelklehr marcelklehr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@oleksandr-nc
oleksandr-nc merged commit cbf0706 into main Aug 3, 2026
54 checks passed
@oleksandr-nc
oleksandr-nc deleted the fix/deprecated-oc-app-usage branch August 3, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecated usage of OC_App

2 participants