fix(deprecation): stop using OC_App and non-public AppManager methods - #960
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (3)
lib/Controller/ExAppsPageController.phptests/php/Controller/ExAppsPageControllerTest.phptests/psalm-baseline.xml
💤 Files with no reviewable changes (1)
- tests/psalm-baseline.xml
0e6e2c7 to
183d25c
Compare
Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
183d25c to
135d560
Compare
OC_Appis scheduled for removal, soforce()now uses the already-injectedIAppManager::cleanAppId().Worth noting the two are not equivalent:
OC_App::cleanAppId()strips a denylist of characters, whileIAppManager::cleanAppId()applies an allowlist (lowercase alphanumeric plus_and-). Core does the same thing in the appstoreApiController::enableApp(). I ran the real ExApp and app ids through both and none of them change.Removing the
OC_Appusage also dropped its psalm baseline entry, which turned out to be masking a second issue in the same method:overwriteNextcloudRequirement()is not part ofIAppManagerat all, it only exists on the concreteOC\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 callsgetAppPath()first, which throws for ExApps since they have no app directory on disk). All that method does is append the id to theapp_install_overwritesystem value, which this controller already reads back throughIConfiga few hundred lines up, so the write now goes throughIConfigas well and both sides are symmetric. No private API left in this path.It would still be nicer if core exposed
overwriteNextcloudRequirement()onIAppManager. Happy to open a server issue for that.force()had no test coverage, so I added some.Fixes #906
🤖 AI (if applicable)